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

VERIFIED FIXED in Firefox 11

Status

()

defect
P3
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: nhirata, Assigned: bnicholson)

Tracking

({late-l10n})

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 verified, firefox12 verified, fennec11+)

Details

(Whiteboard: [testday-20111111])

Attachments

(4 attachments, 1 obsolete attachment)

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.
Whiteboard: [testday-20111111]
pretty sure there is a bug on this, let's get it duped
Priority: -- → P5
Whiteboard: [testday-20111111] → [testday-20111111][dupeme]
Priority: P5 → P3
Assignee: nobody → bnicholson
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.
tracking-fennec: --- → 11+
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #589242 - Flags: review?(mark.finkle)
Screenshot with Show Character Encoding and summary in settings.
UI list of available character sets.
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.
(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 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
Attachment #589242 - Flags: review?(mark.finkle) → review-
Posted patch patch v2Splinter Review
Attachment #589242 - Attachment is obsolete: true
Attachment #590064 - Flags: review?(mark.finkle)
Depends on: 719662
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
Attachment #590064 - Flags: review?(mark.finkle) → review+
Whiteboard: [testday-20111111][dupeme] → [testday-20111111]
https://hg.mozilla.org/mozilla-central/rev/735345948d32
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 590064 [details] [diff] [review]
patch v2

[Approval Request Comment]
Mobile only. Adds character encoding preference. Low risk.
Attachment #590064 - Flags: approval-mozilla-aurora?
Adds strings too
Keywords: late-l10n
Comment on attachment 590064 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #590064 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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"
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.