Last Comment Bug 708595 - Clarify download history related preferences in the Privacy pane
: Clarify download history related preferences in the Privacy pane
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Javi Rueda
:
Mentors:
Depends on: 564900
Blocks: 432724 703932
  Show dependency treegraph
 
Reported: 2011-12-08 05:01 PST by :Paolo Amadini
Modified: 2012-02-14 02:30 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch WIP (5.00 KB, patch)
2011-12-19 11:52 PST, Javi Rueda
no flags Details | Diff | Splinter Review
patch 2 WIP (6.06 KB, patch)
2011-12-21 11:16 PST, Javi Rueda
no flags Details | Diff | Splinter Review
patch (13.26 KB, patch)
2012-01-04 04:09 PST, Javi Rueda
limi: ui‑review+
Details | Diff | Splinter Review
Screenshot taken which shows the new position (92.54 KB, image/png)
2012-01-23 06:05 PST, Javi Rueda
limi: ui‑review-
Details
patch (14.10 KB, patch)
2012-01-25 04:23 PST, Javi Rueda
jaws: review+
Details | Diff | Splinter Review

Description :Paolo Amadini 2011-12-08 05:01:46 PST
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.
Comment 1 Javi Rueda 2011-12-08 06:57:00 PST
(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.
Comment 2 Javi Rueda 2011-12-08 07:08:15 PST
Uhm... Better to leave ids unmodified (the same with tests source code) because these could be used for some extensions?
Comment 3 :Paolo Amadini 2011-12-08 09:33:44 PST
(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.
Comment 4 Javi Rueda 2011-12-08 09:50:48 PST
(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 :-)
Comment 5 :Paolo Amadini 2011-12-08 10:29:03 PST
(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.
Comment 6 Javi Rueda 2011-12-11 11:57:38 PST
(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"?
Comment 7 :Paolo Amadini 2011-12-12 10:11:38 PST
(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).
Comment 8 Javi Rueda 2011-12-14 11:32:58 PST
I've finished with the UI based on comment 0. Waiting for Alex to say whatever about what would be the most correct direction.
Comment 9 Javi Rueda 2011-12-19 11:52:04 PST
Created attachment 582908 [details] [diff] [review]
patch WIP

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.
Comment 10 Javi Rueda 2011-12-21 11:16:24 PST
Created attachment 583560 [details] [diff] [review]
patch 2 WIP

This time it is taken into account the inverted sense of the check-box.
Comment 11 :Paolo Amadini 2012-01-02 08:08:59 PST
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.
Comment 12 Javi Rueda 2012-01-04 04:09:21 PST
Created attachment 585703 [details] [diff] [review]
patch

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.
Comment 13 Javi Rueda 2012-01-22 12:25:27 PST
ping.
Comment 14 :Paolo Amadini 2012-01-23 03:14:01 PST
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
Comment 15 Javi Rueda 2012-01-23 06:05:20 PST
Created attachment 590686 [details]
Screenshot taken which shows the new position

This screenshot was taken in order to show faster for the reviewer the result of apply the patch from attachment 585703 [details] [diff] [review].
Comment 16 Javi Rueda 2012-01-23 06:17:02 PST
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 17 Alex Limi (:limi) — Firefox UX Team 2012-01-24 15:33:51 PST
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 18 Alex Limi (:limi) — Firefox UX Team 2012-01-24 15:34:57 PST
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.
Comment 19 Javi Rueda 2012-01-24 16:39:01 PST
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.
Comment 20 Javi Rueda 2012-01-25 04:16:27 PST
Thank you for the review, Alex.
Comment 21 Javi Rueda 2012-01-25 04:23:47 PST
Created attachment 591415 [details] [diff] [review]
patch

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.
Comment 22 Javi Rueda 2012-01-25 04:39:08 PST
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.
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-02-08 01:13:47 PST
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.
Comment 24 Javi Rueda 2012-02-08 18:01:43 PST
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 25 Jared Wein [:jaws] (please needinfo? me) 2012-02-09 08:32:02 PST
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.
Comment 27 Marco Bonardo [::mak] 2012-02-14 02:30:23 PST
https://hg.mozilla.org/mozilla-central/rev/1609860cca3f

Note You need to log in before you can comment on or make changes to this bug.