Closed Bug 993971 Opened 10 years ago Closed 10 years ago

Preview lightweight themes from the Apply Theme menu

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.29

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

Attachments

(1 file, 3 obsolete files)

There are at least two SeaMonkey compatible addons that do this. This may or may not indicate some sort of user demand.

Lightweight Themes Preview for SeaMonkey 10 users
https://addons.mozilla.org/en-US/seamonkey/addon/lightweight-themes-preview/

Themes Menu 27,597 users
https://addons.mozilla.org/en-US/seamonkey/addon/themes-menu/
Attachment #8403884 - Flags: review?(iann_bugzilla)
Attachment #8403884 - Flags: ui-review?(neil)
Comment on attachment 8403884 [details] [diff] [review]
Patch v1.0 Preview Lightweight Themes (a.k.a. Persona)

>+XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
>+  "resource://gre/modules/LightweightThemeManager.jsm");
LightweightThemeWebInstaller could use this.

>-    menuitem.setAttribute("label", theme.name);
>+    menuitem.setAttribute("label", theme.name + " " + theme.version);
Doesn't this have to be localised? Why is the version important anyway?

>+    if (theme.id.match(/personas.mozilla.org$/)) {
if (theme.id.endsWith("@personas.mozilla.org")) {
Or at the very least use test.

>+      var id = theme.id.replace(/\@personas.mozilla.org$/, "");
var id = theme.id.slice(0, -"@personas.mozilla.org".length);
(Unfortunately RegExp.leftContext is deprecated.)

>+      var persona = LightweightThemeManager.getUsedTheme(id);
Is this cheap or should we be doing it in the DOMMenuItemActive handler?
Trying to preview a background when one isn't very active is a little disconcerting as the various styles move the parent menu around... can we limit this to apply only when a background is already active?
Comment on attachment 8403884 [details] [diff] [review]
Patch v1.0 Preview Lightweight Themes (a.k.a. Persona)

Working on a new patch.

(In reply to neil@parkwaycc.co.uk from comment #3)
> Trying to preview a background when one isn't very active is a little
> disconcerting as the various styles move the parent menu around... can we
> limit this to apply only when a background is already active?

In Firefox this doesn't happen so I think that all this moving around is a bug in our lightweight-theme CSS.
Attachment #8403884 - Flags: ui-review?(neil)
Attachment #8403884 - Flags: review?(iann_bugzilla)
Attached patch Patch v1.1 address nits. (obsolete) — Splinter Review
>>+XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
>>+  "resource://gre/modules/LightweightThemeManager.jsm");
>LightweightThemeWebInstaller could use this.
Fixed.

>>-    menuitem.setAttribute("label", theme.name);
>>+    menuitem.setAttribute("label", theme.name + " " + theme.version);
> Doesn't this have to be localised? Why is the version important anyway?
I got carried away (one of the extensions did this). Removed.

>>+    if (theme.id.match(/personas.mozilla.org$/)) {
> if (theme.id.endsWith("@personas.mozilla.org")) {
> Or at the very least use test.
Fixed.

>>+      var id = theme.id.replace(/\@personas.mozilla.org$/, "");
> var id = theme.id.slice(0, -"@personas.mozilla.org".length);
> (Unfortunately RegExp.leftContext is deprecated.)
Fixed.

>>+      var persona = LightweightThemeManager.getUsedTheme(id);
> Is this cheap or should we be doing it in the DOMMenuItemActive handler?
It's very sad. See Bug 714841 for the gory details.

I'm now caching the usedThemes list at the top of checkTheme() and using that instead.

> Trying to preview a background when one isn't very active is a little 
> disconcerting as the various styles move the parent menu around... can we limit 
> this to apply only when a background is already active?
On my TO DO list. Do you know how I can do this?
Attachment #8403884 - Attachment is obsolete: true
Attachment #8407086 - Flags: review?(neil)
Comment on attachment 8407086 [details] [diff] [review]
Patch v1.1 address nits.

Looking at it again the version was ugly anyway because it appeared for the provided themes too.

>+XPCOMUtils.defineLazyModuleGetter(this, "lwtManager",
>+  "resource://gre/modules/LightweightThemeManager.jsm",
>+  "LightweightThemeManager");
What was wrong with calling it LightweightThemeManager?

>+  var usedThemes = lwtManager.usedThemes.map( x => [x.id, x] );
>+  usedThemes = new Map(usedThemes);
...
>+    if (theme.id.endsWith(ID_SUFFIX)) {
>+      var id = theme.id.slice(0, -ID_SUFFIX.length);
>+      var persona = usedThemes.get(id);
>+      menuitem.persona = persona;
Nice improvement, but you can do better by suffixing the used ids before you add them to the map, then you don't even have to check the addon suffix.
Attached patch Patch v1.2 fix review issues. (obsolete) — Splinter Review
> Looking at it again the version was ugly anyway because it appeared for the
> provided themes too.
Removed theme version.

>>+XPCOMUtils.defineLazyModuleGetter(this, "lwtManager",
>>+  "resource://gre/modules/LightweightThemeManager.jsm",
>>+  "LightweightThemeManager");
> What was wrong with calling it LightweightThemeManager?
Nothing much, just reduces the need to wrap at 80 cols. Reverted to "LightweightThemeManager".

>>+  var usedThemes = lwtManager.usedThemes.map( x => [x.id, x] );
>>+  usedThemes = new Map(usedThemes);
> ...
>>+    if (theme.id.endsWith(ID_SUFFIX)) {
>>+      var id = theme.id.slice(0, -ID_SUFFIX.length);
>>+      var persona = usedThemes.get(id);
>>+      menuitem.persona = persona;

> Nice improvement, but you can do better by suffixing the used ids before you
> add them to the map, then you don't even have to check the addon suffix.
Fixed.

> Trying to preview a background when one isn't very active is a little 
> disconcerting as the various styles move the parent menu around... can
> we limit this to apply only when a background is already active?
Yes we can. Done.
Attachment #8407086 - Attachment is obsolete: true
Attachment #8407086 - Flags: review?(neil)
Attachment #8413402 - Flags: review?(neil)
Comment on attachment 8413402 [details] [diff] [review]
Patch v1.2 fix review issues.

>   get _manager () {
>-    var temp = {};
>-    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
>     delete this._manager;
>-    return this._manager = temp.LightweightThemeManager;
>+    return this._manager = LightweightThemeManager;
[Not what I was thinking of, but I guess I get to do a followup...]

>+  var usedThemes = LightweightThemeManager.usedThemes
>+                                          .map( x => [x.id + ID_SUFFIX, x] );
>+  usedThemes = new Map(usedThemes);
Could move the .map to this line which has more spare space.

>+    if (theme.id.endsWith(ID_SUFFIX)) {
>+      var persona = usedThemes.get(theme.id);
Actually my idea was to do
var persona = usedThemes.get(theme.id);
if (persona) {
...

>+  if (!gBackgroundIsActive)
>+    return;
>+  if (!event.target.persona)
>+    return;
[Why separate statements?]
Attachment #8413402 - Flags: review?(neil) → review+
>>+  var usedThemes = LightweightThemeManager.usedThemes
>>+                                          .map( x => [x.id + ID_SUFFIX, x] );
>>+  usedThemes = new Map(usedThemes);
> Could move the .map to this line which has more spare space.
Fixed.

>>+    if (theme.id.endsWith(ID_SUFFIX)) {
>>+      var persona = usedThemes.get(theme.id);
> Actually my idea was to do
> var persona = usedThemes.get(theme.id);
> if (persona) {
> ...
Fixed.

>>+  if (!gBackgroundIsActive)
>>+    return;
>>+  if (!event.target.persona)
>>+    return;
> [Why separate statements?]
It just looks clearer to me, that's all. I'll smush them together on checkin.
Attachment #8413402 - Attachment is obsolete: true
Attachment #8413759 - Flags: review+
Pushed comm-central changeset d08a2b3e73e9
http://hg.mozilla.org/comm-central/rev/d08a2b3e73e9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: