Closed Bug 340219 Opened 15 years ago Closed 14 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla1.9alpha7

People

(Reporter: JasnaPaka, Assigned: mossop)

References

Details

Attachments

(3 files, 2 obsolete files)

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".
Also the latest branch (WinXP) shows this problem.
Summary: Add-ons dialog shows bad version of upgraded extension → When upgrading an extension the installed version nnumber is displayed until the upgrade has been completed by a restart.
Summary: When upgrading an extension the installed version nnumber is displayed until the upgrade has been completed by a restart. → When upgrading an extension the installed version number is displayed until the upgrade has been completed by a restart.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → trivial
*** Bug 362825 has been marked as a duplicate of this bug. ***
note: the about page also has the old info until after restart.
Duplicate of this bug: 348219
Duplicate of this bug: 367020
Duplicate of this bug: 376835
Duplicate of this bug: 377361
Duplicate of this bug: 379541
Yoink!
Assignee: nobody → dtownsend
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #267321 - Flags: review?(robert.bugzilla)
Status: NEW → ASSIGNED
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.
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.
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... 
Attachment #267321 - Attachment is obsolete: true
Attachment #267321 - Flags: review?(robert.bugzilla)
Attached image 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.
Attachment #268347 - Flags: ui-review?(beltzner)
Attached image installs pane
Attachment #268348 - Flags: ui-review?(beltzner)
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.
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.
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
Is that for both the extensions pane and installs pane?
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?
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.
Attachment #268348 - Flags: ui-review?(beltzner) → ui-review+
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."
Attachment #268347 - Flags: ui-review?(beltzner) → ui-review-
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.
That sounds right to me. In that last string, s/upgraded/updated/ ?
Attached patch patch rev 2 (obsolete) — Splinter Review
This implements the strings from the previous comments.
Attachment #269833 - Flags: review?(robert.bugzilla)
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
Attachment #269833 - Flags: review?(robert.bugzilla) → review+
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!
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
Attached patch patch rev 3Splinter Review
Fixed the strings and renamed the appropriate entities. Ready for checkin.
Attachment #269833 - Attachment is obsolete: true
Attachment #270012 - Flags: review+
(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 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.
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta1
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre ID:2007103004.
Status: RESOLVED → VERIFIED
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Duplicate of this bug: 406508
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.