Closed Bug 953807 Opened 7 years ago Closed 7 years ago

Show in the Themes preference pane which theme are disabled

Categories

(Instantbird :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: florian, Assigned: benediktp)

Details

Attachments

(2 files, 4 obsolete files)

*** Original post on bio 364 at 2010-03-09 09:39:00 UTC ***

When a theme add-on is disabled (either disabled by the user or because of an incompatible version) in the add-on manager, it is still in the list of themes visible in the Themes prefpane, and the user has no visible way to understand why this theme is not usable.

The disabled themes should probably have their menuitem disabled (grayed) in the theme list, and in addition, it may be a good idea to add an icon or to append "(disabled") to the name.
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 364 as attmnt 393 at 2010-10-28 22:51:00 UTC ***

This patch is functional as far as I can tell, but I don't really like the code duplication. Therefore it's only a first version, requesting review to get Florian's opinion.

Changes: the patch adds either (disabled) or (incompatible) after the menu items for emoticon and message style themes, depending on what flags are set. 'Disabled' takes precedence of 'incompatible' here, since this order works around a few cases when compatibility checking is disabled, but doesn't really do harm if it isn't.

The menu item will be only deactivated if the extension is not active (i.e. not loaded) _or also_ if the userDisabled flag is set, the latter to cover special case #2.

Special cases: 

#1 the user disabled compatibility checking -> the item will be shown with the (incompatible) label but won't be disabled, so it can be chosen. The label is there so the user knows that it in is his own responsibility if he choses this theme.

#2: userDisabled, but active -> this case occurs when a theme was disabled, but the application hasn't been restarted yet. Disallow selecting this theme, since it won't be available after the restart.

A few words on the localization:
I renamed the file messagestyle.properties to themes.properties to include the translation of the new labels, which get used on both the message style tab and the emoticon tab.
This change could be reverted, e.g. if we decide that these two translations should be separated; one reason could be that there are languages that require different forms for items that represent emoticon themes or message style themes.
Comment on attachment 8352136 [details] [diff] [review]
Patch v1

*** Original change on bio 364 attmnt 393 at 2010-10-28 22:52:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352136 - Flags: review?(florian)
Assignee: nobody → benediktp
*** Original post on bio 364 at 2010-11-04 09:09:00 UTC ***

I don't really like the code duplication either, but by code inspection (I haven't tried it yet) it looks good otherwise.

To reduce the code duplication, you could add a buildThemeList method in gThemePane (in themes.js).
That method would take aThemeType (either "emoticons" or "messagestyle"), aEmptyItemId (either "noemoticons-menuitem" or "nomessagestyles-menuitem") and aMenulistId ("smileythemename" or "themename") as parameters.
Comment on attachment 8352136 [details] [diff] [review]
Patch v1

*** Original change on bio 364 attmnt 393 at 2010-12-16 09:32:09 UTC ***

r- per previous comment.
Attachment #8352136 - Flags: review?(florian) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 364 as attmnt 446 at 2010-12-24 00:17:00 UTC ***

Creating and adding the menu items on a separate method in themes.js now.
Attachment #8352189 - Flags: review?(florian)
Comment on attachment 8352136 [details] [diff] [review]
Patch v1

*** Original change on bio 364 attmnt 393 at 2010-12-24 00:17:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352136 - Attachment is obsolete: true
Comment on attachment 8352189 [details] [diff] [review]
Patch v2

*** Original change on bio 364 attmnt 446 at 2010-12-24 16:12:08 UTC ***

I thought the new method in themes.js would build the whole theme list (and construct the regexp only once) and not just append a single item. Apparently there was some misunderstanding :(.

Otherwise, coding style nits:

>diff --git a/instantbird/content/preferences/themes.js b/instantbird/content/preferences/themes.js

>+  appendThemeMenuItem: function(aMenuList, aItem, aExtensionType) {
>+      let aItemLabel = aItem.name;
2 spaces indent.

>+      if(!aItem.isActive || aItem.userDisabled)
space between if and (
Attachment #8352189 - Flags: review?(florian) → review-
Status: NEW → ASSIGNED
*** Original post on bio 364 as attmnt 451 at 2010-12-25 18:26:00 UTC ***

Moved buildThemeList to shared code of the themes pane.
Attachment #8352194 - Flags: review?(florian)
Comment on attachment 8352189 [details] [diff] [review]
Patch v2

*** Original change on bio 364 attmnt 446 at 2010-12-25 18:26:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352189 - Attachment is obsolete: true
Comment on attachment 8352194 [details] [diff] [review]
Patch v3 - changed to 'buildThemeList'

*** Original change on bio 364 attmnt 451 at 2010-12-25 19:11:03 UTC ***

>diff --git a/instantbird/content/preferences/themes.js b/instantbird/content/preferences/themes.js

>+  buildThemeList: function (aThemeType) {
>+    let noCustomStylesId;
>+    let listId;
>+
>+    switch (aThemeType) {
>+      case "emoticons":
>+        noCustomStylesId = "noemoticons-menuitem";
>+        listId = "smileythemename";
>+        break;
>+      case "messagestyle":
>+        noCustomStylesId = "nomessagestyles-menuitem";
>+        listId = "themename";
>+        break;
>+      default:
>+        throw "Encountered unknown style extension type " +
>+              "('" + aThemeType + "') while creating theme list.";
>+    }
Can't we simplify this a lot by adapting the ids of the XUL elements to be no-<aThemeType>-menuitem and <aThemeType>-themename?
These two variable may not even be needed anymore.

>+    let themeTypeRegExp = new RegExp("^" + aThemeType + "-");
>+    let themeList =
>+      gThemePane.getExtensionList()
gThemePane is "this" here :).

>+    themeList.forEach(function(aItem) {
>+      let aItemLabel = aItem.name;
a as the first letter of a variable name means argument. Here aItemLabel should be named itemLabel, or just label (as it's obvious it's the label of the current item).

>+      // Set it to deactivated if it is not active;
>+      //  this is independent from the reason displayed
Nit: . at the end of the sentence.

Looks good otherwise. Sorry it's not an r+ yet :-/.
Attachment #8352194 - Flags: review?(florian) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
*** Original post on bio 364 as attmnt 453 at 2010-12-26 20:30:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352196 - Flags: review?(florian)
Comment on attachment 8352194 [details] [diff] [review]
Patch v3 - changed to 'buildThemeList'

*** Original change on bio 364 attmnt 451 at 2010-12-26 20:30:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352194 - Attachment is obsolete: true
Attached patch Patch v4.1Splinter Review
*** Original post on bio 364 as attmnt 454 at 2010-12-26 20:34:00 UTC ***

Sorry, forgot the 'this' change. It's very impractical to test changes on an unpacked copy and having to move them to Hg once it works. Is there a better way to do it?

Sorry for the bugspam.
Attachment #8352197 - Flags: review?(florian)
Comment on attachment 8352196 [details] [diff] [review]
Patch v4

*** Original change on bio 364 attmnt 453 at 2010-12-26 20:34:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352196 - Attachment is obsolete: true
Attachment #8352196 - Flags: review?(florian)
Comment on attachment 8352197 [details] [diff] [review]
Patch v4.1

*** Original change on bio 364 attmnt 454 at 2010-12-26 23:21:02 UTC ***

r=fqueze. I still noticed a few coding style nits, I'm going to attach an interdiff of your patch vs the version I'm pushing.
Attachment #8352197 - Flags: review?(florian) → review+
*** Original post on bio 364 as attmnt 455 at 2010-12-26 23:25:00 UTC ***

(the empty lines changes were not strictly necessary, but as I was already editing the file, I moved the things to be how they make the most sense according to me... I hope you don't mind :))
*** Original post on bio 364 at 2010-12-26 23:27:43 UTC ***

Fixed, thanks!
https://hg.instantbird.org/instantbird/rev/894316e9bb37
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a1
*** Original post on bio 364 at 2010-12-26 23:34:34 UTC ***

(In reply to comment #9)

> [...] It's very impractical to test changes on an
> unpacked copy and having to move them to Hg once it works. Is there a better
> way to do it?

If your machine is beefy enough to build it from source, that's the good way.
*** Original post on bio 364 at 2011-02-07 15:30:48 UTC ***

Verified fixed on 0.3a1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.