Closed Bug 1434603 Opened 2 years ago Closed 2 years ago

Settings Header not changed when visiting sub-menus

Categories

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

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: sflorean, Assigned: petru)

References

(Blocks 1 open bug)

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(2 files)

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
Assignee: nobody → petru.lingurar
Whiteboard: --do_not_change--[priority:high]
Attachment #8975770 - Flags: review?(sdaswani) → review?(michael.l.comella)
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!
Attachment #8975770 - Flags: review?(michael.l.comella) → review+
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!" :)
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5ecf6627dfd
Settings Header not changed when visiting sub-menus on Oreo; r=mcomella
https://hg.mozilla.org/mozilla-central/rev/e5ecf6627dfd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify+
Verified as fixed on latest Nightly build (62.0a1 - 2018-05-17).
Devices: Google Pixel(Android 8.0), Nexus 6P(Android 8.1.0).
Flags: qe-verify+
Depends on: 1462594
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(petru.lingurar)
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
Flags: needinfo?(petru.lingurar)
Attachment #8975770 - Flags: approval-mozilla-beta?
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.
Attachment #8975770 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed in Beta 61.0b11 on Samsung S8+ (Android 8.0.0)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.