444582 - Migrate SeaMonkey's Download preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Manuel Reimer]

Comment 1

[Manuel Reimer]

Attachment: First patch

First patch. I also use filefield for the sound file, as I think it's nicer to have a "real" path displayed.

Comment 2

[Stefan [:stefanh]]

cc:ing Callek, since I know he's working on migrating the toolkit download manager.

Comment 3

[Justin Wood (:Callek)]

Stefan, MReimer:

Thanks, go ahead and push this through as I'm unsure if DLMGR will make first alpha at this stage. I'll see about giving this patch a cursory review in a day or so as well (I'm not technically a peer here).

Comment 4

[[e-mail]]

Comment on attachment 328892 [details] [diff] [review]
First patch

>+function readDLFolder() {
>+  var pref = document.getElementById("browser.download.dir");
>+  var field = document.getElementById("downloadFolder");
>+
>+  if (!pref.value) {
Hmm... we don't actually default this directory in the code, I wonder why we bother doing it in preferences.

>+  if (ret == nsIFilePicker.returnOK) {
>+    pref.value = fp.file;
>+  }
>+}
> 
>+function setPrefDLElements() {
Two {} nits here... the first is that you don't need {}s for one line, the second is that for main functions the { should be on its own line.

>   var initialDir = null;
>+  if (pref.value != "") {
>+    var fphandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>+                              .getService(Components.interfaces.nsIFileProtocolHandler);
>+    initialDir = fphandler.getFileFromURLSpec(pref.value)
>+                          .parent.QueryInterface(nsILocalFile);
>+  }
I notice that if you move this code down a bit you could assign directly to fp.displayDirectory

>+  var ret=fp.show();
Nit: space around =

r=me with the nits fixed.

Comment 5

[Ian Neal]

Comment on attachment 328892 [details] [diff] [review]
First patch

>Index: suite/common/pref/pref-download.js
>===================================================================
>+function Startup() {
>   setPrefDLElements();
>   PlaySoundCheck();
> 
>+  // if we don't have the alert service, hide the pref UI for using alerts to
>+  // notify on download completion
>   // see bug #158711
>   var downloadDoneNotificationAlertUI = document.getElementById("finishedNotificationAlert");
>   downloadDoneNotificationAlertUI.hidden = !("@mozilla.org/alerts-service;1" in Components.classes);
> }
> 
>+function readDLFolder() {
>+  var pref = document.getElementById("browser.download.dir");
>+  var field = document.getElementById("downloadFolder");
If you passed "this" into this function as an argument you would already have field.

>+// Should never get called as filefield can't be modified by user
>+function writeDLFolder() {
>+  // Just echo the value already in preference
>+  var pref = document.getElementById("browser.download.dir");
>+  return pref.value;
You won't need this function if you drop the onsynctopreference that calls it.

>+function Browse() {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  const nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var initialDir = null;
>+  if (pref.value != "") {
>+    var fphandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>+                              .getService(Components.interfaces.nsIFileProtocolHandler);
>+    initialDir = fphandler.getFileFromURLSpec(pref.value)
>+                          .parent.QueryInterface(nsILocalFile);
>+  }
You could globally define fphandler as you are using it in multiple locations

> 
>+  var fp = Components.classes["@mozilla.org/filepicker;1"]
>+                     .createInstance(nsIFilePicker);
>+  var prefutilitiesBundle = document.getElementById("bundle_prefutilities");
>+  var title = prefutilitiesBundle.getString("choosesound");
>+  fp.init(window, title, nsIFilePicker.modeOpen);
>+  if (initialDir) fp.displayDirectory = initialDir;
>+  var ftype = prefutilitiesBundle.getString("SoundFiles");
>+  fp.appendFilter(ftype, "*.wav; *.wave");
>+  fp.appendFilters(nsIFilePicker.filterAll);
>+
>+  var ret=fp.show();
>   if (ret == nsIFilePicker.returnOK) {
>+    pref.value = fp.fileURL.spec;
>   }
> }
> 
>+function PreviewSound() {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  var sound = Components.classes["@mozilla.org/sound;1"]
>+                        .createInstance(Components.interfaces.nsISound);
>+  var ioservice = Components.classes["@mozilla.org/network/io-service;1"]
>+                            .getService(Components.interfaces.nsIIOService);
>+  if (pref.value != "")
>+    sound.play(ioservice.newURI(pref.value, null, null));
As ioservice is only used within the if you could just define it within the if

>   else
>+    sound.beep();
>+}
>+
>+function readSndFile() {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  var field = document.getElementById("downloadSndURL");
If you passed "this" into this function as an argument you would already have field.
>+
>+  // Make sure sound_url setting is actually a URL
>+  const nsIURIFixup = Components.interfaces.nsIURIFixup;
>+  try {
>+    var URIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                             .getService(nsIURIFixup);
>+    pref.value = URIFixup.createFixupURI(pref.value,
>+                                         nsIURIFixup.FIXUP_FLAG_NONE).spec;
>+  } catch (ex) { }
Shouldn't really be doing this here but in profile migration, see what I did in bug 441403.

>+
>+  if (pref.value) {
>+    var fphandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>+                              .getService(Components.interfaces.nsIFileProtocolHandler);
>+    var file = fphandler.getFileFromURLSpec(pref.value);
>+    field.file = file;
>+    field.label = (/Mac/.test(navigator.platform)) ? file.leafName : file.path;
>+  }
>+  return undefined;
>+}
>+// Should never get called as filefield can't be modified by user
>+function writeSndFile() {
>+  // Just echo the value already in preference
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  return pref.value;
You won't need this function if you drop the onsynctopreference that calls it.

>Index: suite/common/pref/pref-download.xul
>===================================================================
>+          <filefield id="downloadFolder"
>+                     flex="1"
>+                     preference="browser.download.dir"
>+                     preference-editable="true"
>+                     onsyncfrompreference="return document.getElementById('download_pane').readDLFolder();"
Pass "this" as an argument.

>+                     onsynctopreference="return document.getElementById('download_pane').writeDLFolder();"/>
Drop this attribute

>+        <filefield id="downloadSndURL"
>+                   flex="1"
>+                   preference="browser.download.finished_sound_url"
>+                   preference-editable="true"
>+                   onsyncfrompreference="return document.getElementById('download_pane').readSndFile();"
Pass "this" as an argument.

>+                   onsynctopreference="return document.getElementById('download_pane').writeSndFile();"/>
Drop this attribute.

Comment 6

[Ian Neal]

Oh another comment, functions should start with a capital letter so ReadDLFolder rather than readDLFolder for example.

Comment 7

[Manuel Reimer]

Attachment: Second patch

Comment 8

[Manuel Reimer]

(In reply to comment #4)
> Hmm... we don't actually default this directory in the code, I wonder why we
> bother doing it in preferences.

Good question. I dropped that code. IMHO defaulting to the profile directory is pretty silly. Noone really wants to store download in such a critical location.

Comment 9

[Ian Neal]

Comment on attachment 330778 [details] [diff] [review]
Second patch

>+function SetAutoDLEnabled(aEnable)
Strictly aEnable is the wrong name for the argument, it should be either aDisable or aAuto or something like that.
> {
>+  EnableTextbox("downloadLocation", !aEnable, false);
> }
> 
>+function SetSoundEnabled(aEnable)
> {
>+  EnableTextbox("downloadSndURL", aEnable, false);
>+  document.getElementById("downloadSndPreview").disabled = !aEnable;
Why not get the button to observe like you did for "downloadSndBrowse"?

>+}
>+function ReadDLFolder(aField)
>+{
>+  var pref = document.getElementById("browser.download.dir");
>+  if (pref.value) {
>+    aField.file = pref.value;
>+    aField.label = (/Mac/.test(navigator.platform)) ? pref.value.leafName : pref.value.path;
>   }
> }
> 
>+function prefDownloadSelectFolder()
Nit: capital letter for first letter of function name.
> {
>+  var pref = document.getElementById("browser.download.dir");
>+  const nsIFilePicker = Components.interfaces.nsIFilePicker;
>+  var fp = Components.classes["@mozilla.org/filepicker;1"]
>+                     .createInstance(nsIFilePicker);
>+  var prefutilitiesBundle = document.getElementById("bundle_prefutilities");
>+  var title = prefutilitiesBundle.getString("downloadfolder");
>+  fp.init(window, title, nsIFilePicker.modeGetFolder);
>+  fp.displayDirectory = pref.value;
>+  fp.appendFilters(nsIFilePicker.filterAll);
>+  var ret = fp.show();
>+  if (ret == nsIFilePicker.returnOK)
>+    pref.value = fp.file;
> }
Why are you duplicating code? The old JS shared this common code, and still could with a little tweaking.

>+function Browse()
> {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  const nsIFilePicker = Components.interfaces.nsIFilePicker;
>+
>+  var fp = Components.classes["@mozilla.org/filepicker;1"]
>+                     .createInstance(nsIFilePicker);
>   var prefutilitiesBundle = document.getElementById("bundle_prefutilities");
>+  var title = prefutilitiesBundle.getString("choosesound");
>+  fp.init(window, title, nsIFilePicker.modeOpen);
> 
>+  if (pref.value) {
>+    var fphandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>+                              .getService(Components.interfaces.nsIFileProtocolHandler);
>+    fp.displayDirectory = fphandler.getFileFromURLSpec(pref.value)
>+                          .parent.QueryInterface(nsILocalFile);
>   }
>+  var ftype = prefutilitiesBundle.getString("SoundFiles");
>+  fp.appendFilter(ftype, "*.wav; *.wave");
>+  fp.appendFilters(nsIFilePicker.filterAll);
>+
>+  var ret = fp.show();
>+  if (ret == nsIFilePicker.returnOK)
>+    pref.value = fp.fileURL.spec;
> }
As I said before, you should be migrating the pref value in profile migration to make sure it is a URLspec (nsNetscapeProfileMigratorBase.cpp/h and nsSeamonkeyProfileMigrator.cpp)

>+function PreviewSound()
> {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  var sound = Components.classes["@mozilla.org/sound;1"]
>+                        .createInstance(Components.interfaces.nsISound);
>+
>+  if (pref.value != "") {
>+    var ioservice = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+    sound.play(ioservice.newURI(pref.value, null, null));
>   }
>+  else
>+    sound.beep();
> }
> 
>+function ReadSndFile(aField)
> {
Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound);
>+  var pref = document.getElementById("browser.download.finished_sound_url");
> 
>+  if (pref.value) {
>+    var fphandler = Components.classes["@mozilla.org/network/protocol;1?name=file"]
>+                              .getService(Components.interfaces.nsIFileProtocolHandler);
>+    var file = fphandler.getFileFromURLSpec(pref.value);
>+    aField.file = file;
>+    aField.label = (/Mac/.test(navigator.platform)) ? file.leafName : file.path;
>+  }
As before, you should make fphandler a global and probably document.getElementById("browser.download.finished_sound_url") too

r- for the moment

Comment 10

[Manuel Reimer]

Marking this on as being blocked by the "Popup preferences" patch, now. IMHO it is a *pretty* silly idea to add that much backend patches in a bug that should only contain a patch for the popup preferences...

Comment 11

[Ian Neal]

(In reply to comment #10)
> Marking this on as being blocked by the "Popup preferences" patch, now. IMHO it
> is a *pretty* silly idea to add that much backend patches in a bug that should
> only contain a patch for the popup preferences...
> 
What in that patch is blocking this one? There are some changes in the "Popup preferences" patch that affect what a filefield looks like in the UI but that won't stop this bug from being fixed.
If you are referring to the preference migration that is specific to popup preferences, at the moment, nothing else.

Comment 12

[Ian Neal]

I've spun off creating a more generic backend pref migration transform method into a new bug, so that is now the blocker.

Comment 13

[Ian Neal]

Attachment: Profile migration patch for download preferences v0.1

This is probably the sort of thing you need for migrating a SM 1.x to a 2.x profile now that patch on bug 448105 has landed.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 331649 [details] [diff] [review]
Profile migration patch for download preferences v0.1

if you are dropping browser.download.* full transform you'll need to migrate more prefs than you do here...

(From memory, I'll investigate more later).

Comment 15

[Justin Wood (:Callek)]

MReimer, do you feel we can push this to completion by our planned freeze date of September 9'th. If you don't think you can complete this, please let us know, and/or find someone to finish it for you please.

(see also KaiRo's c#3 in Bug 445014)

Comment 16

[Manuel Reimer]

I'll do the XUL and JS stuff and will just drop the URL verify stuff.

It would be nice if Ian Neal could finish his work with the profile migration stuff.

Comment 17

[Manuel Reimer]

Attachment: Another patch (Checkin: Comment 23)

Comment 18

[Manuel Reimer]

(In reply to comment #9)
> >+  EnableTextbox("downloadSndURL", aEnable, false);
> >+  document.getElementById("downloadSndPreview").disabled = !aEnable;
> Why not get the button to observe like you did for "downloadSndBrowse"?

I don't think we should disable the "play" button to preview the sound. Why should the user not be able to preview the sound just because the pref is locked?

> Why are you duplicating code? The old JS shared this common code, and still
> could with a little tweaking.

The old JS was *heavy* to read and understand. Sometimes it makes things easier if a few lines are duplicated.

> As I said before, you should be migrating the pref value in profile migration
> to make sure it is a URLspec (nsNetscapeProfileMigratorBase.cpp/h and
> nsSeamonkeyProfileMigrator.cpp)

Dropped that code. Would be nice if Ian Neal could finish the migration stuff.

> As before, you should make fphandler a global and probably
> document.getElementById("browser.download.finished_sound_url") too

I made fphandler a global, even if I think that global variables are usually bad programming style and should be only used if *really* required. They usually make code *less* readable...

To make stuff easier to read it would have been a better idea to add

var Cc = Components.classes;
var Ci = Components.interfaces;

directly to preferences.js.

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> var Cc = Components.classes;
> var Ci = Components.interfaces;

We're going with not using the shorthands but the long versions in all SeaMonkey code, from what I understand, so this isn't needed.

Comment 20

[[e-mail]]

Comment on attachment 336540 [details] [diff] [review]
Another patch (Checkin: Comment 23)

>+  window.alert("t1");

>+  window.alert("t2");
Oops ;-)

> }
> 
>+
Nit: don't need this extra blank line.

Comment 21

[Ian Neal]

Comment on attachment 336540 [details] [diff] [review]
Another patch (Checkin: Comment 23)

>+function ReadDLFolder(aField)
>+{
>+  var file = document.getElementById("browser.download.dir").value;
>+  if (file) {
Opening brace needs to be on a new line.
>+    aField.file = file;
>+    aField.label = (/Mac/.test(navigator.platform)) ? file.leafName : file.path;
>   }
> }
> 
>+  fp.init(window, title, nsIFilePicker.modeGetFolder);
>+  fp.displayDirectory = pref.value;
>+  fp.appendFilters(nsIFilePicker.filterAll);
>+  var ret = fp.show();
>+  if (ret == nsIFilePicker.returnOK)
You could do if (fp.show() == nsIFilePicker.returnOK) here and remove var ret line.
>+    pref.value = fp.file;
> }
> 
>+  var ftype = prefutilitiesBundle.getString("SoundFiles");
>+  fp.appendFilter(ftype, "*.wav; *.wave");
>+  fp.appendFilters(nsIFilePicker.filterAll);
> 
>+  var ret = fp.show();
>+  if (ret == nsIFilePicker.returnOK)
You could do if (fp.show() == nsIFilePicker.returnOK) here and remove var ret line.
>+    pref.value = fp.fileURL.spec;
> }
> 
> function PreviewSound()
> {
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  var sound = Components.classes["@mozilla.org/sound;1"]
>+                        .createInstance(Components.interfaces.nsISound);
> 
>+  if (pref.value) {
Opening brace needs to be on a new line.
>+    var ioservice = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+    sound.play(ioservice.newURI(pref.value, null, null));
>+  }
>+function ReadSndFile(aField)
>+{
>+  var pref = document.getElementById("browser.download.finished_sound_url");
>+  if (pref.value) {
Opening brace needs to be on a new line.
>+    var file = gFPHandler.getFileFromURLSpec(pref.value);
>+    aField.file = file;
>+    aField.label = (/Mac/.test(navigator.platform)) ? file.leafName : file.path;
>+  }
>+}

>+          <treeitem id="downloadItem" label="&download.label;"
>+                    prefpane="download_pane"
>+                    helpTopic="navigator_pref_downloads"
>                     url="chrome://communicator/content/pref/pref-download.xul"/>
One attribute per line please.

Comment 22

[Justin Wood (:Callek)]

Actually checkin-needed per https://bugzilla.mozilla.org/describekeywords.cgi#push-needed

Comment 23

[Ian Neal]

Comment on attachment 336540 [details] [diff] [review]
Another patch (Checkin: Comment 23)

http://hg.mozilla.org/comm-central/rev/509bee54949d with brace/alert/fp.show changes made

Comment 24

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1220865201.1220869318.8909.gz#err27
Win2k3 comm-central dep unit test on 2008/09/08 02:13:21
[
Double id='bundle_prefutilities' detected in chrome://communicator/content/pref/preferences.xul:
  Tree 0:
    stringbundle bundle_prefutilities [2]
    prefwindow prefDialog [42]
    #document undefined [0]
  Tree 1:
    stringbundle bundle_prefutilities [1]
    prefpane download_pane [16]
    prefwindow prefDialog [42]
    #document undefined [0]
*** 1842 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/suite/common/tests/chrome/test_idcheck.xul | check id: preferences.xul#bundle_prefutilities
]

"Same" case as bug 444161 had...

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) remove unneeded stringbundle [Checkin: Comment 25]

(In reply to comment #24)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080908013336 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Assuming same r+sr=neil as bug 444161.

http://hg.mozilla.org/comm-central/rev/aa85609d6160

Comment 26

[[e-mail]]

Comment on attachment 336540 [details] [diff] [review]
Another patch (Checkin: Comment 23)

>+    fp.displayDirectory = gFPHandler.getFileFromURLSpec(pref.value)
>+                                    .parent.QueryInterface(nsILocalFile);
Nit: xpconnect can do this QI for you.

Comment 27

[Justin Wood (:Callek)]

Comment on attachment 331649 [details] [diff] [review]
Profile migration patch for download preferences v0.1

List of prefs... 

browser.downloadmanager.behavior
browser.download.progressDnlgDialog.dontAskForLaunch
browser.download.progressDnldDialog.keepAlive
browser.download.progressDnldDialog.enable_launch_reveal_buttons
browser.download.finished_download_sound
browser.download.finished_sound_url
browser.download.finished_download_alert
browser.download.autoDownload
browser.download.dir
browser.download.show_plugins_in_list
browser.download.hide_plugins_without_extensions
browser.download.lastLocation
browser.download.save_converter_index

Unless I am mistaken that is all of them [that we use today], I don't think _all_ are needed to be migrated. but most should

Comment 28

[Ian Neal]

Attachment: Profile migration patch for download preferences v0.1a (Checkin: Comment 30)

Changes since v0.1:
* Adds missing prefs to migrate

Comment 29

[Justin Wood (:Callek)]

Comment on attachment 337506 [details] [diff] [review]
Profile migration patch for download preferences v0.1a (Checkin: Comment 30)

>diff --git a/suite/profile/migration
>+  MAKESAMETYPEPREFTRANSFORM("browser.download.dir",                    String),

Nit: Might as well migrate this to a File, considering it is even read as one outside of migration (complexValue to an nsIFile)


Neil, I'm requesting an additional review from you just for sanity sake, I don't think we need to wait for your eyes on this to commit.

Comment 30

[Ian Neal]

Comment on attachment 337506 [details] [diff] [review]
Profile migration patch for download preferences v0.1a (Checkin: Comment 30)

http://hg.mozilla.org/comm-central/rev/4f4554bf2b19

No a= required as it's a blocking (thanks mcsmurf for reminding me)
No change from String to File as the pref is not a URLSpec