441050 - Create UI for new shell service in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Frank Wein [:mcsmurf]]

In Bug 380347 the new shell service was checked in. Now the current default browser UI needs to be replaced/changed so that it uses the new shellservice. Additionally the default mail client dialog needs to be changed, see http://lxr.mozilla.org/seamonkey/source/mailnews/mapi/mapihook/src/, for Thunderbird this was already changed.

Comment 1

[Robert Kaiser]

I think Frank is actually working on this, right?

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: First try at a patch

This patch changes the default browser and mail dialog to the new shell service back-end and also changes the UI a bit. The new UI is not fixed yet, we can also keep the old UI, need to see what to do here. The "Browser" pref panel was not changed yet in this patch.
The new UI checks if SeaMonkey is the default browser and mail client when starting SeaMonkey the normal way. When starting SeaMonkey with the -mail switch, it only checks if SeaMonkey is the default mail application. If it is not, it displays the default client dialog (the dialog looks the same in MailNews and in the browser).
I'll attach a screenshot, which shows the new UI and one case that can occur: SeaMonkey is already the default mail client, but not the default browser yet (that is why "E-Mail" is grayed out: It's already the default). The other cases are SeaMonkey not being the default mail client and SeaMonkey not being the default browser/mail client yet. The "Newsgroup" check box is not checked by default.

Comment 3

[Frank Wein [:mcsmurf]]

Attachment: Screenshot

Comment 4

[Robert Kaiser]

Actually, I think the UI proposal is good. Perhaps there's room for improvement in the future, but I'm happy with having any usable UI in so that we use the new backend and can drop drop winhooks. We can always add improvements over time, in the real open source way of things.

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: New patch

Neil, can you take a look at this patch if this is the way I should be doing it (I used preprocessing mostly instead of if-checks to check the existence of the shell service)? This patch changes all the default browser and mailnews UI, changes the pref panels and disables the old MAPI default client dialog code.

Comment 6

[[e-mail]]

(In reply to comment #5)
> (I used preprocessing mostly instead of if-checks to check the existence of the
> shell service)?
Ah, well I'd prefer if-checks...

Comment 7

[[e-mail]]

Comment on attachment 329198 [details] [diff] [review]
New patch

>+// This function gets the shell service and has it check its setting
> // This will do nothing on platforms other than Windows.
s/other than Windows/without a shell service/

>-  const NS_WINHOOKS_CONTRACTID = "@mozilla.org/winhooks;1";
>-  var dialogShown = false;
>-  if (NS_WINHOOKS_CONTRACTID in Components.classes) {
This is the way you should be checking.

>+      && !shellService.isDefaultClient(true, nsIShellService.BROWSER | nsIShellService.MAIL)) {
Hmm, so if you set SeaMonkey as the default for NEWS only, then it won't warn if somebody else changes it?

Maybe the shell service needs a .shouldBeDefaultClientFor setting. Or maybe .shouldCheckDefaultClient should return the nsIShellService constants.

>+# ***** BEGIN LICENSE BLOCK *****
JS licence please.

>+var gDefaultClientDialog = {
Ridiculous. There are only two functions, after all!


>+    // read the raw pref value and not shellSvc.shouldCheckDefault
>+    // XXX Why?
Because shellSvc.shouldCheckDefault only checks once per startup.

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+    document.getElementById('checkOnStartup').checked = prefs.getBoolPref("shell.checkDefaultClient");
This dialog only appears if the check was made... which would suggest that we can always assume this checkbox is checked, can't we?

>+    if (document.getElementById('checkBrowser').checked && !shellSvc.isDefaultClient(false, nsIShellService.BROWSER))
Or possibly check for .disabled

>+      appTypes |= nsIShellService.BROWSER;
>+    if (document.getElementById('checkMail').checked && !shellSvc.isDefaultClient(false, nsIShellService.MAIL))
>+      appTypes |= nsIShellService.MAIL;
>+    if (document.getElementById('checkNews').checked && !shellSvc.isDefaultClient(false, nsIShellService.NEWS))
>+      appTypes |= nsIShellService.NEWS;
Perhaps you can give the listitems value namess, and use a loop
for ([each listitem])
  if (listitem.checked && !listitem.disabled)
    appTypes |= nsIShellService[listitem.value];

>+    // for testing
??

>+# ***** BEGIN LICENSE BLOCK *****
XML licence please.

>+#ifdef HAVE_SHELL_SERVICE
>       <!-- default browser settings are shown only if supported by the platform -->
>-      <groupbox id="defaultBrowserGroup" flex="1000" align="center" hidden="true">
>+      <groupbox id="defaultBrowserGroup" flex="1000" align="center">
Don't change this file.

>+  var rv = shellSvc.setDefaultClient(false, false, nsIShellService.BROWSER);
> }
unused variable

> function InitPlatformIntegration()
> {
>-  // In future, we will ask the Shell Service about platform integration here,
This is a hint!

>+    document.getElementById("defaultBrowserButton").disabled = true;
>+    document.getElementById("defaultBrowserButton").disabled = true;
Could possibly avoid this duplication.

>Index: mailnews/jar.mn
Will continue to review from here later.

Comment 8

[[e-mail]]

Comment on attachment 329198 [details] [diff] [review]
New patch

>+  if (shellService && defaultAccount &&
>+      shellService.shouldCheckDefaultClient &&
>+      !shellService.isDefaultClient(true, nsIShellService.MAIL))
Given that shouldCheckDefaultClient has side-effects, I think we should check that last (but my question about noticing that we used to be the default client for news still stands.)

>-        // migrate quoting preferences from global to per account. This function returns
>-        // true if it had to migrate, which we will use to mean this is a just migrated
>-        // or new profile
>+        // migrate quoting preferences from global to per account. This function
>+        // returns true if it had to migrate, which we will use to mean this is
>+        // a just migrated or new profile
>         var newProfile = migrateGlobalQuotingPrefs(am.allIdentities);
>-
???

> function defaultClientSetup()
> {
>-  if ("@mozilla.org/browser/shell-service;1" in Components.classes) {
Wait, this code is already shell-service ready! Why are you removing it?

>+#if !(defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE))
Um... who does that leave?

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

Changes in this patch:
- Fixed the review comments, see below for comments on this, the minor ones were fixed without a comment
- Fixed a small logic bug in the Browser pref pane (displayed the wrong string when SeaMonkey was already the default)
- Deleted the mapi registry source files which were only used by SeaMonkey

(In reply to comment #7)
> >+      && !shellService.isDefaultClient(true, nsIShellService.BROWSER | nsIShellService.MAIL)) {
> Hmm, so if you set SeaMonkey as the default for NEWS only, then it won't warn
> if somebody else changes it?

Yes, this is the way it currently is, so I kept that behavior. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mapi/mapihook/src/nsMapiRegistry.cpp&rev=1.19#179 for the code that decides if the dialog box will come up.

> Maybe the shell service needs a .shouldBeDefaultClientFor setting. Or maybe
> .shouldCheckDefaultClient should return the nsIShellService constants.

Left this as is for now. But I can change this if you want this function to be more flexible.

> >+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+                          .getService(Components.interfaces.nsIPrefBranch);
> >+    document.getElementById('checkOnStartup').checked = prefs.getBoolPref("shell.checkDefaultClient");
> This dialog only appears if the check was made... which would suggest that we
> can always assume this checkbox is checked, can't we?

Yeah, this was a leftover from the Thunderbird code. Thunderbird also displays the default mail client dialog when clicking on the "Check Now" button in the preferences window. Fixed this.

> Perhaps you can give the listitems value namess, and use a loop
> for ([each listitem])
>   if (listitem.checked && !listitem.disabled)
>     appTypes |= nsIShellService[listitem.value];

Done. Used value="..." for fixing this.

> >+  if (shellService && defaultAccount &&
> >+      shellService.shouldCheckDefaultClient &&
> >+      !shellService.isDefaultClient(true, nsIShellService.MAIL))
> Given that shouldCheckDefaultClient has side-effects, I think we should check
> that last (but my question about noticing that we used to be the default client
> for news still stands.)

Moved this to the end.

> >-        // migrate quoting preferences from global to per account. This function returns
[...]

Wanted to fix the 80 chars limit while I'm at it ;-). Restored this.

> > function defaultClientSetup()
> > {
> >-  if ("@mozilla.org/browser/shell-service;1" in Components.classes) {
> Wait, this code is already shell-service ready! Why are you removing it?

Because I wanted to #ifdef the code ;-). Removed all ifdefs and replaced those with proper checking via ... in Components.classes everywhere.

> >+#if !(defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE))
> Um... who does that leave?

Nobody. I removed the relevant parts in the code and deleted nsMapiRegistry.h/cpp and nsMapiRegistryUtils.h/cpp, which are only used by SeaMonkey.

Comment 10

[Frank Wein [:mcsmurf]]

Ok, forgot to fix three things: 1. Changes to mailnews/jar.mn, ignore those. 2. Changes to suite/common/jar.mn, ignore the * for preprocessing. I already fixed those two locally.
3. Respect the system.windows.lock_ui.defaultMailClient and system.windows.lock_ui.defaultNewsclient and other locking prefs. Not yet fixed.

Comment 11

[[e-mail]]

Comment on attachment 330457 [details] [diff] [review]
Patch 2

From the feedback I've got so far, I think it would be better if we could track what to check for default, so I'll file a new bug blocking this one.

>+ifneq (,$(filter windows, $(MOZ_WIDGET_TOOLKIT)))
>+DEFINES += -DHAVE_SHELL_SERVICE=1
>+endif
Obsolete, right?

>+  const nsIShellService = Components.interfaces.nsIShellService;
>+  var shellService;
>+  var defaultAccount;
>+  try {
>+    shellService = Components.classes["@mozilla.org/suite/shell-service;1"]
>+                             .getService(nsIShellService);
>+    defaultAccount = accountManager.defaultAccount;
>+  } catch (ex) {}
You forgot to define accountManager. Oops.

>+  // show the default client dialog only if we have at least one account,
>+  // if we should check for the default client, and we aren't already the
>+  // default for mail
>+  if (shellService && defaultAccount &&
>+      !shellService.isDefaultClient(true, nsIShellService.MAIL) &&
>+      shellService.shouldCheckDefaultClient)
>+    window.openDialog("chrome://communicator/content/defaultClientDialog.xul",
>+                      "DefaultClient", "modal,centerscreen,chrome,resizable=no");
I'd prefer this code to be inside the try/catch, so you can move the var declarations inside too ;-)

Comment 12

[[e-mail]]

Comment on attachment 330457 [details] [diff] [review]
Patch 2

>+ * Contributor(s):
>+ *   Mark Banner <[e-mail]>
You added yourself as a contributor to the .xul file, but not the .js file ;-)

>+  return;
Unnecessary.
> }

Comment 13

[Frank Wein [:mcsmurf]]

Comment on attachment 330457 [details] [diff] [review]
Patch 2

Canceling review to fix Bug 447637 and other issues with this patch first.

Comment 14

[Frank Wein [:mcsmurf]]

Attachment: Patch 3

Changes in this patch:
- Now uses the pref introduced by Bug 447637; this pref will control what app types SeaMonkey will check for if it is the default and this pref will also be used for the default selection in the default client dialog
- Adds a checkbox to the Advanced pref panel to enable the dialog again in case the user disabled it
- When the checkbox to "check on startup" is checked in the default client dialog, we store the current checked apps as what apps to check for in shell.checkDefaultApps

Regarding review comments:
- accountManager is now defined and default client dialog is working in MailNews; but I had to move the shouldCheckDefaultClient check before the isDefaultClient check again as calling isDefaultClient changes what shouldCheckDefaultClient returns

to do:
- Fix pref locking, will be done when this patch is reviewed ;-)

Comment 15

[[e-mail]]

(In reply to comment #14)
> - Adds a checkbox to the Advanced pref panel to enable the dialog again in case
> the user disabled it
Yes, but if you don't set anything as default, you'll never get prompted again, even if you reenable the dialog, so I think we need to make it that when you set something as default it should be added to the pref of types to check, so that when you enable the dialog it will work.

Comment 16

[[e-mail]]

And I'm not sure whether we should check for the defaults for the window that you're opening, or for all apps, whichever window you open.

Comment 17

[[e-mail]]

Comment on attachment 334951 [details] [diff] [review]
Patch 3

>+  try {
What throws here? My suspicion is that accountManager.defaultAccount throws if you don't have an account in which case you don't need to test for it.

>+    if (shellService && defaultAccount &&
>+        shellService.shouldCheckDefaultClient &&
>+        !shellService.isDefaultClient(true, nsIShellService.MAIL))
Need to check for shellService.shouldBeDefaultClientFor & (MAIL | NEWS)

>     return;
>   }
>-  if ("@mozilla.org/mapiregistry;1" in Components.classes) {
>-    var mapiRegistry = Components.classes["@mozilla.org/mapiregistry;1"]
>-                     .getService(Components.interfaces.nsIMapiRegistry);
>-
>-    document.getElementById("setDefaultMail").disabled =
>-      mapiRegistry.isDefaultMailClient;
>-
>-    document.getElementById("setDefaultNews").disabled =
>-      mapiRegistry.isDefaultNewsClient;
>-
>-    return;
>-  }
> 
>   document.getElementById("defaultMailPrefs").hidden = true;
If we hide the element by default, then we can show it inside the shell service code, and lose the explicit return.

> function onSetDefaultMail()
> {
>+  if ("@mozilla.org/suite/shell-service;1" in Components.classes) {
Don't need to check this as the button will be hidden if there's no service.

> // The list of components we register
> static const nsModuleComponentInfo components[] = 
> {
>-#ifndef MOZ_THUNDERBIRD
>   {
>-    NS_IMAPIREGISTRY_CLASSNAME, 
>-    NS_IMAPIREGISTRY_CID,
>-    NS_IMAPIREGISTRY_CONTRACTID, 
>-    nsMapiRegistryConstructor
>-  },
>-#endif
>-
>-{
>     NS_IMAPISUPPORT_CLASSNAME,
>     NS_IMAPISUPPORT_CID,
>     NS_IMAPISUPPORT_CONTRACTID,
>     nsMapiSupportConstructor,
>     nsMapiRegistrationProc,
>     nsnull
>-}
>+  }
> };
Is this module part of static mail? I don't see any mailnews/build changes.

>+    var defaultAccount;
Not used here?

>+    if (shellService && shellService.shouldCheckDefaultClient 
Don't need to check for shellService here. Also, there's a trailing space.

>+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(nsIPrefBranch);
>+  var defaultList = document.getElementById("defaultList");
>+  var appTypes = pref.getIntPref("shell.checkDefaultApps");
Use the shell service ;-)

>+  <listbox rows="3" id="defaultList" seltype="single">   
More trailing spaces, also isn't seltype="single" the default for a listbox?

>+    <listitem id="checkBrowser" value="BROWSER" type="checkbox" label="&browser.label;"/>
>+    <listitem id="checkMail" value="MAIL" type="checkbox" label="&email.label;"/>
>+    <listitem id="checkNews" value="NEWS" type="checkbox" label="&newsgroups.label;"/>
I don't think you use these ids any more :-)

>+    if(shellSvc.isDefaultClient(false, nsIShellService.BROWSER))
Nit: space after (

So, you open editor, look at preferences, and suddenly we stop checking to see we're the default? I think we need a separate patch to fix the shell service!

Reluctantly setting - on this although we're really close now.

Comment 18

[Frank Wein [:mcsmurf]]

Attachment: Patch 4

Ok, changes in this patch compared to the patch before:
- Fixed small logic error in the default client dialog (it's possible that we are the default client for an app type but we do not want to check if we are the default for that type)
- MailNews opens the default client dialog on app startup only when we want to check if mail is the default app and it is not and/or if news is the default app and it is not
- Clicking on the Set Default Browser/Mail/News button in prefs now adds that app type to the list of applications we want to check for if we are the default
- Fixed the review comments:

> Is this module part of static mail? I don't see any mailnews/build changes.
MAPI has its own DLL.

> More trailing spaces, also isn't seltype="single" the default for a listbox?
Yes, is the default, removed it.

> So, you open editor, look at preferences, and suddenly we stop checking to see
> we're the default? I think we need a separate patch to fix the shell service!
No, the preferences window uses a different first parameter for the isDefaultClient check so that this will not happen.

Comment 19

[[e-mail]]

Comment on attachment 335998 [details] [diff] [review]
Patch 4

>+        (shellService.shouldBeDefaultClientFor & nsIShellService.MAIL &&
>+        !(shellService.isDefaultClient(true, nsIShellService.MAIL))) |
>+        (shellService.shouldBeDefaultClientFor & nsIShellService.NEWS &&
>+        !(shellService.isDefaultClient(true, nsIShellService.NEWS))))
I think you could make this simpler - if you get the app types into a variable, and mask the mail and news bits, and at least one of those bits was set, then those are the bits you want to pass to isDefaultClient.

>+  var appTypes = shellService.shouldBeDefaultClientFor;
...
>+  if (appTypes != (appTypes | nsIShellService.MAIL))
>+    shellService.shouldBeDefaultClientFor = appTypes | nsIShellService.MAIL;
What's wrong with
shellService.shouldBeDefaultClientFor |= nsIShellService.MAIL;
(the same applies for NEWS, BROWSER and the default client dialog).
[I seem to remember wanting some follow-up fixes in a separate bug, I think I might want to add moving this code into .setDefaultClient to the list.]

>+    var appTypes = pref.getIntPref("shell.checkDefaultApps");
Why are you using the pref instead of the shell service?

>+    if (shellService.shouldCheckDefaultClient &&
>+        !shellService.isDefaultClient(true, appTypes)) {
Should we check to see whether appTypes is zero first?

>+    if (nsIShellService[currentItem.value] & appTypes || isDefault) {
>+      currentItem.checked = true;
>+      
>+      if (isDefault)
>+        currentItem.disabled = true;
I'm not wholly convinced this is right, see below.

>+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(nsIPrefBranch);
Still using pref when you don't need to.

>+  var appTypes = 0;
>+  var appTypesCheck = 0;
I'm not sure you need both of these. If you build up a value of types that the user wants to set as default, then setting those as default should then add them to the list of types to check, although obviously we have to do it manually for now (see far above).

>+  if (document.getElementById('checkOnStartup').checked &
>+      pref.getIntPref("shell.checkDefaultApps") != appTypesCheck)
>+    // Update the pref for which app types we should check if we are the default app
>+    pref.setIntPref("shell.checkDefaultApps", appTypesCheck);
I think we should always update the "pref", and rely on shouldCheckDefaultClient telling us whether or not to check.

Comment 20

[Frank Wein [:mcsmurf]]

Attachment: Patch 5

Ok, here we go:
- Addressed all review comments

> I think you could make this simpler - if you get the app types into a 
> variable, and mask the mail and news bits, and at least one of those bits was 
> set, then those are the bits you want to pass to isDefaultClient.

Good idea, done.

> What's wrong with
> shellService.shouldBeDefaultClientFor |= nsIShellService.MAIL;

Nothing, I did not know about |= before...though, isn't XPCOM access usually slower (but now this code looks better indeed)? ;-)

> Why are you using the pref instead of the shell service?

Sloppy coding, fixed.

> Should we check to see whether appTypes is zero first?

Why not, fixed.

> I think we should always update the "pref", and rely on
> shouldCheckDefaultClient telling us whether or not to check.

Probably makes sense as the user can enable this check again in the prefs.

Comment 21

[Frank Wein [:mcsmurf]]

Comment on attachment 336516 [details] [diff] [review]
Patch 5

>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
>--- a/suite/browser/navigator.js
>+++ b/suite/browser/navigator.js
>@@ -2049,24 +2049,28 @@ function URLBarClickHandler(aEvent)
[...]
>+  const NS_SHELLSERVICE_CID = "@mozilla.org/suite/shell-service;1";
>+  
>+  if (NS_SHELLSERVICE_CID in Components.classes) {
>+    const nsIShellService = Components.interfaces.nsIShellService;
>+    var appTypes = shellService.shouldBeDefaultClientFor;
>+    var shellService = Components.classes["@mozilla.org/suite/shell-service;1"]
>+                                 .getService(nsIShellService);

The var appTypes and var shellService line need to be exchanged of course. Fixed this locally.

Comment 22

[[e-mail]]

Comment on attachment 336516 [details] [diff] [review]
Patch 5

>+    if (shellService.shouldCheckDefaultClient && appTypesCheck &&
>+        !shellService.isDefaultClient(true, appTypesCheck))
Nit: appTypesCheck check (!) should be first, as per navigator.

>+  var appTypes = shellService.shouldBeDefaultClientFor;
...
>+  if (appTypes != (appTypes | nsIShellService.MAIL))
>+    shellService.shouldBeDefaultClientFor |= nsIShellService.MAIL;
I don't think you need to check the existing appTypes (all three versions).

>+    if (currentItem.checked || currentItem.disabled) {
>+      // Add selected app types and app types where we are already the defautlt to the list
>+      appTypesCheck |= currentAppType;
>+      
>+      if (currentItem.checked)
>+        appTypes |= currentAppType;
I don't think this is right. appTypesCheck should be those items that are checked. appTypes should be those items that are checked and not disabled.
[Edit: it looks as if attachment 334951 [details] [diff] [review] got this right.]

>+  if (shellSvc.shouldBeDefaultClientFor != appTypesCheck)
I don't think we need to check the existing value. Just overwrite it.

r=me with these and comment 21 fixed.

Comment 23

[Frank Wein [:mcsmurf]]

Attachment: Patch that was checked in

Comment 24

[Frank Wein [:mcsmurf]]

Patch checked in, changeset 26e570823110.
Thanks for all the reviews! :)

Comment 25

[[e-mail]]

Comment on attachment 336748 [details] [diff] [review]
Patch that was checked in

>+  var appTypes = shellService.shouldBeDefaultClientFor;
Unused.

>+  var appTypes = shellService.shouldBeDefaultClientFor;
Unused.

>+  var appTypes = shellSvc.shouldBeDefaultClientFor; 
> 
>+  shellSvc.setDefaultClient(false, false, nsIShellService.BROWSER);
>+
>+  if (appTypes != (appTypes | nsIShellService.BROWSER))
>+    shellSvc.shouldBeDefaultClientFor |= nsIShellService.BROWSER;
This was the third version that I was talking about.
(Maybe the change in the default client dialog confused you?)

Comment 26

[Frank Wein [:mcsmurf]]

Fixed that.

Comment 27

[[e-mail]]

Ugh, this made nsIMapiRegistry.idl obsolete, but it never got removed, and it's still cluttering up the tree :-(