Since the introduction of the Downloads folder in the Library window, the "Remember download history" checkbox, displayed in the "Privacy" pane when "Use custom settings for history" is selected, became misleading. In fact, the contents of the Downloads folder in the Library window are controlled by the "Remember my browsing history" setting, that should be renamed to "Remember my browsing and download history". The preference controlled by the "Remember download history" checkbox, instead, determines whether successful downloads are immediately removed from the Downloads window (or the new Downloads Panel in bug 564934). The purpose of this preference might be to avoid that recent downloads appear prominently in the main user interface (bug 697678 comment 2). This is orthogonal to Private Browsing Mode. If we still want to support this use case through an option in the Preferences window, we should probably move the checkbox outside the history area and label it something like "Hide downloads as soon as they are finished" (assuming we are inverting its value, for label clarity). Note that, when the Downloads Panel will be enabled, recent downloads will be always cleared when the last browser window is closed.

(In reply to Paolo Amadini from comment #1) > Since the introduction of the Downloads folder in the Library window, the > "Remember download history" checkbox, displayed in the "Privacy" pane when > "Use custom settings for history" is selected, became misleading. > > In fact, the contents of the Downloads folder in the Library window are > controlled by the "Remember my browsing history" setting, that should be > renamed to "Remember my browsing and download history". When relabeling the entity, I will rename it, as it should describe much better the purpose of itself. Then I will need to spread the change to the tests and the scripts.

Uhm... Better to leave ids unmodified (the same with tests source code) because these could be used for some extensions?

(In reply to Javi Rueda from comment #2) > Uhm... Better to leave ids unmodified (the same with tests source code) > because these could be used for some extensions? There isn't a general rule with regard to keeping extension compatibility versus renaming for code clarity. In this case, the ID of the element is "rememberHistory", and I think it's clear that "history" can include both browsing and download history, thus I see no need to change that. You can just rename the entity to "&rememberHistory2;" since the text changed. CC'ing Gavin since he's probably going to review the patch, and maybe has better recommendations.

(In reply to Paolo Amadini from comment #3) > (In reply to Javi Rueda from comment #2) > > Uhm... Better to leave ids unmodified (the same with tests source code) > > because these could be used for some extensions? > > There isn't a general rule with regard to keeping extension compatibility > versus renaming for code clarity. In this case, the ID of the element is > "rememberHistory", and I think it's clear that "history" can include both > browsing and download history, thus I see no need to change that. Yes. > You can just rename the entity to "&rememberHistory2;" since the text > changed. I used the longer string "rememberBrowsingDownloadHistory" for the entity, similar to the entity used for the search and forms history. This string is only used on the DTD and the XUL, so I think the size is not very important. Anyway, should I use your suggestion? > CC'ing Gavin since he's probably going to review the patch, and maybe has > better > recommendations. Good :-)

(In reply to Javi Rueda from comment #4) > > You can just rename the entity to "&rememberHistory2;" since the text > > changed. > > I used the longer string "rememberBrowsingDownloadHistory" for the entity, > similar to the entity used for the search and forms history. This string is > only used on the DTD and the XUL, so I think the size is not very important. > Anyway, should I use your suggestion? Both are fine for me. "rememberHistory2" has the advantage of being similar to the ID and findable when searching for rememberHistory using full text search.

(In reply to Paolo Amadini from comment #6) > If we still want to support this use case through an option in the > Preferences window, we should probably move the checkbox outside the > history area and label it something like "Hide downloads as soon as they > are finished" (assuming we are inverting its value, for label clarity). Then this option, once it has been moved out the area, won't be related to the "Use custom settings"?

(In reply to Javi Rueda from comment #6) > (In reply to Paolo Amadini from comment #6) > > If we still want to support this use case through an option in the > > Preferences window, we should probably move the checkbox outside the > > history area and label it something like "Hide downloads as soon as they > > are finished" (assuming we are inverting its value, for label clarity). > > Then this option, once it has been moved out the area, won't be related to > the "Use custom settings"? From a purely functional viewpoint, the option is unrelated. When presenting it to users, however, there could be additional considerations to make. Actually, I'm not certain we should keep the option in the interface. I've CC'd Alex, who might give us a more informed answer, or refer us to someone that can help with this specific User Experience issue (see bug description in comment 0).

I've finished with the UI based on comment 0. Waiting for Alex to say whatever about what would be the most correct direction.

This is work in progress code while waiting for a decision about what to do with the "Hide downloads when finished". The check-box for remember download history is removed from the History groupbox and is moved to a new "Downloads" group above the History one.

This time it is taken into account the inverted sense of the check-box.

Comment on attachment 583560 [details] [diff] [review] patch 2 WIP Review of attachment 583560 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch! While waiting for UI review, I've tried it locally and wrote a few notes. ::: browser/components/preferences/privacy.xul @@ +130,5 @@ > accesskey="&doNotTrack.accesskey;" > preference="privacy.donottrackheader.enabled"/> > </groupbox> > > + <!-- Downloads --> I'm pretty sure that UX will ask you to put the Downloads section after the History section (in order of relative importance) :-) @@ +134,5 @@ > + <!-- Downloads --> > + <groupbox id="downloadsGroup"> > + <caption label="&downloads.label;"/> > + > + <checkbox id="rememberDownloads" There is still some code that controls the enabled state of this checkbox based on the selected option in the drop-down menu, it should be removed. You should also change the ID, because this checkbox has a different function from the old one.

Same as previous one, but with corrections from comments by Paolo. It also modifies some tests as they still assumed that the original checkbox depended of the History mode selected.

ping.

Comment on attachment 585703 [details] [diff] [review] patch (In reply to Javi Rueda from comment #13) Hi Javi, thanks for the reminder! Starting from a few days ago, there is a new process in place for requesting review for user interface changes, consisting in putting the new ux-review mailing list in the requestee field (I've edited your current request). If you can attach a screenshot of the new interface and a brief explanation of the changes, it can facilitate their review. They have a goal of responding to requests within a couple of (working) days. https://developer.mozilla.org/En/Developer_Guide/Requesting_feedback_and_ui-review_for_desktop_Firefox_front-end_changes

This screenshot was taken in order to show faster for the reviewer the result of apply the patch from attachment 585703 [details] [diff] [review].

Explanation to the patch and screenshot: A new group-box is added to the Privacy tab in the options dialog box. This group includes the checkbox that controls the preference value for the "remember downloads once they are finished" setting. This checkbox was included until now in the history customization group of preferences, but it was not related to it as explained on this bug.

Comment on attachment 585703 [details] [diff] [review] patch Review of attachment 585703 [details] [diff] [review]: ----------------------------------------------------------------- I think showing this as an option doesn't make a lot of sense in the new implementation. I would just rename the checkbox "remember my browsing and download history" — as you have already done — and not show a second checkbox for automatically clearing the download listing in the panel. This seems like an edge case, and could exist as an about:config option, and possibly even have a super-simple add-on for adding in a checkbox to control this. ui-r+ for the change in label, and not showing the checkbox for automatically clearing it on completion.

Comment on attachment 590686 [details] Screenshot taken which shows the new position See the other ui-r for details, but essentially we don't want to include this option anymore.

Removing changes about the new downloads groupbox, building and runing mochitests... Will remove the Downloads group-box and related code (preferences in XUL and back-end code). Then I will attach it so it could be code-reviewed again.

Thank you for the review, Alex.

Shows the real purpose of the checkbox in the UI and removes the checkbox about hiding the new download once it had been finished. Also removes code for testing this feature. UI review was done in previous comments about attachment 585703 [details] [diff] [review]. Could you take a look at this one, Gavin? Thank you.

I must say that I am affected by some of the bugs that disallowed me to use normal building process, so I had to patch my local repository with a patch from bug 718541. Only to be sure, please, send patch to try-server in order to test it in a more normal condition.

Javi: I have read through all the comments here to get familiar with the bug and will try to review this patch tonight or tomorrow.

Thank you Jared. I marked the patch which was reviewed positively by the UX-team (Alex) as obsolete to avoid confusion when this bug patch needs to land. Finally, I have been able to run mochitest-browser-chrome on my last Nightly build and got 9 failed tests. Running without the patch applied I got 10 failed, so maybe they are unrelated tests. Anyway, it would be better to try-build it :-)

Comment on attachment 591415 [details] [diff] [review] patch Review of attachment 591415 [details] [diff] [review]: ----------------------------------------------------------------- I have pushed this patch to try-server: https://tbpl.mozilla.org/?tree=Try&rev=5f9eab8fd1f3 Pending a good try run, then this is r=me. The previous test failures you saw before could have been from some flaky tests.