1434603 - Settings Header not changed when visiting sub-menus

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sorina Florean [:sflorean]]

Attachment: Screenshot_20180131-160855.png

Environment: 
Device: Google Pixel(Android 8.0), HTC 10 (Android 8.0.0);
Build: Nightly 60.0a1 (2018-01-31);

Steps to reproduce:
1. Launch Fennec;
2. Open Settings, and tap on General, Search, Privacy, Advanced sub-menu.

Expected result:
The header is changed accordingly with the menu visited; if we visit General the header name is General not Settings.

Actual result:
The header is changed only for Privacy, Search, Notification Menu.
When General, Accessibility, Advanced menu is opened the header name is still Settings.

Notes: reproducible on Android O

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1434603 - Settings Header not changed when visiting sub-menus on Oreo;

The ActionBar's title will always be updated with the title of the visiting PreferenceScreen.

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8975770 [details]
Bug 1434603 - Settings Header not changed when visiting sub-menus on Oreo;

https://reviewboard.mozilla.org/r/243986/#review250576

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java:97
(Diff revision 1)
> -     * We only return titles for the preference screens that are
> +     * The result will be used to set the title that you see on non-multi-pane devices.
> -     * launched directly, and thus might need to be redisplayed.
> -     *
> -     * This method sets the title that you see on non-multi-pane devices.
>       */
>      private String getTitle() {

It'd be great if we could get this value from the `PreferenceScreen` itself to avoid code duplication but it looks like the precedent is already set to map layout -> string resources so this seems fine.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java:99
(Diff revision 1)
> -     *
> -     * This method sets the title that you see on non-multi-pane devices.
>       */
>      private String getTitle() {
>          final int res = getResource();
>          if (res == R.xml.preferences) {

nit: fwiw, for a set of cascading ifs like this, I'd probably create a hash map with all of these values: it separates the key -> value assignment, making it harder to mess up, and then the logic in this method becomes really simple:
```java
final int stringId = prefIdToTitle.get(res);
return stringId == null ? null : getString(stringId);
```

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java:186
(Diff revision 1)
>  
>      @Override
>      public void onResume() {
>          // This is a little delicate. Ensure that you do nothing prior to
>          // super.onResume that you wouldn't do in onCreate.
> +        // This will also set the title in the parent activity's ActionBar to the title of the current PreferenceScreen

Helpful comments, thanks!

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java:210
(Diff revision 1)
>              getPreferenceScreen().removeAll();
>              addPreferencesFromResource(getResource());
>          }
>  
>          // Fix the parent title regardless.
> -        updateTitle();
> +        updateParentTitle();

Thanks for cleaning up the code!

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8975770 [details]
Bug 1434603 - Settings Header not changed when visiting sub-menus on Oreo;

https://reviewboard.mozilla.org/r/243986/#review250576

> nit: fwiw, for a set of cascading ifs like this, I'd probably create a hash map with all of these values: it separates the key -> value assignment, making it harder to mess up, and then the logic in this method becomes really simple:
> ```java
> final int stringId = prefIdToTitle.get(res);
> return stringId == null ? null : getString(stringId);
> ```

Dropping this issue: I didn't mean to open it as one, just a "this is how I'd do it and maybe you'd think it's cool too!" :)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5ecf6627dfd
Settings Header not changed when visiting sub-menus on Oreo; r=mcomella

Comment 5

[Cosmin Sabou [:CosminS]]

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

Comment 6

[Sorina Florean [:sflorean]]

Verified as fixed on latest Nightly build (62.0a1 - 2018-05-17).
Devices: Google Pixel(Android 8.0), Nexus 6P(Android 8.1.0).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8975770 [details]
Bug 1434603 - Settings Header not changed when visiting sub-menus on Oreo;

Approval Request Comment
[Feature/Bug causing the regression]: Settings Header not changed when visiting sub-menus
[User impact if declined]: The Settings UX would be affected by the fact that accessing some settings would update title in toolbar and accessing others won't.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Has been verified by QE
[List of other uplifts needed for the feature/fix]: bug 1462594
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Minor change, verified by QE
[String changes made/needed]: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975770 [details]
Bug 1434603 - Settings Header not changed when visiting sub-menus on Oreo;

Fixes the settings header not consistently changing when visiting sub-menus, causing confusion. Approved for 61.0b11.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3607d8d3561b

Comment 11

[Levente Sacal]

Verified as fixed in Beta 61.0b11 on Samsung S8+ (Android 8.0.0)