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

Status: RESOLVED FIXED

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

Description

[Andrzej Hunt :ahunt]

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 ?

Comment 1

[Andrzej Hunt :ahunt]

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.

Comment 2

[:Margaret Leibovic]

(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.

Comment 3

[Teodora Vermesan (:TeoVermesan)]

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"

Comment 4

[Jim Mathies [:jimm]]

needs an owner

Comment 5

[: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.

Comment 7

[Daniel Lim [:dlim]]

Attachment: Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review commit: https://reviewboard.mozilla.org/r/57436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57436/

Comment 8

[Daniel Lim [: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/

Comment 9

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6593099a58e0

Comment 10

[:Grisha Kruglov]

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 11

[Daniel Lim [:dlim]]

Attachment: Bug 1263726 - Settings title remains as "Language" after switching locale and returning to "General Settings".

Review commit: https://reviewboard.mozilla.org/r/58006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58006/

Comment 12

[Daniel Lim [: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/2-3/

Comment 13

[Daniel Lim [:dlim]]

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/

Comment 14

[Daniel Lim [:dlim]]

(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 15

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8680e694c17d

Comment 16

[Daniel Lim [:dlim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa933381917d

Comment 17

[Daniel Lim [: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/3-4/

Comment 18

[Daniel Lim [: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/4-5/

Comment 19

[Daniel Lim [: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/5-6/

Comment 20

[:Grisha Kruglov]

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bc9db3800f3c
Settings title remains as "Language" after switching locale and returning to "General Settings". r=grisha

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc9db3800f3c

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc9db3800f3c

Comment 24

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 (2016-06-15);
Device: Nexus 5 (Android 6.0.1).

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Daniel, are you going to request to uplift or let it ride the train from 50? Thanks

Comment 26

[Daniel Lim [:dlim]]

Attachment: bug-1263726-fix.patch

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

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8765046 [details] [diff] [review]
bug-1263726-fix.patch

Please uplift to aurora, let's verify it on aurora once it lands.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdd969907fe7

Comment 29

[Sorina Florean [:sflorean]]

Verified as fixed in build 49.0a2 (2016-07-17);
Device: LG G4 (Android 5.1).