701824 - "Show character encoding" (Text encoding) preference should be hooked up

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. go to the preferences
2. select Show character encoding

Expected: some ui to change the character encoding method
Actual: can't find any

Note:
1. This is a known issue, but I didn't see a bug on it.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

pretty sure there is a bug on this, let's get it duped

Comment 2

[Madhava Enros [:madhava]]

Attachment: Character Encoding UI

The bug talks about the item in prefs -- really, that's a thing to toggle whether the actual feature shows up in the menu. The real feature is the ability to override a page's character encoding -- see the attached UI flow.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: patch

A few notes about the patch:

- The title for x-gbk isn't defined in browser.properties, so I just removed it from the charset options. This encoding didn't show up in XUL Fennec either (since it was caught in an empty try/catch when looking up the title). If we do want x-gbk in the default charset list, let me know and I'll try to figure out how to fix it.

- I added a summary for the the character encoding preference. We don't normally have summaries for booleans, but I wanted to be more clear about what "Show Character Encoding" means. Ideally, the string would be something like "Show Character Encoding in menu", but there isn't enough space.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: screenshot - Show Character Encoding in settings

Screenshot with Show Character Encoding and summary in settings.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: screenshot - Character set list

UI list of available character sets.

Comment 6

[Wesley Johnston (:wesj)]

This seeme like another good place for us to use, test, and expand our built in extension API's if possible. We should able to dynamically add this menu item and generate a menu from a click on it, all in JS.

Comment 7

[Madhava Enros [:madhava]]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Created attachment 589585 [details]
> screenshot - Show Character Encoding in settings
> 
> Screenshot with Show Character Encoding and summary in settings.

What about

"Character Encoding Menu" instead of "Show Character Encoding"

With two values -- "Enabled" and "Disabled"

I think that's a little clearer in setting user expectations...

Comment 8

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

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


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("CharEncoding:Data")) {
>+                JSONObject charsets = message.getJSONObject("charsets");
>+                JSONArray codes = charsets.getJSONArray("codes");
>+                JSONArray titles = charsets.getJSONArray("titles");

I'd like this to be an array of an object, instead of 2 arrays.

>diff --git a/mobile/android/base/GeckoPreferences.java b/mobile/android/base/GeckoPreferences.java

>-    private PreferenceScreen mPreferenceScreen;
>+    private static PreferenceScreen mPreferenceScreen;

Why have we been changing this back and forth? I remember it being static in the past.

I think we want to change this UI to something like the plugin preference UI:

"Character encoding menu" -- two options "Enabled" "Disabled"

Changing the UI will likely change this code.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> var BrowserApp = {

>+  _charsets: { codes: [], titles: [] },

>+    Services.obs.addObserver(this, "CharEncoding:Get", false);
>+    Services.obs.addObserver(this, "CharEncoding:Set", false);

Can we move this out of BrowserApp and make a CharacterEncoding helper?

>+    let showCharEncoding = false;
>+    try {
>+      showCharEncoding = Services.prefs.getComplexValue("browser.menu.showCharacterEncoding", Ci.nsISupportsString).data == "true";
>+    } catch (e) { /* Optional */ }
>+    sendMessageToJava({
>+      gecko: {
>+        type: "CharEncoding:State",
>+        visible: showCharEncoding
>+      }
>+    });

If we have a helper:
CharacterEncoding.sendState() ?

>+  getCharEncoding: function() {

These would both be in CharacterEncoding

>+    let charsets = this._charsets;
>+    if (!charsets.codes.length) {
>+      charsets.codes = Services.prefs.getComplexValue("intl.charsetmenu.browser.static", Ci.nsIPrefLocalizedString)
>+          .data.split(",").map(normalizeCharsetCode);
>+      charsets.titles = charsets.codes.map(getTitle);
>+    }

Could this be a way to make charsets be an array of { code, title } objects ? I don't like have two arrays

>+  setCharEncoding: function(aEncoding) {

These would both be in CharacterEncoding

>+    } else if (aTopic == "CharEncoding:Get") {
>+      this.getCharEncoding();
>+    } else if (aTopic == "CharEncoding:Set") {
>+      this.setCharEncoding(aData);

Call in CharacterEncoding

r- for the UI changes and the JS changes

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Comment 10

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

Comment on attachment 590064 [details] [diff] [review]
patch v2


>+            } else if (event.equals("CharEncoding:Data")) {
>+                JSONArray charsets = message.getJSONArray("charsets");
>+                int selected = message.getInt("selected");
>+
>+                final int len = charsets.length();
>+                final String[] codeArray = new String[len];
>+                final String[] titleArray = new String[len];
>+                for (int i = 0; i < len; i++) {
>+                    JSONObject charset = charsets.getJSONObject(i);
>+                    codeArray[i] = charset.getString("code");
>+                    titleArray[i] = charset.getString("title");
>+                }
>+
>+                final AlertDialog.Builder dialogBuilder = new AlertDialog.Builder(this);
>+                dialogBuilder.setSingleChoiceItems(titleArray, selected, new AlertDialog.OnClickListener() {
>+                    @Override
>+                    public void onClick(DialogInterface dialog, int which) {
>+                        GeckoAppShell.sendEventToGecko(new GeckoEvent("CharEncoding:Set", codeArray[which]));
>+                        dialog.dismiss();
>+                    }
>+                });

You could probably drop the codeArray by reusing the charsets array, but not a big deal

Comment 11

[Brian Nicholson (:bnicholson)]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/735345948d32

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/735345948d32

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 590064 [details] [diff] [review]
patch v2

[Approval Request Comment]
Mobile only. Adds character encoding preference. Low risk.

Comment 14

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

Adds strings too

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 590064 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/673bb97ab111

Comment 17

[Catalin Suciu [:csuciu]]

Verified fixed on Nightly 13.0a1 (2012-02-01)
                  Aurora  11.0a2 (2012-02-01)
Device:Samsung Galaxy S2 (Android 2.3.4)
Now, we have "Character encoding" with two options: "Show Menu"
                                                    "Don't show menu"