Closed Bug 1263726 Opened 4 years ago Closed 4 years ago

Settings title remains as "Language" after switching locale and returning to "General Settings"

Categories

(Firefox for Android :: Settings and Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fix-optional
firefox49 --- verified
fennec 48+ ---
firefox50 --- verified

People

(Reporter: ahunt, Assigned: dlim)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

1. Open Settings
2. Go to General->Language
3. Select a new Language
   Note: title in the ActionBar switches to equivalent of "Language" in the new Locale
4. Press back (either in AB, or the system back button)
5. Title remains "Language" (or translated equivalent) but should be "General" (we're showing General settings correctly, just the title is wrong)

Could be a regression caused by Bug 1221679 ?
I've tested on both an N4 and an N7 running nightly. Both of these use the phone-layout (single-pane) settings. I haven't tested on a real (multi-pane) tablet yet.
(In reply to Andrzej Hunt :ahunt from comment #0)

> Could be a regression caused by Bug 1221679 ?

Could very well be. This code is really tricky and difficult to work with.
tracking-fennec: --- → ?
regression:
good build: 11-11-2015
bad build: 12-11-2015

pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc473fe5dc512c450634506f68cbacfb40a06a23&tochange=3cc3b1968524248450c465c4ea2ee5596ffa65f2

Bug 1221679 - Changing language on phone will update language settings page title with "General"
needs an owner
Flags: needinfo?(margaret.leibovic)
I caused this, so I'll look into it. However, I don't think this is too big of a problem, so it's okay that we're shipping this regression.
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 48+
Flags: needinfo?(margaret.leibovic)
Duplicate of this bug: 1274889
Assignee: margaret.leibovic → dlim
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57436/diff/1-2/
Attachment #8759449 - Flags: review?(gkruglov)
https://reviewboard.mozilla.org/r/57436/#review54482

Good first pass!

Check out the try results - there are some lint/checkstyle issues, and tests seem to be failing. I'll give it another look once those are resolved.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:285
(Diff revision 2)
>              if (onIsMultiPane()) {
>                  updateActionBarTitle(R.string.settings_title);
>              }
>  
>              // Update the title to for the preference pane that we're currently showing.
> -            setTitle(R.string.pref_category_language);
> +            int titleId = getIntent().getExtras().getInt(PreferenceActivity.EXTRA_SHOW_FRAGMENT_TITLE);

final whenever possible

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:289
(Diff revision 2)
>              // Update the title to for the preference pane that we're currently showing.
> -            setTitle(R.string.pref_category_language);
> +            int titleId = getIntent().getExtras().getInt(PreferenceActivity.EXTRA_SHOW_FRAGMENT_TITLE);
> +            if (titleId != NO_SUCH_ID) {
> +                setTitle(titleId);
> +            } else {
> +                Log.e(LOGTAG, "Title id not found in intent bundle extras");

Should this ever happen? Perhaps we want to throw, to avoid getting into a weird state?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/SettingsComponent.java:1
(Diff revision 2)
> +package org.mozilla.gecko.tests.components;

license

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/SettingsComponent.java:9
(Diff revision 2)
> +
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.tests.UITestContext;
> +
> +/**
> + * Created by root on 6/2/16.

proper comment

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPreference.java:1
(Diff revision 2)
> +package org.mozilla.gecko.tests;

Missing license. Use Public Domain; look at other tests in the tree, e.g. https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/android/VisitsHelperTest.java#1

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPreference.java:13
(Diff revision 2)
> +import org.mozilla.gecko.tests.helpers.GeckoHelper;
> +
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertEquals;
> +
> +/**
> + * Created by root on 6/2/16.

Please provide a proper comment describing what this is testing.
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57436/diff/2-3/
Comment on attachment 8760437 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58006/diff/1-2/
Attachment #8760437 - Attachment description: Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings" → Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".
Attachment #8760437 - Flags: review?(gkruglov)
(In reply to Daniel Lim [:dlim] from comment #13)
> Comment on attachment 8760437 [details]
> Bug 1263726 - Settings title remains as "Language" after switching locale
> and returning to "General Settings".
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/58006/diff/1-2/

Removed robocop test due to try-server not being able to do multilocale build resulting in test failure.
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57436/diff/3-4/
Attachment #8760437 - Attachment is obsolete: true
Attachment #8760437 - Flags: review?(gkruglov)
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57436/diff/4-5/
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57436/diff/5-6/
Attachment #8759449 - Flags: review?(gkruglov) → review+
Comment on attachment 8759449 [details]
Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

https://reviewboard.mozilla.org/r/57436/#review54962

Nice, thanks dlim! Feel free to add "checkin-needed" keyword.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/UITest.java
(Diff revision 6)
>  
>  package org.mozilla.gecko.tests;
>  
>  import org.mozilla.gecko.Actions;
>  import org.mozilla.gecko.Assert;
> -import org.mozilla.gecko.BrowserApp;

nit: for future reference, changes such as these ideally should be in a Pre or Post commit; No need to change this now, just an FYI.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/bc9db3800f3c
Settings title remains as "Language" after switching locale and returning to "General Settings". r=grisha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc9db3800f3c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified as fixed in build 50.0a1 (2016-06-15);
Device: Nexus 5 (Android 6.0.1).
Daniel, are you going to request to uplift or let it ride the train from 50? Thanks
Flags: needinfo?(dlim)
Approval Request Comment
[Feature/regressing bug #]: 1263726
[User impact if declined]: Settings title will be incorrect after switching locale and returning to "General Settings"
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa933381917d
[Risks and why]: Low risk, settings title will be affected, it's read-only and doesn't affect content
[String/UUID change made/needed]: None
Flags: needinfo?(dlim)
Attachment #8765046 - Flags: approval-mozilla-aurora?
Comment on attachment 8765046 [details] [diff] [review]
bug-1263726-fix.patch

Please uplift to aurora, let's verify it on aurora once it lands.
Attachment #8765046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build 49.0a2 (2016-07-17);
Device: LG G4 (Android 5.1).
You need to log in before you can comment on or make changes to this bug.