679117 - Add View/Apply Theme to MailNews

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Currently, View/Apply Theme is only available in the browser part. Here are some reasons why I think it should be in MailNews, too:

1. Theming, be it "real" themes or Personas, applies to most components of SeaMonkey, but certainly to both the browser and MailNews.

2. The switching functionality does not logically depend on the browser. Switching the theme from View/Apply Themes simply issues a dialog that offers to restart. The two options for getting new themes/Personas open a web page, but so do Tools/Data Manager and Tools/Add-ons Manager.

3. The browser is not necessarily the main part of the application. Many people start SeaMonkey by opening MailNews first (or only!), or leave it open while closing all browser windows at times. In such a situation, changing the theme can only be done by opening a browser window, be it explicitly or implicitly by opening the Add-ons Manager (from where the switch can be made, too).

Before anyone asks: I won't try to sneak this into other components like the Address Book or Composer, even though theming applies there, too. I don't consider those central enough for justifying the extra menu item. And ChatZilla disqualifies itself by not supporting theming (or using its own).

Opinions welcome.

Comment 1

[Karsten Düsterloh]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #0)
> 3. The browser is not necessarily the main part of the application. Many
> people start SeaMonkey by opening MailNews first (or only!), or leave it
> open while closing all browser windows at times.

Exactly!

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Since the most part, including the new files, is not in MailNews, I'm swapping the reviewers this time.

Comment 3

[[e-mail]]

Comment on attachment 560967 [details] [diff] [review]
patch

>+        <menu id="menu_ViewApplyTheme"/>
Probably should be a lower case v.

>+ *   Jens Hatlak <[e-mail]> (Original Author)
Most of the JS is mine, actually ;-)
[I can't say the same for the rest though]

>+var gBrandBundle;
Might be better to list this as a dependency.

>+  AddonManager.addAddonListener(gAddonListener);
Need to move the import of AddonManager from navigator.js

>+      restartApp();
Need to move this from navigator.js

>+<overlay id="viewZoomOverlay"
Too much copy and paste ;-)

> function OnLoadMessenger()
> {
>+  // see viewApplyThemeOverlay.js
>+  applyThemeStartup();
This should probably be added as an onload event handler instead.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #3)
> >+        <menu id="menu_ViewApplyTheme"/>
> Probably should be a lower case v.

I was thinking about that, but menu_ViewApplyTheme_Popup already existed so either we'll create an inconsistency or possibly break add-ons compatibility. Your choice.

> >+ *   Jens Hatlak <[e-mail]> (Original Author)
> Most of the JS is mine, actually ;-)
> [I can't say the same for the rest though]

Locally changed the Original Author to you for the JS file.

> >+var gBrandBundle;
> Might be better to list this as a dependency.

In which way? As a stringbundle in the corresponding XUL file (probably useless), as a comment, or...?

> >+  AddonManager.addAddonListener(gAddonListener);
> Need to move the import of AddonManager from navigator.js

Right, plus the shutdown part.

> >+      restartApp();
> Need to move this from navigator.js

Done locally.

> >+<overlay id="viewZoomOverlay"
> Too much copy and paste ;-)

Oops.
 
> > function OnLoadMessenger()
> > {
> >+  // see viewApplyThemeOverlay.js
> >+  applyThemeStartup();
> This should probably be added as an onload event handler instead.

Hmm, need to read up tomorrow what exactly you mean by that.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1a

This has all nits addressed except "list this [gBrandBundle] as a dependency" which was a little too vague for me.

Comment 6

[[e-mail]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4)
> Locally changed the Original Author to you for the JS file.
But using the wrong email address :-(

> > >+var gBrandBundle;
> > Might be better to list this as a dependency.
I originally had a number of comments relating to this, and didn't leave the most enlightening one. Sorry about that.

I thought it was confusing that both the overlay and the main windows wanted to access the brand bundle, and they're using the same variable for it. I didn't really want the code duplication, so, as you already need the stringbundle element to exist in the main window's XUL, maybe you could have just added a comment somewhere noting that the code expects the bundle to exist.

> > >+  AddonManager.addAddonListener(gAddonListener);
> > Need to move the import of AddonManager from navigator.js
> Right, plus the shutdown part.
So, I inadvertently helped you find a bug :-)

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1b [Checkin: comment 13]

Comment 8

[[e-mail]]

Comment on attachment 561564 [details] [diff] [review]
patch v1b [Checkin: comment 13]

>+var gBrandBundle; // bundle_brand stringbundle from overlayed XUL file
Actually want I meant was that you would avoid duplicating the variable too, leaving just the reference in the prompt code.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #8)
> Comment on attachment 561564 [details] [diff] [review]
> patch v1b
> 
> >+var gBrandBundle; // bundle_brand stringbundle from overlayed XUL file
> Actually want I meant was that you would avoid duplicating the variable too,
> leaving just the reference in the prompt code.

Sure, why not, locally removed the line (and the corresponding init in applyThemeOnLoad) and added an inline comment before the remaining use of the variable, reading: "gBrandBundle: bundle_brand stringbundle from overlayed XUL file".

Comment 10

[[e-mail]]

(In reply to Jens Hatlak from comment #9)
> Locally removed the line (and the corresponding init in applyThemeOnLoad) and
> added an inline comment before the remaining use of the variable, reading:
> "gBrandBundle: bundle_brand stringbundle from overlayed XUL file".
Great, thanks!

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Karsten, any chance that you can get to this by tomorrow evening the latest? Otherwise it'll miss the Aurora uplift (which would be a pity, given how far this bug/patch is).

Comment 12

[Karsten Düsterloh]

Comment on attachment 561564 [details] [diff] [review]
patch v1b [Checkin: comment 13]

>+# XXX - this sucks and should only be temporary.

Comments like this are and have always been useless and don't deserve copying over.
Just drop it.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 561564 [details] [diff] [review]
patch v1b [Checkin: comment 13]

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

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

FTR: This bug intentionally only added Apply Theme to MailNews (see the end of comment 0), but it also laid the foundations for simple re-use in other suite components. If anyone wants to have it copied to, say, Composer, please file a new bug. Thanks.

Comment 15

[[e-mail]]

Oops, duplicated "A" access key in the View menu :-(