490464 - Update the Download pref pane for use with new DLMGR

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Justin Wood (:Callek)]

Attachment: disable most prefs

With the work in Bug 381157 and Bug 472001 we now need to ensure that the Download prefs that had previously been exposed are updated.

This first patch simply disables most aspects of the pref panel, this was only even created to have a scapegoat for b1 if we need to.

Comment 1

[Justin Wood (:Callek)]

Comment on attachment 374906 [details] [diff] [review]
disable most prefs

>diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
>--- a/suite/common/src/nsSuiteGlue.js
>+++ b/suite/common/src/nsSuiteGlue.js

Ignore this bit

Comment 2

[[e-mail]]

Comment on attachment 374906 [details] [diff] [review]
disable most prefs

Lines that you don't need should either be a) removed if you don't need them for real or b) block commented so that you don't end up editing them in place.

Similarly, disabled attributes should be inserted on new lines thus avoiding editing existing lines.

Comment 3

[Karsten Düsterloh]

What's the point of disabling all these elements?
Will they be activated again by other code? If not so, they should be removed. 
If so, the disabling should probably be part of the code which activates them.

Comment 4

[[e-mail]]

I'd like to see the browser.download.manager.focusWhenStarting preference added to the panel as when the download manager window is open it overrides the browser.download.manager.behavior preference (false always flashes the DM).

Comment 6

[Robert Kaiser]

Attachment: convert .behavior, switch to folder prefs from FF, comment sound

As I see repeating questions on .behavior, we need to get this done for Beta 1 at least far enough that the prefs shown there actually act the way they are supposed to.
This patch switches to the new name for .behavior, switches the location prefs to what Firefox uses (as we know that this uses the new prefs correctly), and comments out the finish/sound block for now - it should come back in whatever way needed in bug 490467.

Comment 7

[[e-mail]]

Comment on attachment 385396 [details] [diff] [review]
convert .behavior, switch to folder prefs from FF, comment sound

>       <preference id="browser.download.finished_download_alert"
>                   name="browser.download.finished_download_alert"
>                   type="bool"/>
Oops. This should have been migrated to showAlertOnComplete :-(

Comment 8

[[e-mail]]

Comment on attachment 385396 [details] [diff] [review]
convert .behavior, switch to folder prefs from FF, comment sound

We don't actually use the folderList preference. We just set it to 2 to use the browser.download.dir folder.

We do still have to remove browser.download.lastLocation though; toolkit doesn't have that pref.

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> (From update of attachment 385396 [details] [diff] [review])
> We don't actually use the folderList preference. We just set it to 2 to use the
> browser.download.dir folder.

I don't understand what you mean. The download manager code uses the preference. All we need to do to support it is to add the code in this patch to correctly manage the preference. Even if our default is 2, IMHO it's a good idea to have this in place so one can e.g. set it to 1 and we'll honor whatever the system's download folder is (and if people select that folder in the panel, we'll activate it and when he changes the default download folder in the system, we'll follow that).
Are you saying that's a bad idea and when people manually change it in about:config (or migrate it over from an FF profile once we support that) the toolkit dlmgr will use it but our pref panel should be effectively broken (as it might display a different folder than what is actually used)?

Comment 10

[[e-mail]]

I don't want us to blindly copy Firefox's backwards-compatibility logic. After all, we don't have to worry about older versions using our profile.

(In reply to comment #9)
> (or migrate it over from an FF profile once we support that)
Actually I think this is the stronger of your arguments, although even then the migrator should be able to handle that.

> (as it might display a different folder than what is actually used)
OK, but even if we display the actual folder, we could simply reset the pref to 2 if the user changes the folder. Or we could expose both prefs ;-)

Comment 11

[Robert Kaiser]

Nominating for blocking beta 1 as shipping with a broken pref panel is awkward.

Comment 12

[[e-mail]]

a=me for an interim pref pane that not necessarily closes this bug!

Comment 13

[Robert Kaiser]

(In reply to comment #10)
> > (as it might display a different folder than what is actually used)
> OK, but even if we display the actual folder, we could simply reset the pref to
> 2 if the user changes the folder. Or we could expose both prefs ;-)

For one thing, with my patch, we do reset the pref to 2 if any other folder than the desktop or downloads folder is chose, we set it to the appropriate value if the OS desktop or download folder is chosen.
For the other, I thought a bit about this, esp. how I would feel from a user POV how that pref should be exposed and I came to the conclusion that the most logical thing would be to have a "set to Downloads" or "set to Desktop" option in the chooser that comes up when clicking the "Choose Folder" button. And then I realized that the filepicker windows I know from modern OSes actually have a list of folder shortcuts usually containing Desktop and Downloads (if the OS has a dedicated Downloads folder, IIRC XP hasn't, Vista and above do). So, actually, the combination of this pref code and the OS filepicker we're using makes this as easy and logical to the user as we can be.
And when the Downloads or Desktop folder is being changed by the OS, we honor that by using the pref in the background that abstracts away the actual path, just as we're setting it back to 2 and the actual path when the user switches to a different folder than those two magic ones. IMHO, this is perfectly transparent and user-friendly in Firefox as well as my patch.

Comment 14

[[e-mail]]

Although Vista has a Downloads folder I had to look it up on the Internet and failed to find any UI for it.

Did you want to set the default to 1 and only change it to 2 if there's a saved folder to migrate?

Comment 15

[Robert Kaiser]

(In reply to comment #14)
> Although Vista has a Downloads folder I had to look it up on the Internet and
> failed to find any UI for it.

Hmm, that's too bad, it would make sense...

> Did you want to set the default to 1 and only change it to 2 if there's a saved
> folder to migrate?

Actually, that might be a good idea, esp. as we have .useDownloadDir set to false by default in any case, so it wouldn't make much of a difference for what we're doing by default, but people would just need to flip the radio to switch to the Downloads folder.

Would you accept the UI of the current patch with that default pref change?

Comment 16

[[e-mail]]

Comment on attachment 385396 [details] [diff] [review]
convert .behavior, switch to folder prefs from FF, comment sound

>-var gFPHandler;
Part of sound support...

>+  /* XXX: sound is to be reintroduced with bug 490467
>   var pDLSound = document.getElementById("browser.download.finished_download_sound");
>   SetSoundEnabled(pDLSound.value);
>- 
>-  // Define globals
>-  gFPHandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>-                         .getService(Components.interfaces.nsIFileProtocolHandler);
Oddly enough, also part of sound support!

>-  EnableElementById("downloadLocation", !aEnable, false);
>+  var downloadFolder = document.getElementById("downloadFolder");
>+  var chooseFolder = document.getElementById("chooseFolder");
>+  var preference = document.getElementById("browser.download.useDownloadDir");
>+  downloadFolder.disabled = !preference.value;
>+  chooseFolder.disabled = !preference.value;
These should use EnableElementById

>+  // don't override the preference's value in UI
>+  return undefined;
Don't do this, just return implicitly.

>+    var file = fp.file.QueryInterface(nsILocalFile);
fp.file already is an nsILocalFile...

>+  var fph = ios.getProtocolHandler("file")
>+                .QueryInterface(Components.interfaces.nsIFileProtocolHandler);
Could reinstate gFPHandler

>+  // Display a 'pretty' label or the path in the UI.
It looks as if this code is back-to-front.
* First, compute the effective folder path (as per Browse... above)
* If it's the same as the desktop, then show the Desktop label and icon
* Otherwise if it's the same as the default download folder, show that label and icon
* Otherwise show the custom path and icon

>+/**
>+  * Returns the Downloads folder.  If aFolder is "Desktop", then the Downloads
>+  * folder returned is the desktop folder; otherwise, it is a folder whose name
>+  * indicates that it is a download folder and whose path is as determined by
>+  * the XPCOM directory service via the download manager's attribute 
>+  * defaultDownloadsDirectory.
>+  *
>+  * @throws if aFolder is not "Desktop" or "Downloads"
>+  */
>+function GetDownloadsFolder(aFolder)
This seems rather silly. Why not just write GetDesktopFolder() and GetDownloadsFolder() ?

>+        // Resolve to a known location if possible. We are writing out
>+        // to prefs on this call, so now would be a good time to do it.
>+        return FolderToIndex(currentDirPref.value);
Except this is completely unnecessary because ChooseFolder() already does this.

> <!ENTITY showAlert.label                "Show an alert">
> <!ENTITY showAlert.accesskey            "n">
Can we change this to "a" now?

Comment 17

[Robert Kaiser]

(In reply to comment #16)
> >+  // don't override the preference's value in UI
> >+  return undefined;
> Don't do this, just return implicitly.

not sure what you mean with that - https://developer.mozilla.org/en/Preferences_System/New_attributes says one should return undefined when leaving default behavior.

> >+  var fph = ios.getProtocolHandler("file")
> >+                .QueryInterface(Components.interfaces.nsIFileProtocolHandler);
> Could reinstate gFPHandler

I don't really understand this, what's the difference in how that one and fph are defined?

> >+  // Display a 'pretty' label or the path in the UI.
> It looks as if this code is back-to-front.
> * First, compute the effective folder path (as per Browse... above)
> * If it's the same as the desktop, then show the Desktop label and icon
> * Otherwise if it's the same as the default download folder, show that label
> and icon
> * Otherwise show the custom path and icon

We could do that, it has one weakness though: in the unusual case that .download.dir isn't set (which technically isn't needed with .download.folderList != 2) it would fail, folderList takes precedence in dlmgr itself as well.

> >+function GetDownloadsFolder(aFolder)
> This seems rather silly. Why not just write GetDesktopFolder() and
> GetDownloadsFolder() ?

Why use two different functions when one works and can potentially be extended in the future?

> >+        // Resolve to a known location if possible. We are writing out
> >+        // to prefs on this call, so now would be a good time to do it.
> >+        return FolderToIndex(currentDirPref.value);
> Except this is completely unnecessary because ChooseFolder() already does this.

If it ever was called, yes.

> > <!ENTITY showAlert.label                "Show an alert">
> > <!ENTITY showAlert.accesskey            "n">
> Can we change this to "a" now?

Sure!

Comment 18

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #16)
> > >+  // don't override the preference's value in UI
> > >+  return undefined;
> > Don't do this, just return implicitly.
> not sure what you mean with that -
> https://developer.mozilla.org/en/Preferences_System/New_attributes says one
> should return undefined when leaving default behavior.
undefined is the "default" return value (either via "return;" or simply falling off the end of the function.)

> > >+  var fph = ios.getProtocolHandler("file")
> > >+                .QueryInterface(Components.interfaces.nsIFileProtocolHandler);
> > Could reinstate gFPHandler
> I don't really understand this, what's the difference in how that one and fph
> are defined?
I was suggesting loading gFPHandler on startup (so that we don't duplicate work when we restore the sound preferences) but you can set it using this code instead (which seems to be preferred across the codebase).

> > >+  // Display a 'pretty' label or the path in the UI.
> > It looks as if this code is back-to-front.
> > * First, compute the effective folder path (as per Browse... above)
> > * If it's the same as the desktop, then show the Desktop label and icon
> > * Otherwise if it's the same as the default download folder, show that label
> > and icon
> > * Otherwise show the custom path and icon
> We could do that, it has one weakness though: in the unusual case that
> .download.dir isn't set (which technically isn't needed with
> .download.folderList != 2) it would fail, folderList takes precedence in dlmgr
> itself as well.
Which bit would fail? In fact, we can save code, since we can save the effective folder path and reuse it for the Browse... button instead of recomputing it. Or at least move the recomputation into a helper method.

> > >+function GetDownloadsFolder(aFolder)
> > This seems rather silly. Why not just write GetDesktopFolder() and
> > GetDownloadsFolder() ?
> Why use two different functions when one works and can potentially be extended
> in the future?
The two functions share no code, and the parameter to switch codepaths is always known in advance. Ironically all the extensibility has to be duplicated into the two callers IndexToFolder and FolderToIndex anyway. And while I'm talking about IndexToFolder, those people who call it with a parameter of zero or one could be fixed to call GetDesktopFolder and GetDownloadsFolder directly.

> > >+        // Resolve to a known location if possible. We are writing out
> > >+        // to prefs on this call, so now would be a good time to do it.
> > >+        return FolderToIndex(currentDirPref.value);
> > Except this is completely unnecessary because ChooseFolder() already does this.
> If it ever was called, yes.
How else would you change the currentDirPref?

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> > > >+  // Display a 'pretty' label or the path in the UI.
> > > It looks as if this code is back-to-front.
> > > * First, compute the effective folder path (as per Browse... above)
> > > * If it's the same as the desktop, then show the Desktop label and icon
> > > * Otherwise if it's the same as the default download folder, show that label
> > > and icon
> > > * Otherwise show the custom path and icon
> > We could do that, it has one weakness though: in the unusual case that
> > .download.dir isn't set (which technically isn't needed with
> > .download.folderList != 2) it would fail, folderList takes precedence in dlmgr
> > itself as well.
> Which bit would fail? In fact, we can save code, since we can save the
> effective folder path and reuse it for the Browse... button instead of
> recomputing it. Or at least move the recomputation into a helper method.

Coming back from "Browse..." and displaying what the prefs are set to in the beginning are two different things, in the former case we can assume the important piece of info is the folder we got returned, in the latter case (which this is about) we need to assume that .folderList take precendence and .dir is only relevant if .folderList is 2, just like in the backend.

> > > >+function GetDownloadsFolder(aFolder)
> > > This seems rather silly. Why not just write GetDesktopFolder() and
> > > GetDownloadsFolder() ?
> > Why use two different functions when one works and can potentially be extended
> > in the future?
> The two functions share no code, and the parameter to switch codepaths is
> always known in advance. Ironically all the extensibility has to be duplicated
> into the two callers IndexToFolder and FolderToIndex anyway. And while I'm
> talking about IndexToFolder, those people who call it with a parameter of zero
> or one could be fixed to call GetDesktopFolder and GetDownloadsFolder directly.

Whatever. For me, it sounded useful to have some abstraction here but I know more understandable code that has more abstracted functions is not what you prefer most of the time.

> > > >+        // Resolve to a known location if possible. We are writing out
> > > >+        // to prefs on this call, so now would be a good time to do it.
> > > >+        return FolderToIndex(currentDirPref.value);
> > > Except this is completely unnecessary because ChooseFolder() already does this.
> > If it ever was called, yes.
> How else would you change the currentDirPref?

Who said you need to change currentDirPref to get here? Isn't opening and closing the panel enough?

Comment 20

[Robert Kaiser]

It looks like this patch isn't a good use of my time, unassigning, let someone else deal with this blocker, I get too easily frustrated on code reviews, it seems.

Comment 21

[[e-mail]]

Attachment: Like this

Comment 22

[[e-mail]]

(In reply to comment #19)
> Coming back from "Browse..."
Actually I meant going in to Browse... to set the displayed folder.

> For me, it sounded useful to have some abstraction here
IndexToFolder is already enough abstraction.

> > How else would you change the currentDirPref?
> Who said you need to change currentDirPref to get here? Isn't opening and
> closing the panel enough?
Why would you want to change prefs just because someone opened a pane?

Comment 23

[[e-mail]]

Comment on attachment 387195 [details] [diff] [review]
Like this

Sorry, but you've previously encountered download manager preferences ;-)

Comment 24

[Frank Wein [:mcsmurf]]

Comment on attachment 387195 [details] [diff] [review]
Like this

Looks good.

Comment 25

[Robert Kaiser]

Comment on attachment 387195 [details] [diff] [review]
Like this

a=me for string landing past the slushy freeze (blocking+ makes up for potential landing after slushy code freeze as well)

Comment 26

[[e-mail]]

Pushed changeset 8b921b3250e6, no wait, 459dcc8b27e2 to comm-central ;-)