553143 - Make it easier to view all add-ons (recommended and search results)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: patch

The built-in add-on manager will show small subsets of the total set of recommended add-ons and search results. Currently, we only show at most 5 results.

We should make it easier for the user to view all recommended add-ons and search results.

This patch adds support for special "rows" in the result list that open new tabs with AMO content. In the recommended add-ons case, the AMO page is the mobile frontpage. In the search result case, the mobile AMO page is loaded with the search terms.

We might tweak the style a bit, but the functionality is in place.

Screen shots of the UI:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-browseall.png
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-showallsearch.png

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 433242 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings/extensions.xml b/chrome/content/bindings/extensions.xml
>--- a/chrome/content/bindings/extensions.xml
>+++ b/chrome/content/bindings/extensions.xml
>@@ -220,9 +220,23 @@
>+  <binding id="extension-search-showmore" extends="chrome://browser/content/bindings.xml#richlistitem">
>+    <content orient="vertical">
>+      <xul:hbox align="start">
>+        <xul:image width="32" height="32" xbl:inherits="src=image"/>

Can we do that with CSS?

>+  
>+        let url = gPrefService.getCharPref("extensions.getAddons.search.browseURL");
>+        url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));
>+        url = formatter.formatURL(url);
>+        showmore.setAttribute("url", url);

let url = formatter.formatURLPref("extensions.getAddons.search.browseURL", "");
url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));


>+      let showmore = document.createElement("richlistitem");
>+      showmore.setAttribute("typeName", "showmore");
>+      showmore.setAttribute("image", "chrome://browser/skin/images/addons-32.png");

Put that into css too

>+
>+      let url = gPrefService.getCharPref("extensions.getAddons.browseAddons");
>+      url = formatter.formatURL(url);

nsURLFormatter.formatURLPref?


>diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties
>--- a/locales/en-US/chrome/browser.properties
>+++ b/locales/en-US/chrome/browser.properties
>@@ -3,16 +3,20 @@ addonsLocalNone.label=No add-ons install
> addonsSearchStart.label=Searching for add-onsâ¦

Character at the end looks strange, i know that it is an UTF-8 char but i'm not if I'm supposed to see it well or not in the bugzilla text editor? 



r+ with comments adressed

Comment 2

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

(In reply to comment #1)
> (From update of attachment 433242 [details] [diff] [review])
> >+    <content orient="vertical">
> >+      <xul:hbox align="start">
> >+        <xul:image width="32" height="32" xbl:inherits="src=image"/>
> 
> Can we do that with CSS?

I guess we could

> >+        let url = gPrefService.getCharPref("extensions.getAddons.search.browseURL");
> >+        url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));
> >+        url = formatter.formatURL(url);
> >+        showmore.setAttribute("url", url);
> 
> let url = formatter.formatURLPref("extensions.getAddons.search.browseURL", "");
> url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));

Changed

> >+      let showmore = document.createElement("richlistitem");
> >+      showmore.setAttribute("typeName", "showmore");
> >+      showmore.setAttribute("image", "chrome://browser/skin/images/addons-32.png");
> 
> Put that into css too

Well, I wanted the code to be able to use different images. And we don't have known ID or class name here. I guess we could make this not be flexible with images for now.

> >+      let url = gPrefService.getCharPref("extensions.getAddons.browseAddons");
> >+      url = formatter.formatURL(url);
> 
> nsURLFormatter.formatURLPref?

Yeah

> > addonsSearchStart.label=Searching for add-onsâ¦
> 
> Character at the end looks strange, i know that it is an UTF-8 char but i'm not
> if I'm supposed to see it well or not in the bugzilla text editor? 

It's a UTF-8 char that is fine in the properties file.

Comment 3

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

(In reply to comment #1)
> (From update of attachment 433242 [details] [diff] [review])
> >+        let url = gPrefService.getCharPref("extensions.getAddons.search.browseURL");
> >+        url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));
> >+        url = formatter.formatURL(url);
> >+        showmore.setAttribute("url", url);
> 
> let url = formatter.formatURLPref("extensions.getAddons.search.browseURL", "");
> url = url.replace(/%TERMS%/g, encodeURIComponent(this.searchBox.value));

formatURLPref will throw an error for the %TERMS% part since it is not recognized by URLFormatter. I am leaving it as it was.

> >+      let url = gPrefService.getCharPref("extensions.getAddons.browseAddons");
> >+      url = formatter.formatURL(url);
> 
> nsURLFormatter.formatURLPref?

This one is ok to change.

Comment 4

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/fb6942f27953

Comment 5

[Vlado Valastiak [:wladow]]

Comment on attachment 433242 [details] [diff] [review]
patch


>+addonsSearchMore.label=Show all %S results
>+addonsSearchMore.message=If these %S results aren't what you're looking for, try this

this definetely needs a pluralForm.

Comment 6

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

Attachment: patch for plurals

Changes to use plural forms for strings

Comment 7

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

pushed plural changes:
http://hg.mozilla.org/mobile-browser/rev/8d5e3793e250

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2.2pre) Gecko/20100319
Namoroka/3.6.2pre Fennec/1.1a1

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre)
Gecko/20100319 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a3pre) Gecko/20100319
Namoroka/3.7a3pre Fennec/1.1a2pre


litmus testcases added/updated to regression test this feature:

https://litmus.mozilla.org/show_test.cgi?id=7191
https://litmus.mozilla.org/show_test.cgi?id=7166
https://litmus.mozilla.org/show_test.cgi?id=11624
https://litmus.mozilla.org/show_test.cgi?id=11625
https://litmus.mozilla.org/show_test.cgi?id=9693

Comment 9

[Francesco Lodolo [:flod]]

(In reply to comment #6)
> Changes to use plural forms for strings

Please don't change strings without changing their names, even with a few hours gap between the two commits: not all l10n tools are able to catch this kind of change (an obviously I was lucky enough to translate the first version...).

Not sure what it's best here, maybe a simple warning of dev.l10n is enough.