404263 - Migrate Mailnews preference pane to the new Preferences window.

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Mark Banner (:standard8)]

Attachment: WIP v1

As the title says really.

The patch I'm attaching does everything bar the mapi overlays.

I'm waiting to see what's happening with the browser patch on bug 394522 before I do anything with that, but I may end up integrating them onto the one pane so that when we get the shell service we can use them cross-platform.

If anyone wants to take a look/test of this before I update it feel free ;-)

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix

This patch migrates the current mailnews pref pane and incorporates the mapi overlay into it. At the same time it starts to implement some calls to the shell service (bug 380347) which will hopefully provide a basis for what we actually need if its not quite right.

The mapi/shell service interfaces are selected depending on what is available - nothing available and nothing is shown. This should give us an easier time as we switch over to the shell service and implement it on our various platforms.

Unfortunately I don't have a windows dev env at the moment, so apart from checking the show nothing option, I've not been able to test the mapi parts of this. As the shell service isn't implemented either, some of that is speculation, but I'm hoping its reasonably close to what we need/will provide something when we switch.

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix v1a

This time with the associated locale file changes...

Comment 3

[[e-mail]]

Comment on attachment 290048 [details] [diff] [review]
The fix v1a

>+  <script type="application/x-javascript">
>+  <![CDATA[
>+    var panel;
>+    if (panel != undefined) {
>+      switch(panel) 
>+      {
>+      case "chrome://communicator/content/pref/pref-scripts.xul":
>+        _elementIDs.push("javascriptAllowMailNews");
>+        _elementIDs.push("pluginAllowMailNews");
>+        break;
>+      case "chrome://communicator/content/pref/pref-appearance.xul":
>+        _elementIDs.push("generalStartupMail");
>+        _elementIDs.push("generalStartupAddressBook");
>+        break;
>+      }
>+    }
>+  ]]>
>+  </script>         
This is part of the old prefwindow system and has no place here.

>+        <!-- Enable these as we migrate them -->
>+<!--
>+        <treeitem label="&viewingMessages.label;"
>+                  url="chrome://messenger/content/pref-viewing_messages.xul"/>
Mnyromyr likes these all to have ids, see e.g. browser and mousewheel.

>-            <treecell url="chrome://messenger/content/addressbook/pref-addressing.xul" label="&address.label;"/> 
>+            <treecell url="chrome://messenger/content/addressbook/pref-addressing.xul" label="&address.label;"/>
Don't bother, it needs migrating anyway ;-)

> function setHomePageToDefaultPage(folderFieldId)
> {
>   var homePageField = document.getElementById(folderFieldId);
>-  var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>-                              .getService(Components.interfaces.nsIPrefService);
>   var prefs = prefService.getDefaultBranch(null);
>   var url = prefs.getComplexValue("mailnews.start_page.url",
>                                   Components.interfaces.nsIPrefLocalizedString).data;
>   homePageField.value = url;
> }
This is wrong, the <preference> element has a defaultValue property.

>+    document.getElementById("setDefaultMail").disabled =
>+      shellService.isDefaultClient(false,
>+                                   Components.interfaces.nsIShellService.MAIL);
>+
>+    document.getElementById("setDefaultNews").disabled =
>+      shellService.isDefaultClient(false,
>+                                   Components.interfaces.nsIShellService.NEWS);
This doesn't check for locked ui. Even if toolkit doesn't allow you to link a <preference> directly to a button you should still use one to read the locked state.

>+    return;
>+  }
>+  else if ("@mozilla.org/mapiregistry;1" in Components.classes) {
No else necessary after return; maybe leave a blank line though.

>+    var mapiRegistry = Components.classes["@mozilla.org/mapiregistry;1"]
>+                     .getService(Components.interfaces.nsIMapiRegistry);
>+
>+    if (mapiRegistry) {
You don't need to check the same thing twice (OK so maybe the old code did).

>+    document.getElementById("setDefaultMail").disabled = true;
>+    document.getElementById("setDefaultMail").disabled = true;
Unnecessary code duplication.

I couldn't review any more because the pref pane wouldn't load.

Comment 4

[[e-mail]]

Comment on attachment 290048 [details] [diff] [review]
The fix v1a

Whoops, clicked on + instead of -. Good thing I found some more errors ;-)

>-                  prefstring="mailnews.start_page.enabled" oncommand="StartPageCheck();"
What happend to this check?

>+                 preftype="localizedstring" type="autocomplete"
I don't think you need preftype any more.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v2

This is an updated version that I think fixes all the comments.

Note that it has a possible problem whereby the enable/disable of the mailnews start page textbox is inverted to the checkbox. Not sure why, I'm going to grab latest trunk just to make sure it hasn't been fixed...

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Note that it has a possible problem whereby the enable/disable of the mailnews
> start page textbox is inverted to the checkbox. Not sure why, I'm going to grab
> latest trunk just to make sure it hasn't been fixed...

Two thoughts (I can't check until later):

1) Could it be that I need to access the state of the preference mailnews.start_page.enabled rather than its associated checkbox field.
2) Is onchange="StartPageCheck();" set on the preference getting in the way of the one I've added on the checkbox? If it is, then why wasn't I getting 2 dumps on the console for the dump() statement I added, and why did it not seem to work before I added the oncommand for the checkbox.

Comment 7

[[e-mail]]

(In reply to comment #6)
>1) Could it be that I need to access the state of the preference
>mailnews.start_page.enabled rather than its associated checkbox field.
Yes, but you are, so that's OK!

>2) Is onchange="StartPageCheck();" set on the preference getting in the way of
>the one I've added on the checkbox? If it is, then why wasn't I getting 2 dumps
>on the console for the dump() statement I added, and why did it not seem to
>work before I added the oncommand for the checkbox.
The preference would be the correct place if you used a lower case s!

Comment 8

[[e-mail]]

Comment on attachment 291242 [details] [diff] [review]
The fix v2

>+    content/messenger/mailPreferences.xul                                      (base/prefs/resources/content/mailPreferences.xul)
I'm guessing here that you're creating a new overlay because you can't (currently) use the same overlay for both pref windows because they're both using the same id="panelChildren". Assuming that this will fix the problem, I asked Mnyromyr and he said that renaming the new one would be fine.

>-            <treecell url="chrome://messenger/content/addressbook/pref-addressing.xul" label="&address.label;"/> 
>+            <treecell label="(migrated)"/> 
Except it isn't ;-) You've also got a trailing space there.

>+function setHomePageToDefaultPage()
>+{
>+  document.getElementById("mailnews.start_page.url").reset();
>+}
This only works if the pref is set, so it fails badly without instant apply.

>+  var prefs = prefsService.getBranch("system.windows.lock_ui.");
I think you only use prefsService once (here) so no point making it a global. Better still, use separate <preference> elements. Can you use a <preference> to lock a button? If so, that would be the neatest solution.

>+    return;
>   }
>+  else if ("@mozilla.org/mapiregistry;1" in Components.classes) {
You don't need an else here because you return...

>+  document.getElementById("defaultMail").hidden = true;
As Seth pointed out in bug 349251 this doesn't actually work, but we might end up fixing this in button.xml so don't worry too much about it yet.

>+  if ("@mozilla.org/browser/shell-service;1" in Components.classes) {
>+    var shellService = Components.classes["@mozilla.org/browser/shell-service;1"]
>+      .getService(Components.interfaces.nsIShellService);
>+
>+    shellService.setDefaultClient(false, false,
>+                                  Components.interfaces.nsIShellService.MAIL);
Consider adding const nsIShellService = Components.interfaces.nsIShellService; then you can probably afford to line up the .getService with the .classes and the .setDefaultClient will fit on one line.

>+      <preference id="mailnews.start_page.enabled" onchange="StartPageCheck();"
Typo, should be startPageCheck();

>+        <checkbox id="mailnewsStartPageEnabled" oncommand="startPageCheck();"
Don't need to startPageCheck(); here.

>+<!ENTITY defaultMailSettings.caption      "Make &vendorShortName; Mail &amp; Newsgroups the default application for:">
Don't use a : in a groupbox caption.

I notice that the new pane is quite a bit taller than the old one... could you rearrange it at all to take up less space; maybe if you didn't put the set default buttons in their own group?

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 291242 [details] [diff] [review])
> >+  var prefs = prefsService.getBranch("system.windows.lock_ui.");
> I think you only use prefsService once (here) so no point making it a global.
> Better still, use separate <preference> elements. Can you use a <preference> to
> lock a button? If so, that would be the neatest solution.

I couldn't find a way to link them together, so I used the preference element and just got its value instead.

> I notice that the new pane is quite a bit taller than the old one... could you
> rearrange it at all to take up less space; maybe if you didn't put the set
> default buttons in their own group?

I've changed it back and copied the existing mapi way of doing it - so it should look the same apart from the fact we've now got buttons instead of check boxes.

Comment 10

[Mark Banner (:standard8)]

Attachment: The fix v3

Comment 11

[Karsten Düsterloh]

> > Better still, use separate <preference> elements. Can you use a 
> > <preference> to lock a button? If so, that would be the neatest solution.
> 
> I couldn't find a way to link them together, so I used the preference 
> element and just got its value instead.

You mean like this?
<http://mxr.mozilla.org/seamonkey/source/suite/common/pref/pref-navigator.xul#156>

Comment 12

[Mark Banner (:standard8)]

Attachment: The fix v4

Ok, so linking them does work.

Comment 13

[Ian Neal]

Comment on attachment 291948 [details] [diff] [review]
The fix v4

>Index: suite/locales/jar.mn
>===================================================================
>@@ -24,6 +24,7 @@
>   locale/@AB_CD@/communicator/notification.properties                       (%chrome/common/notification.properties)
>   locale/@AB_CD@/communicator/openLocation.dtd                              (%chrome/common/openLocation.dtd)
>   locale/@AB_CD@/communicator/openLocation.properties                       (%chrome/common/openLocation.properties)
>+  locale/@AB_CD@/communicator/passwordManager.dtd                           (%chrome/common/passwordManager.dtd)
Did this sneak in by accident?

Comment 14

[[e-mail]]

(In reply to comment #13)
>(From update of attachment 291948 [details] [diff] [review])
>>+  locale/@AB_CD@/communicator/passwordManager.dtd                           (%chrome/common/passwordManager.dtd)
>Did this sneak in by accident?
Looks suspiciously like he's running with my UI patch for login manager ;-)

Comment 15

[[e-mail]]

Comment on attachment 291948 [details] [diff] [review]
The fix v4

>+      <preference id="mailnews.start_page.enabled" onchange="startPageCheck();"
I believe that this.parentNode.parentNode.startPageCheck(); is slightly superior because it means the JS engine doesn't have to search for the method.

>+          <button id="setDefaultMail" accesskey="&setDefaultMail.accesskey;"
>+		  label="&setDefaultMail.label;" oncommand="onSetDefaultMail();"
>+                  preference="system.windows.lock_ui.defaultMailClient"/>
>+          <button id="setDefaultNews" accesskey="&setDefaultNews.accesskey;"
>+		  label="&setDefaultNews.label;" oncommand="onSetDefaultNews();"
>+                  preference="system.windows.lock_ui.defaultNewsClient"/>
I think some tabs crept in here...

Comment 16

[[e-mail]]

Comment on attachment 291948 [details] [diff] [review]
The fix v4

>+      <preference id="system.windows.lock_ui.defaultMailClient"
>+                  name="system.windows.lock_ui.defaultMailClient" type="bool"/>
>+      <preference id="system.windows.lock_ui.defaultNewsClient"
>+                  name="system.windows.lock_ui.defaultNewsClient" type="bool"/>
These need to be readonly="true" as well. (Conveniently they fit on the first line!) r=me with this, the tabs, and the spurious locale jar entry fixed.

Comment 17

[Mark Banner (:standard8)]

Patch checked in with comments addressed. Fixed.