489229 - Cleanup in Add-ons Manager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 474480 added muchof the Add-on Manager UI, but left some parts out of the initial landing. We should address these:

> Just as a thought, it might be worth just using an nsIRDFObserver on the EM
> datasource in order to keep the local items up to date, as a way of avoiding
> templates but still keeping things in better sync over time. It could also
> detect new add-on installed from content.

I need to talk to Madhava about how to best display newly installed add-ons.
Will revisit this in a followup bug

---------------------------------

> You probably want to get the opType for the item and add that too. isDisabled
> covers a variety of reasons for being disabled, at the very least you want to
> get appDisabled and userDisabled. If the item is appDisabled then the user
> shouldn't be offered the choice to enable (it won't work anyway but it's crappy
> UI). Perhaps for a later revision but you'd want to get blocklisted,
> blocklistedsoft, compatible, providesUpdatesSecurely for the full set of
> states.

OK. I added "appDisabled" and "opType" here too.
I will add the others to a followup bug

---------------------------------

> >+  _installCallback: function em__installCallback(aItem, aStatus) {
>
> >+    else {
> >+      // Sucess
> >+    aItem.setAttribute("opType", "needs-restart");
> 
> Again, get the actual opType I think.

I skipped this for now. We need a better story for the newly installed add-ons

---------------------------------

> >+  confirmInstall: function(aParent, aPackages, aCount) {
> >+    // Return true to get the install started
> >+    return true;
> >+  },
> 
> No confirmation dialog? Shaver would not be happy I think.

Followup bug (as much as I don't want it)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

> > >+  openProgressDialog: function(aPackages, aCount, aManager) {
> > 
> > I think you can basically strip this function and just call back to
> > aManager.openProgressDialog rather than duplicating what it does.
> 
> I can't do that because the aManager is the nsXPInstallManager, which will try
> to open a dialog in ::OpenProgressDialog
> 
> http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsXPInstallManager.cpp#540

It will try to open a dialog unless the dialog is already open, so you could
just set PREF_XPINSTALL_STATUS_DLG_CHROME to something that is already open,
say the Fennec main UI. No big deal really, just saves some code duplication.

Comment 2

[Madhava Enros [:madhava]]

A couple of quick things to polish this up:

For add-ons you have
- show the type "Extension" "Theme" etc. in the upper right-hand corner of the row


In the Get Add-ons Section:

- "Recommended" in the upper right-hand-corner for the ones that we show pre-search
- for search results, show their ratings in the upper right-hand corner
- change text in the search box to "search catalog" from "search all add-ons"

Comment 3

[Madhava Enros [:madhava]]

An additional type to show, for installed add-ons: "Locale"

Comment 4

[Madhava Enros [:madhava]]

Attachment: some text alignment issues

two text alignment issues

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch - alignment issues

The alignment issues are caused by the description labels being outside of a parent container. The add-on title labels are inside a parent box and align correctly.

The buttons in the Get-Addons section are _inside_ a box, but should be outside it.

I am aligning the buttons to maximize the given space (favor the Your Addons style, not Get Add-ons style)

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch - add strings and ratings

This patch adds the Add-on type strings and the rating "stars"

The patch also changes the empty text strings for the add-on and download search boxes. It changes the "Install" string to "Add to [Product]" too.

Lastly, the patch removes a bunch of tooltip strings that were not being used. I checked with the a11y group about removing the tooltips, and they were ok with it.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2 - add strings and ratings

Now with over 50% less XBL

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 396838 [details] [diff] [review]
patch 2 - add strings and ratings

>diff --git a/chrome/content/browser.css b/chrome/content/browser.css

>+richlistitem[typeName="search"] hbox.addon-type-or-rating {
>+  -moz-binding: url("chrome://browser/content/bindings/extensions.xml#extension-search-recommended");
>+}
>+
>+richlistitem[typeName="search"] hbox.addon-type-or-rating[rating] {
>+  -moz-binding: url("chrome://browser/content/bindings/extensions.xml#extension-search-rating");
>+}

use of xbl for this still hurts a little, but I suppose it's reasonable.

>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

>+    this._strings["addonType.4"] = strings.getString("addonType.4");
>+    this._strings["addonType.4"] = strings.getString("addonType.8");

typo: forgot to update the third "4"

>diff --git a/locales/en-US/chrome/browser.dtd b/locales/en-US/chrome/browser.dtd

>-<!ENTITY addonsSearch.emptytext    "Search all add-ons">
>+<!ENTITY addonsSearch.emptytext    "Search Catalog">

>-<!ENTITY addonInstall.label        "Install">
>+<!ENTITY addonInstall.label        "Add to &brandShortName;">

>-<!ENTITY downloadsSearch.emptytext "Search for downloaded file">
>+<!ENTITY downloadsSearch.emptytext "Search">

rev the entity names (don't shoot the messenger)?

Can't you put the wince styles in its browser.css rather than duplicating in -low/-high?

r=me with those addressed.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Updated the entity names where needed :(

pushed:
https://hg.mozilla.org/mobile-browser/rev/9e6d655f6dca
https://hg.mozilla.org/mobile-browser/rev/512e9107b026