Closed
Bug 408115
Opened 17 years ago
Closed 16 years ago
Add post-restart notification of new add-on installs
Categories
(Toolkit :: Add-ons Manager, defect, P2)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla1.9beta5
People
(Reporter: mossop, Assigned: mossop)
References
()
Details
(Keywords: late-l10n)
Attachments
(4 files, 7 obsolete files)
24.68 KB,
patch
|
mossop
:
review+
robert.strong.bugs
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
8.08 KB,
image/png
|
Details | |
3.56 KB,
patch
|
asaf
:
review+
shaver
:
approval1.9+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
Details | Diff | Splinter Review |
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•17 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•17 years ago
|
Flags: blocking-firefox3?
Comment 2•17 years ago
|
||
(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.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Assignee | ||
Comment 3•17 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 | ||
Comment 4•17 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 | ||
Comment 5•17 years ago
|
||
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)
Comment 6•16 years ago
|
||
itchy fingers. removed gavin by mistake.
Comment 7•16 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•16 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?
Comment 9•16 years ago
|
||
Let's just land the string now without plural support so we've got it in.
Updated•16 years ago
|
Keywords: checkin-needed
Comment 10•16 years ago
|
||
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•16 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. :(
Comment 12•16 years ago
|
||
So, can we still have this with plurals support before beta 4?
Updated•16 years ago
|
Whiteboard: [has patch] → [has patch][needs review rstrong]
Updated•16 years ago
|
Priority: P3 → P4
Assignee | ||
Comment 13•16 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•16 years ago
|
||
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•16 years ago
|
||
This is the browser patch necessary to actually display the notification on startup.
Comment 16•16 years ago
|
||
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-
Updated•16 years ago
|
Whiteboard: [has patch][needs review rstrong]
Assignee | ||
Comment 17•16 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•16 years ago
|
||
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•16 years ago
|
||
Updated to use the pref rather than the API as per the changes above.
Attachment #307726 -
Attachment is obsolete: true
Comment 20•16 years ago
|
||
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 21•16 years ago
|
||
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•16 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.
Comment 23•16 years ago
|
||
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•16 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•16 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.
Comment 26•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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•16 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 30•16 years ago
|
||
Comment on attachment 307952 [details] [diff] [review] patch rev 5 great!
Attachment #307952 -
Flags: review?(robert.bugzilla) → review+
Comment 31•16 years ago
|
||
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•16 years ago
|
||
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)
Comment 33•16 years ago
|
||
--> blocking P2 Also, this has blanket approval for string changes if it lands in time for the freeze.
Priority: P4 → P2
Comment 34•16 years ago
|
||
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•16 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•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5
Updated•16 years ago
|
Attachment #308041 -
Flags: approval1.9+
Comment 37•16 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.
Comment 38•16 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. > 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.
Comment 39•16 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.
Comment 40•16 years ago
|
||
1. install Add-ons 2. restart Fiefox 3. show the Add-ons Manager 4. "Get Add-ons" pane All Buttons show in #commandBarBottom
Comment 41•16 years ago
|
||
(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.
Comment 42•16 years ago
|
||
> 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•16 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•16 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•16 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•16 years ago
|
||
This changes to let different plural forms change the entire string rather than just the one term.
Assignee | ||
Comment 47•16 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•16 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•16 years ago
|
||
This fixes the dialog for windows and linux users.
Assignee | ||
Comment 50•16 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
Comment 51•16 years ago
|
||
(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•16 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.
Comment 53•16 years ago
|
||
(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•16 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•16 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 56•16 years ago
|
||
Comment on attachment 308131 [details] [diff] [review] localisation fix [Checkin: Comment 59] r=mano
Attachment #308131 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 57•16 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•16 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
Comment 60•16 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. >
Comment 61•16 years ago
|
||
(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.
Comment 62•16 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
Updated•16 years ago
|
Attachment #308143 -
Attachment description: bustage fix for windows/linux → bustage fix for windows/linux
[Checkin: Comment 50]
Updated•16 years ago
|
Attachment #308131 -
Attachment description: localisation fix → localisation fix
[Checkin: Comment 59]
Updated•16 years ago
|
Attachment #308041 -
Attachment description: combined patch → combined patch
[Checkin: Comment 36]
Updated•16 years ago
|
Attachment #295842 -
Attachment description: EM patch rev 1 → EM patch rev 1
[(String only) Checkin: Comment 10]
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•