408118 - Auto-enable themes on install

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Dave Townsend [:mossop]]

When a new theme is installed it should automatically be selected as the new theme to be used on the next restart. If 5 themes are installed before restarting then the most recent theme will be selected.

Comment 1

[Dave Townsend [:mossop]]

This will be a pretty trivial code change, just setting a pref in the EM component after a successful theme install.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Of course nothing is ever quite as simple as it seems. Turns out that there are a bunch of UI issues with the restart message when changing themes that this feature exposes that I have covered in this patch.

When switching to the themes page we have to check if a theme switch is pending and set the restart message on the appropriate item.

Since switches get triggered from the EM component we need to monitor the switching prefs so we can keep gCurrentTheme up to date in the add-ons UI if it happens to be open,

When switching theme if we are just cancelling the theme switch then just reset the prefs and clear any restart message.

When uninstalling the theme that is currently set to be switched to revert the theme switch, unless the current theme is already marked for uninstall in which case switch to the default theme.

In the EM component, when installing a new theme switch to it. This is only for new themes, not updates, and only if the install doesn't require a restart which should be true for all themes except the currently selected theme.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 295025 [details] [diff] [review]
patch rev 1

I'll finish this later...

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.266
>diff -u8 -p -r1.266 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	12 Dec 2007 00:56:09 -0000	1.266
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	1 Jan 2008 17:29:16 -0000	
>@@ -61,7 +61,6 @@ const PREF_UPDATE_DEFAULT_URL         = 
> const PREF_UPDATE_DEFAULT_URL         = "extensions.update.url";
> const PREF_EM_IGNOREMTIMECHANGES      = "extensions.ignoreMTimeChanges";
> const PREF_EM_DISABLEDOBSOLETE        = "extensions.disabledObsolete";
>-const PREF_EM_LAST_SELECTED_SKIN      = "extensions.lastSelectedSkin";
> const PREF_EM_EXTENSION_FORMAT        = "extensions.%UUID%.";
> const PREF_EM_ITEM_UPDATE_ENABLED     = "extensions.%UUID%.update.enabled";
> const PREF_EM_UPDATE_ENABLED          = "extensions.update.enabled";
>@@ -4434,8 +4433,19 @@ ExtensionManager.prototype = {
>       else {
>         this._configureForthcomingItem(installManifest, installData.id,
>                                         installLocation, installData.type);
>-        if (!restartRequired)
>+        if (!restartRequired) {
>           this._finalizeInstall(installData.id, stagedFile);
>+          if (installData.type == Ci.nsIUpdateItem.TYPE_THEME) {
>+            var internalName = this.datasource.getItemProperty(installData.id, "internalName");
>+            if (gPref.getBoolPref(PREF_EM_DSS_ENABLED)) {
>+              gPref.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, internalTheme);
internalTheme? Shouldn't this be internalName?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> >+            var internalName = this.datasource.getItemProperty(installData.id, "internalName");
> >+            if (gPref.getBoolPref(PREF_EM_DSS_ENABLED)) {
> >+              gPref.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, internalTheme);
> internalTheme? Shouldn't this be internalName?
> 

Err yes quite right. Obviously need to recheck this with dynamic theme switching enabled.

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 295025 [details] [diff] [review]
patch rev 1

I'd prefer to use PREF_EM_LAST_SELECTED_SKIN instead of PREF_DSS_SKIN_TO_SELECT throughout unless there is a good reason not to.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.153
>diff -u8 -p -r1.153 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	3 Dec 2007 12:24:41 -0000	1.153
>+++ toolkit/mozapps/extensions/content/extensions.js	1 Jan 2008 19:18:07 -0000	
>...
>@@ -102,6 +102,21 @@ function setElementDisabledByID(aID, aDo
>     else
>       element.removeAttribute("disabled");
>   }
>+}
>+
>+/**
>+ * This returns the richlistitem element for the theme with the given
>+ * internalName
>+ */
>+function getItemForInternalName(aInternalName) {
>+  var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                      .getService(Components.interfaces.nsIRDFService);
>+  var property = rdf.GetResource(PREFIX_NS_EM + "internalName");
>+  var name = rdf.GetLiteral(aInternalName);
>+  var id = gExtensionManager.datasource.GetSource(property, name, true)
>+  if (id && id instanceof Components.interfaces.nsIRDFResource)
>+    return document.getElementById(id.ValueUTF8);
>+  return null;
Be sure to change this to use gRDF as added by the patch in bug 404024

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.266
>diff -u8 -p -r1.266 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	12 Dec 2007 00:56:09 -0000	1.266
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	1 Jan 2008 17:29:16 -0000	
>...
>@@ -4434,8 +4433,19 @@ ExtensionManager.prototype = {
>       else {
>         this._configureForthcomingItem(installManifest, installData.id,
>                                         installLocation, installData.type);
>-        if (!restartRequired)
>+        if (!restartRequired) {
>           this._finalizeInstall(installData.id, stagedFile);
>+          if (installData.type == Ci.nsIUpdateItem.TYPE_THEME) {
>+            var internalName = this.datasource.getItemProperty(installData.id, "internalName");
>+            if (gPref.getBoolPref(PREF_EM_DSS_ENABLED)) {
>+              gPref.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, internalTheme);
s/internalTheme/internalName/

r=me with those changes. I'm ok with removing PREF_EM_LAST_SELECTED_SKIN and using PREF_DSS_SKIN_TO_SELECT if there is a good reason to do so.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> r=me with those changes. I'm ok with removing PREF_EM_LAST_SELECTED_SKIN and
> using PREF_DSS_SKIN_TO_SELECT if there is a good reason to do so.

To me PREF_DSS_SKIN_TO_SELECT seems far more descriptive since that is what it is, the skin to change to on next restart. PREF_EM_LAST_SELECTED_SKIN suggests it is the previously selected skin, which I think it might have been at one point from looking at comments but it isn't used for that anymore from what I see.

Comment 7

[Robert Strong (they/them - no direct email)]

I'm ok with using it... it is just that we have extensions.dss.* prefs and this is not one of them even though it has dss in the constant's name.

Comment 8

[Dave Townsend [:mossop]]

Well we could rename to PREF_SKIN_TO_SELECT or PREF_EM_SKIN_TO_SELECT, I agree it isnt really a Dynamic Skin Switching pref.

Comment 9

[Robert Strong (they/them - no direct email)]

That would be fine but I'm ok with leaving it if it is a PITA

Comment 10

[Dave Townsend [:mossop]]

Checked in, left the pref as is, we can file something separately if we want to normalise that everywhere.

Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.157; previous revision: 1.156
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.273; previous revision: 1.272
done

Comment 11

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020418 Firefox/3.0b3.