Add post-restart notification of new add-on installs

VERIFIED FIXED in mozilla1.9beta5

Status

()

P2
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({late-l10n})

Trunk
mozilla1.9beta5
late-l10n
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

(Assignee)

Description

11 years ago
We want to add some notification post-restart that some add-ons have been installed.

This will be by bringing up the add-ons manager with a notification bar in it with the install count (possibly taking to the appropriate pane but that gets complicated on multi-type installs).
(Assignee)

Comment 1

11 years ago
This will involve tracking an "un-notified installs count" somewhere, likely a pref the same as "un-notified updates" is currently done.

Questions to be answered are what exactly are we tracking? New installs? updated add-ons? Do we separate out things done through the UI from those done by dropping files/folders into the profile directory?
Whiteboard: [swag 1]
(Assignee)

Updated

11 years ago
Blocks: 404024
(Assignee)

Updated

11 years ago
Flags: blocking-firefox3?
(In reply to comment #1)
> Questions to be answered are what exactly are we tracking? New installs?
> updated add-ons? Do we separate out things done through the UI from those done
> by dropping files/folders into the profile directory?

We should track new installs, I don't think we need to do updates. My feeling is that we want to show this for both UI based installs as well as ones that have just been slurped up from the filesystem.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
(Assignee)

Comment 3

11 years ago
I have code almost much ready that implements this. One bonus feature I've managed to implement is that we can highlight newly installed items in the UI (like windows does on it's start menu). I'm thinking of just a special background colour for them or something. Thoughts?
Keywords: uiwanted
(Assignee)

Updated

11 years ago
Blocks: 407901
(Assignee)

Comment 4

11 years ago
And one irritation. Seems there isn't an easy way to open the add-ons UI after startup and have that window appear on top. We have 3 options:

1. Open the add-ons UI modal before any application windows, irritating to the user I think.
2. Use app-specific code to open the UI.
3. Set on a delay and open 10 seconds or so after startup (ick!)

So I guess 2 is the option to go for.
(Assignee)

Updated

11 years ago
No longer blocks: 407901
(Assignee)

Comment 5

11 years ago
Created attachment 295842 [details] [diff] [review]
EM patch rev 1
[(String only) Checkin: Comment 10]

This is the EM portion of the patch that tracks the add-ons that were installed during startup. It doesn't actually display any notification on startup, that must be done by app-specific code, I have a patch for Firefox to do that to follow.

What this does is:

Counts the number of new add-on installs during startup (doesn't count upgrades, does count new add-ons spotted via the filesystem, per comment 2).
When the Add-ons UI is displayed for the first time in a session when some installs happened a notification bar is displayed showing how many were installed and any that were installed are highlighted (Madhava has agreed on the string and highlight colour).

How it does it:

During the pass through finishOperations we track the items that are upgrades and then the items that are installed that aren't upgrades are added to the global install list. Finally any uninstalls are removed from that list. After the operations are complete the app will restart so the list of items is stored to a pref temporarily.

On startup the EM pulls the list of installs from the preference and clears it. We then know what was installed, provide that could via an attribute and use a pseudo rdf-property to show the newly installed state of the items.

An simple attribute is used to indicate the presence of installs because some apps may want to just display a notification of the number of installs rather than open the Add-ons UI. A full method to retrieve all the installed items however would cause us to hit the datasource during startup.

Another preference is used to track whether the Add-ons UI needs to display the install status. The EM sets it to true on startup if there are new items. When the Add-ons UI is opened it is reset to false, any notification displayed and an attribute set to allow styling.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #295842 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

11 years ago
Keywords: uiwanted
Whiteboard: [swag 1] → [has patch]
(Assignee)

Updated

11 years ago
Depends on: 411207

Comment 6

11 years ago
itchy fingers. removed gavin by mistake.
(Assignee)

Updated

11 years ago
Keywords: late-l10n

Comment 7

11 years ago
Will Add-ons themselves be able to hook into this, for example to check if itself has just been installed? Use case - to load a post-install page.

Comment 8

11 years ago
We have plurals support now, we should use that.

Given that we're dead-on string frozen by now, do we still want to take this bug?
Let's just land the string now without plural support so we've got it in.
Keywords: checkin-needed
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.51; previous revision: 1.50
done
Keywords: checkin-needed

Comment 11

11 years ago
You've landed, and now I have a headache, as in my language in this particular phrase, endings of three out of three words depend on the number used... The result is ugly. :(
So, can we still have this with plurals support before beta 4? 

Updated

11 years ago
Whiteboard: [has patch] → [has patch][needs review rstrong]

Updated

11 years ago
Priority: P3 → P4
(Assignee)

Comment 13

11 years ago
Per the meeting today and the fact that this is an l10n change, we need to land this by the l10n freeze on Friday.
(Assignee)

Comment 14

11 years ago
Created attachment 307725 [details] [diff] [review]
patch rev 2

This is the patch updated to trunk and with plurals support.
Attachment #295842 - Attachment is obsolete: true
Attachment #307725 - Flags: review?(robert.bugzilla)
Attachment #295842 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 15

11 years ago
Created attachment 307726 [details] [diff] [review]
Browser patch

This is the browser patch necessary to actually display the notification on startup.
Comment on attachment 307725 [details] [diff] [review]
patch rev 2

>diff -r 006e7aca5662 toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js	Thu Mar 06 06:05:34 2008 -0800
>+++ b/toolkit/mozapps/extensions/content/extensions.js	Thu Mar 06 16:55:35 2008 +0000
>@@ -78,16 +78,17 @@ const PREF_EM_CHECK_COMPATIBILITY       
>...
>@@ -1089,16 +1092,32 @@ function Startup()
>                   msgText, buttonLabel, buttonAccesskey,
>                   true, notifyData);
>     }
>   }
>   if (gInSafeMode) {
>     showMessage("chrome://mozapps/skin/extensions/question.png",
>                 getExtensionString("safeModeMsg"),
>                 null, null, true, null);
>+  }
>+
>+  var highlightInstalls = false;
>+  try {
>+    highlightInstalls = gPref.getBoolPref(PREF_EXTENSIONS_HIGHLIGHT_INSTALLS);
>+    gPref.setBoolPref(PREF_EXTENSIONS_HIGHLIGHT_INSTALLS, false);
Why aren't you setting this pref to false below since you have a true check for this pref below?
nit: also, why not just clear the pref?

>+  } catch(e) { }
>+  if (highlightInstalls) {
>+    var installs = gExtensionManager.newAddonsCount;
>+    if (installs > 0) {
>+      var addonsTerm = PluralForm.get(installs, getExtensionString("addonsPlural"));
>+      gExtensionsView.setAttribute("highlightInstalls", "true");
>+      showMessage("chrome://mozapps/skin/extensions/question.png",
>+                  getExtensionString("newAddonsNotificationMsg", [installs, addonsTerm]),
>+                  null, null, true, null);
>+    }
>   }
It seems like you should be able to simplify these changes by moving the check of the installedAddons pref into extensions.js, set the elements accordingly, and thereby avoid the addition of newAddonsCount to the EM, etc. You could then check this pref during startup in nsBrowserGlue.js to know whether or not to open the add-ons manager.

Also, the pref name installedAddons implies all installed add-ons when it should only be for new / updated add-ons.

>diff -r 006e7aca5662 toolkit/themes/pinstripe/mozapps/extensions/extensions.css
>--- a/toolkit/themes/pinstripe/mozapps/extensions/extensions.css	Thu Mar 06 06:05:34 2008 -0800
>+++ b/toolkit/themes/pinstripe/mozapps/extensions/extensions.css	Thu Mar 06 16:55:35 2008 +0000
>@@ -55,16 +55,20 @@ richlistitem[isDisabled="true"] .addonIc
> 
> richlistitem[isDisabled="true"] {
>   color: GrayText;
> }
> 
> richlistitem[selected="true"] {
>   background-color: Highlight;
>   color: HighlightText;
>+}
>+
>+#extensionsView[highlightInstalls="true"] richlistitem[newInstall="true"] {
>+  background-color: #fdf2ab;
bah... no system color available for this on Mac OS X?

r- unless there is a good reason for implementing the _rdfGet_newInstall property vs. in extensions.js
Attachment #307725 - Flags: review?(robert.bugzilla) → review-
Whiteboard: [has patch][needs review rstrong]
(Assignee)

Comment 17

11 years ago
(In reply to comment #16)
> >+  } catch(e) { }
> >+  if (highlightInstalls) {
> >+    var installs = gExtensionManager.newAddonsCount;
> >+    if (installs > 0) {
> >+      var addonsTerm = PluralForm.get(installs, getExtensionString("addonsPlural"));
> >+      gExtensionsView.setAttribute("highlightInstalls", "true");
> >+      showMessage("chrome://mozapps/skin/extensions/question.png",
> >+                  getExtensionString("newAddonsNotificationMsg", [installs, addonsTerm]),
> >+                  null, null, true, null);
> >+    }
> >   }
> It seems like you should be able to simplify these changes by moving the check
> of the installedAddons pref into extensions.js, set the elements accordingly,
> and thereby avoid the addition of newAddonsCount to the EM, etc. You could then
> check this pref during startup in nsBrowserGlue.js to know whether or not to
> open the add-ons manager.

I was thinking of keeping the applications only needing to use a pure api call
on the EM to determine whether they need to open the add-ons manager, but sure
I can just have then check the pref instead.

> > richlistitem[selected="true"] {
> >   background-color: Highlight;
> >   color: HighlightText;
> >+}
> >+
> >+#extensionsView[highlightInstalls="true"] richlistitem[newInstall="true"] {
> >+  background-color: #fdf2ab;
> bah... no system color available for this on Mac OS X?

No, this is just a copy of what the notification bar uses as Madhava requested.

> r- unless there is a good reason for implementing the _rdfGet_newInstall
> property vs. in extensions.js

The only issue here is that without this I'd have to manually set the
newInstall attribute on the richlistitems everytime you changed pane but I can
do that. I just never like adding attributes when the template builder could
come along and screw with them later.
(Assignee)

Comment 18

11 years ago
Created attachment 307862 [details] [diff] [review]
patch rev 3

This ditches the pref to determine whether to show the add-ons manager and just uses the single pref listing the id's of new add-ons. Also renames that one. This has the neat effect that if an application does nothing with it then whenever the user opens the addons manager it shows him everything installed since they last opened it.

As I suspected the template builder screws around if we just set attributes in showView (The Get Add-ons pane gets it's results later, adds to the datasource and so the view gets rebuilt and all added attributes lost). So this adds a template build listener and adds the attributes from there which works nicely. Also moved across the restart message attribute to there which should work better than where it is now.
Attachment #307725 - Attachment is obsolete: true
Attachment #307862 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 19

11 years ago
Created attachment 307863 [details] [diff] [review]
browser patch rev 2

Updated to use the pref rather than the API as per the changes above.
Attachment #307726 - Attachment is obsolete: true
Comment on attachment 307862 [details] [diff] [review]
patch rev 3

It seems that you could remove gNewAddons from nsExtensionManager.js.in and just use updates in _finishOperations
Comment on attachment 307862 [details] [diff] [review]
patch rev 3

It seems like this should force the view to be the extensions view when new add-ons are installed. In seems that this will also display for theme and locale installations which I'm not sure we want to do... thoughts?
(Assignee)

Comment 22

11 years ago
(In reply to comment #21)
> (From update of attachment 307862 [details] [diff] [review])
> It seems like this should force the view to be the extensions view when new
> add-ons are installed. In seems that this will also display for theme and
> locale installations which I'm not sure we want to do... thoughts?

I think I chatted with Madhava and he was wanting all new items. However I think highlighting the themes is possibly redundant since the app will have switched to the theme so it'll be pretty obvious. I guess locales the same. I'll see what he says.

As for switching the pane, that seems fine if we're only highlighting extensions I think. Will get a new patch up shortly.
hmmm... one of my concerns is that we'll be showing the ui and if the selected pane doesn't have a newly installed add-on it will be rather confusing - for example, if I install a theme and the last selected pane is extensions then it won't be obvious what was installed or even necessary for that matter to display this ui if I just installed a theme or locale.

on the other hand I do think displaying all newly installed add-ons is better.

Comment 24

11 years ago
Hint: you could use the Updates pane to list not just available updates, but also installed ones, and focus that pane. Or create a new one to list installed updates, perhaps.
(Assignee)

Comment 25

11 years ago
(In reply to comment #23)
> hmmm... one of my concerns is that we'll be showing the ui and if the selected
> pane doesn't have a newly installed add-on it will be rather confusing - for
> example, if I install a theme and the last selected pane is extensions then it
> won't be obvious what was installed or even necessary for that matter to
> display this ui if I just installed a theme or locale.
> 
> on the other hand I do think displaying all newly installed add-ons is better.

Well I could on startup check through the list of new add-ons, figure out whether the majority are extensions/themes/locales and then display that pane. Majority of cases I guess there will only be one but that should be sane for the more edge cases.(In reply to comment #24)

> Hint: you could use the Updates pane to list not just available updates, but
> also installed ones, and focus that pane. Or create a new one to list installed
> updates, perhaps.

That would kind of e a nice idea, but right now we have panes coming out of our ears and adding a new one isn't going to help matters I think. And trying to re-use an existing pane is a higher impact than I'd like at this point.
(In reply to comment #25)
> Well I could on startup check through the list of new add-ons, figure out
> whether the majority are extensions/themes/locales and then display that pane.
> Majority of cases I guess there will only be one but that should be sane for
> the more edge cases.(In reply to comment #24)
If it is reasonable to do this I think it is sufficient for FF3

Yeah, adding another pane and providing the different actions would be a PITA
(Assignee)

Comment 27

11 years ago
Created attachment 307936 [details] [diff] [review]
patch rev 4

This drops the global variable from the component. It still needs to keep the new list and the upgrade list separate, this allows us to only notify the user about brand new add-ons, not upgrades.

This also automatically switches the pane to the most appropriate one based on the newly installed add-ons.

There is one problem I've just spotted. For brand new profiles this will notify for any already existing system add-ons, including the default theme. Need to come up with a way around that.
Attachment #307862 - Attachment is obsolete: true
Attachment #307936 - Flags: review?(robert.bugzilla)
Attachment #307862 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 28

11 years ago
Created attachment 307952 [details] [diff] [review]
patch rev 5

This adds some first run detection to avoid any install notifications whenever the startup cache was not present. This covers both new profiles where the only add-ons should be system provided and the user doesn't need to be bombarded with those I think as well as when any files go corrupt and we wipe the cache and start afresh, in which case we are in no fit state to say what is a new add-on and what isn't.

Fairly simple global flag that is set to true when we try to load from the cache and it is missing. If the flag is set, don't write out the new add-ons to the pref.
Attachment #307936 - Attachment is obsolete: true
Attachment #307952 - Flags: review?(robert.bugzilla)
Attachment #307936 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 29

11 years ago
Comment on attachment 307863 [details] [diff] [review]
browser patch rev 2

This is the browser portion of the patch. After all browser windows are open it checks if there are new add-ons listed in the pref and if so fires up the add-ons manager.
Attachment #307863 - Flags: review?(gavin.sharp)
Comment on attachment 307952 [details] [diff] [review]
patch rev 5

great!
Attachment #307952 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 307863 [details] [diff] [review]
browser patch rev 2

>diff -r 006e7aca5662 browser/components/nsBrowserGlue.js

>+  // Browser startup complete. All initial windows have opened.
>+  _onBrowserStartup: function()
>+  {
>+    var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                     getService(Ci.nsIPrefBranch);
>+    // If new add-ons were installed during startup open the add-ons manager.
>+    if (prefBranch.prefHasUserValue("extensions.newAddons")) {
>+      const EMTYPE = "Extension:Manager";
>+      var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Components.interfaces.nsIWindowMediator);
>+      var theEM = wm.getMostRecentWindow(EMTYPE);
>+      if (theEM) {
>+        theEM.focus();
>+        return;
>+      }
>+    
>+      const EMURL = "chrome://mozapps/content/extensions/extensions.xul";
>+      const EMFEATURES = "chrome,menubar,extra-chrome,toolbar,dialog=no,resizable";
>+      var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+               getService(Ci.nsIWindowWatcher);
>+      ww.openWindow(null, EMURL, "_blank", EMFEATURES, null);
>+    }

Should you reset the pref here, and pass in some flag to the EM, rather than relying on the EM clearing it? Would avoid potential problems with this code always running if the EM fails to reset the pref for whatever reason, and would consolidate the pref-checking code. Not sure about the "already exists" case - won't the EM window have already reset the pref if it's open?
Attachment #307863 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 32

11 years ago
Created attachment 308041 [details] [diff] [review]
combined patch
[Checkin: Comment 36]

This is a combined patch with teh changes from Gavin. Drops the check for whether the window is already open. Also the application now clears the pref and if the pref is set it opens the addons window passing it two string arguments, the first is ignored (currently used elsewhere for specifying the pane to display), the second is the comma separated list of new items.
Attachment #307863 - Attachment is obsolete: true
Attachment #307952 - Attachment is obsolete: true
Attachment #308041 - Flags: review?(robert.bugzilla)
Attachment #308041 - Flags: review?(gavin.sharp)
--> blocking P2

Also, this has blanket approval for string changes if it lands in time for the freeze.
Priority: P4 → P2
Comment on attachment 308041 [details] [diff] [review]
combined patch
[Checkin: Comment 36]

r=me for the EM portions of the patch
Attachment #308041 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 35

11 years ago
Comment on attachment 308041 [details] [diff] [review]
combined patch
[Checkin: Comment 36]

Adding r+ from gavin on IRC.
Attachment #308041 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 36

11 years ago
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.71; previous revision: 1.70
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.52; previous revision: 1.51
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.170; previous revision: 1.169
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.279; previous revision: 1.278
done
Checking in toolkit/themes/gnomestripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.39; previous revision: 1.38
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.48; previous revision: 1.47
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5

Comment 37

11 years ago
Comment on attachment 308041 [details] [diff] [review]
combined patch
[Checkin: Comment 36]

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>--- a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>@@ -1,8 +1,12 @@ aboutWindowTitle=About %S
>+# LOCALIZATION NOTE: Semi-colon list of plural forms.
>+# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>+addonsPlural=add-on;add-ons
>+
[...]
>-newAddonsInstalledMsg=%S new add-ons have been installed.
>+newAddonsNotificationMsg=%S new %S installed.

This part of the patch actually is a problem for localizers, as "new" needs to change for singular/plural forms for many languages as well, see e.g. "1 neues Add-on"/"2 neue Add-ons" in German.
(In reply to comment #37)
> [...]
> >-newAddonsInstalledMsg=%S new add-ons have been installed.
> >+newAddonsNotificationMsg=%S new %S installed.
> 
> This part of the patch actually is a problem for localizers, as "new" needs to
> change for singular/plural forms for many languages as well, see e.g. "1 neues
> Add-on"/"2 neue Add-ons" in German.
> 

Yeah, and for Russian it's even more complex, with one form (nominative singular) for 1, 21, 31, ...; another one (genitive singular) for 2-4, 22-24, ...; and a third one (genitive plural) for 5-20, 25-30, ...

I wonder if there's a formula which can be easily "localized" to all languages.

Updated

11 years ago
Depends on: 421635

Comment 39

11 years ago
Is there a pref to turn this behavior off, for users with a lot of extensions re-opening add-ons window on startup is just another costly behavior.

Can someone test this experience with 30-50+ extensions installed?  

Is there a delay after session window is restored?

Has anyone measured startup/restart impact after installing, with chrome and components needing to be re-registered, etc? this post session restored popup window just delays the whole process.

Additionally, why couldn't this just be a alert/notification on restart/startup like the app and add-ons alerts?

As an extension developer this would drive me mad, delayed or not.  
Created attachment 308115 [details]
Screenshot

1. install Add-ons
2. restart Fiefox
3. show the Add-ons Manager
4. "Get Add-ons" pane

All Buttons show in #commandBarBottom
(In reply to comment #40)
> Created an attachment (id=308115) [details]
> Screenshot

This is the same issue which I already reported in bug 419929 but wasn't able to reproduce. If this STR works at any time it would be great if you could comment there.
> This part of the patch actually is a problem for localizers, as "new" needs to
> change for singular/plural forms for many languages as well, see e.g. "1 neues
> Add-on"/"2 neue Add-ons" in German.

In Italian not only "new" need to change for singular/plural forms, also "installed"! 

(Assignee)

Comment 43

11 years ago
(In reply to comment #42)
> > This part of the patch actually is a problem for localizers, as "new" needs to
> > change for singular/plural forms for many languages as well, see e.g. "1 neues
> > Add-on"/"2 neue Add-ons" in German.
> 
> In Italian not only "new" need to change for singular/plural forms, also
> "installed"! 

Sorry guys this is indeed a problem. I have a plan to fix, hopefully I can get it done and in very shortly.

Comment 44

11 years ago
(In reply to comment #37)
> >-newAddonsInstalledMsg=%S new add-ons have been installed.
> >+newAddonsNotificationMsg=%S new %S installed.
> This part of the patch actually is a problem for localizers, as "new" needs to
> change for singular/plural forms for many languages as well, see e.g. "1 neues
> Add-on"/"2 neue Add-ons" in German.
Just double-checking, but this should work?

newAddonsNotificationMsg=%S new add-on installed.;%S new add-ons installed.

So in german:
newAddonsNotificationMsg=%S neues Add-on;%S neue Add-ons

Mossop: There shouldn't be a gender issue here because the noun is always "add-on", so the localizer knows to put the right adjective. My issue with minutes vs days was that both the number and noun could be changing.

Comment 45

11 years ago
I suppose if you want to keep close to the original message:

%S new add-on has been installed.;%S new add-ons have been installed.
(Assignee)

Comment 46

11 years ago
Created attachment 308131 [details] [diff] [review]
localisation fix
[Checkin: Comment 59]

This changes to let different plural forms change the entire string rather than just the one term.
(Assignee)

Comment 47

11 years ago
Comment on attachment 308131 [details] [diff] [review]
localisation fix
[Checkin: Comment 59]

Don't suppose you are around Rob?
Attachment #308131 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 48

11 years ago
(In reply to comment #40)
> Created an attachment (id=308115) [details]
> Screenshot
> 
> 1. install Add-ons
> 2. restart Fiefox
> 3. show the Add-ons Manager
> 4. "Get Add-ons" pane
> 
> All Buttons show in #commandBarBottom
> 

Oops, have a fix for this coming.
(Assignee)

Comment 49

11 years ago
Created attachment 308143 [details] [diff] [review]
bustage fix for windows/linux
[Checkin: Comment 50]

This fixes the dialog for windows and linux users.
(Assignee)

Comment 50

11 years ago
Landed the bustage fix.

Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.171; previous revision: 1.170
done
(In reply to comment #46)
> Created an attachment (id=308131) [details]
> localisation fix
> 
> This changes to let different plural forms change the entire string rather than
> just the one term.
> 

What about Russian (and, I think, other Slavic languages) where the noun takes up one form for 1 (and 21 but I don't think the number of addons installed together can rise that high in practice), another form for 2, 3 or 4, and a 3rd form for 5 or more (till 20)?
(Assignee)

Comment 52

11 years ago
(In reply to comment #51)
> (In reply to comment #46)
> > Created an attachment (id=308131) [details] [details]
> > localisation fix
> > 
> > This changes to let different plural forms change the entire string rather than
> > just the one term.
> > 
> 
> What about Russian (and, I think, other Slavic languages) where the noun takes
> up one form for 1 (and 21 but I don't think the number of addons installed
> together can rise that high in practice), another form for 2, 3 or 4, and a 3rd
> form for 5 or more (till 20)?

I was under the impression that the plural form library supported all of that (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.237_.283_forms.29)? Is that not the case? If so then I guess the only way forward is to drop the number here and go for a basic "New add-ons have been installed" message instead.
(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #46)
> > > Created an attachment (id=308131) [details] [details] [details]
> > > localisation fix
> > > 
> > > This changes to let different plural forms change the entire string rather than
> > > just the one term.
> > > 
> > 
> > What about Russian (and, I think, other Slavic languages) where the noun takes
> > up one form for 1 (and 21 but I don't think the number of addons installed
> > together can rise that high in practice), another form for 2, 3 or 4, and a 3rd
> > form for 5 or more (till 20)?
> 
> I was under the impression that the plural form library supported all of that
> (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.237_.283_forms.29)?
> Is that not the case? If so then I guess the only way forward is to drop the
> number here and go for a basic "New add-ons have been installed" message
> instead.
> 

Ah, OK: if the l10n routine is "clever enough" to care about a variable number of plural forms, as shown above and below that anchor, then I withdraw my objection.

Comment 54

11 years ago
> I was under the impression that the plural form library supported all of that
> (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.237_.283_forms.29)?
> Is that not the case? If so then I guess the only way forward is to drop the
> number here and go for a basic "New add-ons have been installed" message
> instead.

But for other Slavic language you should consider still plural rule 8 (Slovakian and Czech, (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.238_.283_forms.29)?), plural rule 9 (Polish, (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.239_.283_forms.29)?) and plural rule 10 (Slovenian and Sorbian, (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.2310_.284_forms.29)?). Please consider that Slovenian and Sorbian addionally have a dual form (used for 2 things or persons)
(Assignee)

Comment 55

11 years ago
(In reply to comment #54)
> > I was under the impression that the plural form library supported all of that
> > (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.237_.283_forms.29)?
> > Is that not the case? If so then I guess the only way forward is to drop the
> > number here and go for a basic "New add-ons have been installed" message
> > instead.
> 
> But for other Slavic language you should consider still plural rule 8
> (Slovakian and Czech,
> (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.238_.283_forms.29)?),
> plural rule 9 (Polish,
> (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.239_.283_forms.29)?)
> and plural rule 10 (Slovenian and Sorbian,
> (http://developer.mozilla.org/en/docs/Localization_and_Plurals#Plural_rule_.2310_.284_forms.29)?).
> Please consider that Slovenian and Sorbian addionally have a dual form (used
> for 2 things or persons)

The code will use whatever plural rule the locale is set to. If the locale isn't set to the right rule then it is up to the locale to fix it. If there is no plural rule that works for the locale then the plurals support needs to be fixed for that locale.
Comment on attachment 308131 [details] [diff] [review]
localisation fix
[Checkin: Comment 59]

r=mano
Attachment #308131 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 57

11 years ago
Comment on attachment 308131 [details] [diff] [review]
localisation fix
[Checkin: Comment 59]

Seeking approval to land post string freeze
Attachment #308131 - Flags: approval1.9?
Comment on attachment 308131 [details] [diff] [review]
localisation fix
[Checkin: Comment 59]

a=shaver for localization fix, cover post sent to .l10n, Mossop will provide tests this week out of his promised 2-day allocation. :)
Attachment #308131 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 59

11 years ago
Localisation fix checked in.

Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.53; previous revision: 1.52
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.172; previous revision: 1.171
done
Depends on: 421957
Blocks: 422029

Comment 60

11 years ago
The add-on installation notification should occur WITHOUT the opening of the Add-on Manager. By forcing the opening of the AM rather than a simple pop-up window, use of the browser is unnecessarily delayed.

(In reply to comment #39)
> Is there a pref to turn this behavior off, for users with a lot of extensions
> re-opening add-ons window on startup is just another costly behavior.
> 
> Can someone test this experience with 30-50+ extensions installed?  
> 
> Is there a delay after session window is restored?
> 
> Has anyone measured startup/restart impact after installing, with chrome and
> components needing to be re-registered, etc? this post session restored popup
> window just delays the whole process.
> 
> Additionally, why couldn't this just be a alert/notification on restart/startup
> like the app and add-ons alerts?
> 
> As an extension developer this would drive me mad, delayed or not.  
> 

Blocks: 422240
(In reply to comment #60)
> The add-on installation notification should occur WITHOUT the opening of the
> Add-on Manager. By forcing the opening of the AM rather than a simple pop-up
> window, use of the browser is unnecessarily delayed.

Same thing is bothering me after testing this feature within the last days. I filed bug 422240.
(Assignee)

Updated

11 years ago
No longer blocks: 422240
Blocks: 422133

Comment 62

11 years ago
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre

Also verified on a locale:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; es-ES; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
Blocks: 442167
Attachment #308143 - Attachment description: bustage fix for windows/linux → bustage fix for windows/linux [Checkin: Comment 50]
Attachment #308131 - Attachment description: localisation fix → localisation fix [Checkin: Comment 59]
Attachment #308041 - Attachment description: combined patch → combined patch [Checkin: Comment 36]
Attachment #295842 - Attachment description: EM patch rev 1 → EM patch rev 1 [(String only) Checkin: Comment 10]
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.