Closed
Bug 1048418
Opened 10 years ago
Closed 10 years ago
'Paint flashing' localizations need more space for text, gets cropped/truncated
Categories
(Firefox for Android Graveyard :: Settings and Preferences, defect)
Tracking
(fennec33+)
RESOLVED
FIXED
Firefox 34
Tracking | Status | |
---|---|---|
fennec | 33+ | --- |
People
(Reporter: aryx, Assigned: domivinc, Mentored)
References
Details
(Whiteboard: [lang=java][good-first-bug])
Attachments
(1 file, 2 obsolete files)
4.20 KB,
patch
|
liuche
:
review+
|
Details | Diff | Splinter Review |
Firefox for Android Aurora 33.0a2 20140804, Android 4.1.2 on Nexus S
Bug 900692 added a checkbox with the label 'Paint flashing' to the developer settings. Unfortunately, some localizations (e.g. German) are quite longer, e.g.
"Hervorhebung neu gezeichneter Flächen". Firefox OS had to make the developer settings support two lines for the labels.
Please also extend the available space here.
Updated•10 years ago
|
tracking-fennec: --- → ?
Updated•10 years ago
|
Mentor: liuche
Comment 1•10 years ago
|
||
This is probably something as simple as changing some of the xml attributes of the "Paint flashing" TextView so it wraps instead of truncating and adding ellipses.
Updated•10 years ago
|
Whiteboard: [lang=java][good-first-bug]
Updated•10 years ago
|
Assignee: nobody → liuche
tracking-fennec: ? → 33+
Assignee | ||
Comment 2•10 years ago
|
||
Hello,
I started to work on this bug.
1- I have one question regarding the scope of the bug, should we implement a fix only for the “Paint flashing” checkbox or should we implement a fix for all the checkboxes in the settings menu? There are probably other localisations with the same issue?
2- On a device with a trakball or D-pad (tested on my Desire-Z for instance), it works without any issue. When the checkbox is selected using the D-pad, the text scrolls automatically to the left and you can read the end of the text.
3- I found that xml attributes cannot be used in this case. The xml element is “CheckBoxPreference” (http://developer.android.com/reference/android/preference/CheckBoxPreference.html) and there is no solution to change the style in the XML.
But the documentation gives another way to implement the wrap/ellipse properties. In the method onBindView: “This is a good place to grab references to custom Views in the layout and set properties on them.”. I made a test, and that solution works (see attached apk).
Let me know what you want to do.
Regards, Dom
Assignee | ||
Comment 3•10 years ago
|
||
Updated•10 years ago
|
Flags: needinfo?(liuche)
Comment 4•10 years ago
|
||
Hi Dominique, thanks for picking up this bug!
With respect to your questions:
1. Let's do this just for checkboxes that we're seeing problems for - if it does end up being a much more widespread problem, then we can reconsider, but once this bug is fixed, using the solution for other problems will be easy.
2. Good to know - does your patch cause any problems with this kind of device? And we still do want to fix this for other devices because percentage-wise, devices with trackballs are not very common.
3. That's fine then - have you checked that we can't do what we need with setLayoutResource? That would be ideal, but we can use a custom CheckBox if necessary.
Assignee: liuche → domivinc
Flags: needinfo?(liuche)
Comment 5•10 years ago
|
||
Comment on attachment 8473032 [details] [diff] [review]
Proposed patch
Review of attachment 8473032 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch! I think this could be fine, if setLayoutResource doesn't work.
::: mobile/android/base/preferences/CustomCheckBoxPreference.java
@@ +13,5 @@
> +/**
> + * Represents a Checkbox element in a preference menu.
> + * The title of the Checkbox can be larger than the view,
> + * in this case it will be displayed in 2 or more lines.
> + * The default behavior of the class CheckBoxPreference
Nit: trailing whitespace
@@ +35,5 @@
> + protected void onBindView(View view) {
> + super.onBindView(view);
> + TextView title = (TextView) view.findViewById(android.R.id.title);
> + if (title != null) {
> + title.setSingleLine(false);
Nit: trailing ws
::: mobile/android/base/preferences/CustomListPreference.java
@@ +89,5 @@
> public void setIsDefault(boolean isDefault) {
> mIsDefault = isDefault;
> if (isDefault) {
> setOrder(0);
> + setOrder(0);
Duplicate?
Attachment #8473032 -
Flags: feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Thanks for the code review, new patch added with your changes.
About your remarks:
2 - It works also on a device with a D-Pad. The auto scroll of the title is not triggered because all the text is already visible.
3- I didn’t find a nice solution. The setLayoutResource implementation won’t be object oriented code, I need to test the preference keys in class GeckoPreferences for each checkbox concerned by the wrap:
if (PREFS_DEVTOOLS_PAINT_FLASHING.equals(key)) {
pref.setLayoutResource( R.layout.preference_custom_checkbox );
}
and it will duplicate some xml files from the SDK just to set the wrap property of the title. The risk here is to get differences on devices between the checkboxes with the wrap option and the checkboxes without. The effort to add new checkboxes is also bigger, for each checkbox, we have to add a test in GeckoPreferences. When the SDK files will change, we will have to follow the changes in the duplicated files … not good for the code debt!
It’s also probably for all those reasons that the change of the properties in the onBindView method is the only proposal of the Android documentation.
Assignee | ||
Comment 7•10 years ago
|
||
Final version of the patch after code review changes.
Comment 8•10 years ago
|
||
Comment on attachment 8474446 [details] [diff] [review]
patch1048418AfterCodeReview.diff
Review of attachment 8474446 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great when I tried it out! Just two very small nits, that are below.
Also, not related to the code, but part of our patch-writing process, you should match the patch name to the bug title, and append me (liuche) as the reviewer.
Bug 1048418: 'Paint flashing' localizations need more space for text, gets cropped/truncated. r=liuche
Also, on future patches, you should set a r? flag to be the mentor so that person gets a notification to review your patch. You can also do f? for feedback if you're not sure about the approach.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=976e52e6e50b
Thanks, Dominique!
(If you're still looking for more bugs after this, feel free to check http://www.joshmatthews.net/bugsahoy/?mobile=1&unowned=1 or jump into #mobile and ask around!)
::: mobile/android/base/preferences/CustomCheckBoxPreference.java
@@ +12,5 @@
> +
> +/**
> + * Represents a Checkbox element in a preference menu.
> + * The title of the Checkbox can be larger than the view,
> + * in this case it will be displayed in 2 or more lines.
Nit: run-on sentence
@@ +33,5 @@
> +
> + @Override
> + protected void onBindView(View view) {
> + super.onBindView(view);
> + TextView title = (TextView) view.findViewById(android.R.id.title);
Nit: make this final.
Attachment #8474446 -
Flags: feedback+
Assignee | ||
Comment 9•10 years ago
|
||
Last patch (including the 3 last changes: patch title, run-on sentence, final keyword)
Attachment #8473032 -
Attachment is obsolete: true
Attachment #8474446 -
Attachment is obsolete: true
Attachment #8475074 -
Flags: review?(liuche)
Comment 10•10 years ago
|
||
Comment on attachment 8475074 [details] [diff] [review]
patch1048418.diff
Review of attachment 8475074 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! I'll flag this for checkin since there's a green try run.
Attachment #8475074 -
Flags: review?(liuche) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=java][good-first-bug] → [lang=java][good-first-bug][fixed-in-fx-team]
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good-first-bug][fixed-in-fx-team] → [lang=java][good-first-bug]
Target Milestone: --- → Firefox 34
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•