340219 - When upgrading an extension the installed version number is displayed until the upgrade has been completed by a restart.

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060602 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060602 Minefield/3.0a1

1. From AMO install Leak Monitor 0.2. Restart Firefox.
2. From http://dbaron.org/mozilla/leak-monitor/ download Leak Monitor 0.3. Install from local drive.


Reproducible: Always

Actual Results:  
Add-ons dialog shows "Leak Monitor 0.2, Restart to Complete the installation".


Expected Results:  
Add-ons dialog shows "Leak Monitor 0.3, Restart to Complete the installation".

Comment 1

[Ria Klaassen (not reading all bugmail)]

Also the latest branch (WinXP) shows this problem.

Comment 2

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

Also happens with 1.5.

This was changed before 1.5 was released due to a failed upgrade of an extension retaining the new version number in the extensions datasource while continuing to use the old version of the extension.

It is probably possible to fix this visual glitch by making it clearer that the version number applies to the installed version. Not a high priority though.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

*** Bug 362825 has been marked as a duplicate of this bug. ***

Comment 4

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

note: the about page also has the old info until after restart.

Comment 10

[Dave Townsend [:mossop]]

Yoink!

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Somewhat simple fix, just pull in the new version.

It should be noted that this bug also affects all the rest of the item metedata, but generally names and descriptions don't change (or you wouldn't notice the change) as much as the version.

This has potential impact for extension authors. If they currently retrieve the version of their extension from the EM then when an upgrade has been set up but not happened they will of course get the wrong version back.

Comment 12

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

Dave, the reason we display the installed version is that if the install fails after restarting the new version number would be in the datasource. We either need to resync the version on failure or possibly use a new property that isn't actually stored in the datasource similar to the other dynamic properties. I think I prefer the latter but haven't thought it through thoroughly.

Comment 13

[Dave Townsend [:mossop]]

Can you point out to me where that happens since I searched and couldn't find it. To my eyes on restart we run through _finalizeUpgrade which wipes out all of the manifest data about the add-on and then through _finalizeInstall to actually attempt the install. If this fails I can see no explicit code to rebuild the original manifest data.

Comment 14

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

I believe it happens when we throw during an upgrade so _finalizUpgrade wouldn't be called which happened quite a lot after the EM rewrite for 1.5. Even with it not happening that often nowadays it is best to avoid this all together by using a dynamic property since when it happens the EM thinks it has the new version installed when in fact it has the old version. There is a bug I fixed way back when for this exact issue which is why it currently only displays the installed / operational version.

As I see it if the new version data is stored dynamically then the Installation view would just display the new version number after the installation and the extension, theme, etc. view would display the installed / operational version as it does today and the This add-on will be upgraded when appname is restarted... message would be modified to something like This add-on will be upgraded to version x.x.x when appname is restarted... 

Comment 15

[Dave Townsend [:mossop]]

Attachment: extensions pane

This is what I have working now.

In the extensions pane display the add-on along with its currently installed version and below a message that the add-on will be upgraded on restart and what version it will be upgraded to.

In the installs window display the add-on along with its new version and a message that it will be installed on restart.

I'm not sure if this inconsistency is sensible (the extensions pane is about currently installed versions and the installs pane about to be installed versions) or just a little confusing.

Comment 16

[Dave Townsend [:mossop]]

Attachment: installs pane

Comment 17

[Dave Townsend [:mossop]]

To be clear those last two screenshots are from the same circumstance, JavaScript Options 1.2.3 is currently installed and the upgrade to 1.2.4 is pending restart.

Comment 18

[Dave Townsend [:mossop]]

Note that this inconsistency does not exist in a fresh install-pending addon. In this case the new version is displayed in the extensions and installs pane so I need to make that work in the same way once we have a decision.

Comment 19

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

How about differentiating between updates vs. installs? This is very rough but something like
Upgrade
JavaScript Options 1.2.3
Restart to complete the update to version 1.2.4

Initial Install
JavaScript Options
Restart to complete the installation of version 1.2.4

Comment 20

[Dave Townsend [:mossop]]

Is that for both the extensions pane and installs pane?

Comment 21

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

Just the installs pane but I'm thinking it would be appropriate to do something similar for the other panes as applicable. I'm just throwing it out there as a possible way to address the inconsistency. Thoughts?

Comment 22

[Dave Townsend [:mossop]]

I think that works for both panes. Beltzner has promised me he's going to take a peek at this today so assuming he's happy I can get this together tomorrowish.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 268347 [details]
extensions pane

I'm not sure that the extra information adds a lot in this case, actually, and am concerned about the clutter it adds.

I'd entertain options that don't end up making the string so long, perhaps such as those proposed in comment 19, or something where we actually update the version number in the Add-ons pane as well, and make the string "This updated add-on will be available when %S is restarted."

Comment 24

[Dave Townsend [:mossop]]

Been through so many iterations I'm now a bit confused as to the decided solution, so here is what I have now and I'll grab Beltzner for a final look see.

On installi 1.2.3, in the Installation pane:

JavaScript Options 1.2.3
This new add-on will be available when Minefield is restarted.

And in the Extensions pane:

JavaScript Options 1.2.3
This add-on will be installed when Minefield is restarted.

On upgrade to 1.2.4, in the Installation pane:

JavaScript Options 1.2.4
This updated add-on will be available when Minefield is restarted.

And in the Extensions pane:

JavaScript Options 1.2.3
This add-on will be upgraded when Minefield is restarted.

---

So the extensions pane is as it is now, the installs pane has some minor status wording changes and shows the version that will be installed.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

That sounds right to me. In that last string, s/upgraded/updated/ ?

Comment 26

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This implements the strings from the previous comments.

Comment 27

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

Comment on attachment 269833 [details] [diff] [review]
patch rev 2

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
>retrieving revision 1.20
>diff -u8 -p -r1.20 extensions.dtd
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	4 Jan 2007 18:51:40 -0000	1.20
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	26 Jun 2007 10:25:58 -0000	
>@@ -90,27 +90,28 @@
> 
> <!-- Status Messsages -->
> <!ENTITY needsDependencies.label          "Requires additional items.">
> <!ENTITY blocklisted.label                "Disabled for your protection.">
> <!ENTITY toBeDisabled.label               "This add-on will be disabled when &brandShortName; is restarted.">
> <!ENTITY toBeEnabled.label                "This add-on will be enabled when &brandShortName; is restarted.">
> <!ENTITY toBeInstalled.label              "This add-on will be installed when &brandShortName; is restarted.">
> <!ENTITY toBeUninstalled.label            "This add-on will be uninstalled when &brandShortName; is restarted.">
>-<!ENTITY toBeUpgraded.label               "This add-on will be upgraded when &brandShortName; is restarted.">
>+<!ENTITY toBeUpgraded.label               "This add-on will be updated when &brandShortName; is restarted.">
Please change the ENTITY name so l10n will pick up the change

> 
> <!ENTITY getExtensions.label              "Get Extensions">
> <!ENTITY getExtensions.tooltip            "Get Extensions from addons.mozilla.org">
> <!ENTITY getThemes.label                  "Get Themes">
> <!ENTITY getThemes.tooltip                "Get Themes from addons.mozilla.org">
> <!ENTITY previewNoThemeSelected.label     "No Theme Selected">
> <!ENTITY previewNoPreviewImage.label      "This Theme does not have a Preview Image">
> <!ENTITY moreInfo.label                   "More Information">
> 
> <!ENTITY updateSuccess.label              "Update completed successfully.">
> <!ENTITY installSuccess.label             "Install completed successfully.">
>-<!ENTITY installSuccessRestart.label      "Restart to complete the installation.">
>+<!ENTITY installSuccessRestart.label      "This new add-on will be available when &brandShortName; is restarted.">
Please change the ENTITY name so l10n will pick up the change

This new add-on doesn't seem right to me but if beltzner approves it (and the other strings) I'm fine with it.

With those changes r=me

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Ah, hrm, I missed (sorry, sorry) the fact that the first case there was install, not update.

I think:

ENTITY installSuccessRestart.label      "Restart to complete the installation."

is fine the way it is, actually, and we should adjust the upgrade case to match, since the restart button is there now and everything. So:

+<!ENTITY toUpgradeRestart.label        "Restart to complete the update.">

Apologies for my earlier misdirection!

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Mossop managed to determine that I misdirected him again, saving everyone from yet another apology. Apparently I'm wearing my big ol' "idjit" helmet this morning. Of course, this is what I actually meant:

Extensions pane strings:

toBeInstalled.label         This add-on will be installed when &brandShortName; is restarted.
toBeUpgraded.label          This add-on will be updated when &brandShortName; is restarted.

Installs pane strings:

installSuccessRestart.label Restart to complete the installation.
upgradeSuccessRestart.label Restart to complete the update.

that gets my ui-r

Comment 30

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Fixed the strings and renamed the appropriate entities. Ready for checkin.

Comment 31

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

(In reply to comment #28)
>...
> is fine the way it is, actually, and we should adjust the upgrade case to
> match, since the restart button is there now and everything. So:
note: it was there before as well for the upgrade case. :P

Comment 32

[timeless]

Comment on attachment 268347 [details]
extensions pane

Assuming that the layout of this dialog is mostly correct (understanding that the strings have been minced). I'd like to note that as I read it, it says that this add-on will be updated to version 1.2.4 which is Not compatible with Minefield 3.0a6pre (which presumably is the version of minefield you're running).

IOW, I'm suggesting that the Not compatible line be placed *above* the the will update line.

(personally, I'd also indent it, but ...)

[Rhino] JavaScript Options 1.2.3
_______ Provides advanced ....
_______ === Not compatible with ....
_______ Add-on will be updated to 1.2.4 ....

The strings here are just examples use whatever someone approves, don't take them as part of the advice, I'm only concerned with layout today. __ and == are both used here for indentation although __ I suppose could be overflow from the picture above.

Comment 33

[Dave Townsend [:mossop]]

Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v  <--  extensions.dtd
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/mozapps/extensions/content/extensions.css;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.css,v  <--  extensions.css
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.122; previous revision: 1.121
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.44; previous revision: 1.43
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.224; previous revision: 1.223
done

Closing this bug, opened bug 386441 to look at potential changes to the ordering of the status messages.

Comment 34

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre ID:2007103004.