1205124 - Make GeckoPreferences extend AppCompatActivity

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Or similar.

As of bug 1201206 we extend Theme.AppCompat and want to use its features but because of this bug, we can't! In particular, we'd like to tint the check box preferences.

Comment 1

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

We should be sure to remove the android: prefixes where applicable (see bug 1215198).

Comment 2

[k.krish]

Can I investigate this bug and try to come up with a patch?

Comment 3

[Andrzej Hunt :ahunt]

(In reply to k.krish from comment #2)
> Can I investigate this bug and try to come up with a patch?

Yes, patches are always welcome! Feel free to post any questions around the build / patch here, I'd also reccomend joining the #mobile IRC channel (that tends to be busiest Mon-Fri)!

Comment 4

[Andrzej Hunt :ahunt]

I discovered the Android SDK ships an example AppCompatPreferenceActivity, I wonder if it's worth just importing that into our codebase and switching GeckoPreferences to extend that?

See:
https://android.googlesource.com/platform/development/+/master/samples/Support7Demos/src/com/example/android/supportv7/app/AppCompatPreferenceActivity.java

Comment 5

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

From the docs you posted:

> /**
> * A {@link android.preference.PreferenceActivity} which implements and proxies the necessary calls
> * to be used with AppCompat.
> *
> * This technique can be used with an {@link android.app.Activity} class, not just
> * {@link android.preference.PreferenceActivity}.
> */

Nice find! It sounds like it could work! I'm slightly concerned that inflation won't be entirely correct, however.

k.krish, are you interested in following the approach Andrzej suggested in comment 4?

Comment 6

[Andrzej Hunt :ahunt]

Fixing this seems to be the simplest correct way to solve Bug 1250900, therefore taking this for myself.

My approach involved importing AppCompatActivity, modifying (un-prefixing) some ActionBar related theme attributes, and modifying the GeckoPreferences ActionBar code to use getSupportActionbar(). This results in the preferences looking identical across Android 4 and newer (whereas previously Android 4 had different theming).

Outstanding issues:
- Dialog has old theming
- Need to verify that this runs on Android >= 5 (we seem to set the prefixed actionbar attributes for v21, which I had to remove in v11 as to not crash on Android 4)

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

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

Comment 8

[Andrzej Hunt :ahunt]

Attachment: appcompat_alertdialog.png

I've found one major issue:
- The Android AlertDialog doesn't work with AppCompat theming (looks broken: additional empty borders, and otherwise no change in appearance - screenshot attached)
- We can replace with the v7 support AlertDialog in some but not all cases: most of our Preferences extend DialogPreference, which assumes the standard AlertDialog, hence we show the broken dialog.

I.e. we need to either:
(A) Have a mix of dialog styles (probably the worst option)
(B) Stick with the old dialog style (but still use AppCompat for the preferences activity)
(C) Implement an AppCompatDialogPreference to replace DialogPreference, in order to have consistent new styling.

I don't see any huge issues with (B).

Comment 9

[Andrzej Hunt :ahunt]

Attachment: appcompat_preferences_olddialog.png

Here's what the dialogs currently look like on Android 4 - this is probably what we have to stick with.

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

Does this look like a sane approach to you? Would you be comfortable with uplifting something like this to fix Bug 1250900?

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #10)
> Does this look like a sane approach to you? Would you be comfortable with
> uplifting something like this to fix Bug 1250900?

Note that uplifting this means you will have to support Gingerbread.

Comment 12

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

https://reviewboard.mozilla.org/r/42873/#review39619

I'd be fine uplifting this approach, provided we do a pretty thorough manual comb-over to make sure nothing changed. Like Sebastian said, we should test on all major design changes (GB, 4.0+, 5.0+, 4.0+ tablet, 5.0+ tablet).

::: mobile/android/base/java/org/mozilla/gecko/preferences/AppCompatPreferenceActivity.java:2
(Diff revision 1)
> +package org.mozilla.gecko.preferences;/*
> + * Copyright (C) 2014 The Android Open Source Project

nit: fmting

::: mobile/android/base/java/org/mozilla/gecko/preferences/AppCompatPreferenceActivity.java:30
(Diff revision 1)
> +import android.support.v7.app.AppCompatDelegate;
> +import android.support.v7.widget.Toolbar;
> +import android.view.MenuInflater;
> +import android.view.View;
> +import android.view.ViewGroup;
> +/**

nit: ws above

::: mobile/android/base/java/org/mozilla/gecko/preferences/AppCompatPreferenceActivity.java:35
(Diff revision 1)
> +/**
> + * A {@link android.preference.PreferenceActivity} which implements and proxies the necessary calls
> + * to be used with AppCompat.
> + *
> + * This technique can be used with an {@link android.app.Activity} class, not just
> + * {@link android.preference.PreferenceActivity}.

Add a comment explaining if we modified anything here.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, something for the future (because not uplift-able): Now that we are on Android 4+ we can use our PreferenceFragment implementation exclusively and do not need a PreferenceActivity anymore. I'll file a bug for that.

Comment 14

[Andrzej Hunt :ahunt]

Attachment: gb.png

I'm still going through the various permutations, perhaps the biggest issue is that we now change the checkboxes on Gingerbread, everything else looks the same as before.

I'm not sure this is necessarily a bad thing (GB default checkboxes look terrible IMHO), and since this will only exist in one release (and we're killing GB anyways), I'm not sure it's worth expending much effort on making it perfect, as long as we can fix the crash.

Here's a screen on GB.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #14)
> Created attachment 8736760 [details]
> gb.png
> 
> I'm still going through the various permutations, perhaps the biggest issue
> is that we now change the checkboxes on Gingerbread, everything else looks
> the same as before.
> 
> I'm not sure this is necessarily a bad thing (GB default checkboxes look
> terrible IMHO), and since this will only exist in one release (and we're
> killing GB anyways), I'm not sure it's worth expending much effort on making
> it perfect, as long as we can fix the crash.
> 
> Here's a screen on GB.

I think this is perfectly fine. We are looking forward to unify the themes with AppCompat and having the new checkboxes for the last GB releases is actually an improvement. ;)

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42873/diff/1-2/

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42873/diff/2-3/

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

https://reviewboard.mozilla.org/r/42873/#review40641

Nice!

Some things to test:
* Phone vs. Tablet layouts (For example Nexus 7 portrait is a special case and doesn't use split pane)
* Jumping from notification to settings (For example data reporting notification)
* Switching between portrait / landscape
* Resume with "Do not keep activities" activated

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:323
(Diff revision 3)
> +        // with an NPE in android.support.v7.app.AppCompatDelegateImplV7.ensureSubDecor(), and it's
> +        // likely other strange devices (other Asus devices, some Samsungs) could do the same.
> +        super.onCreate(savedInstanceState);
> +
> +        // Apply the current user-selected locale, if necessary.
> +        checkLocale();

Does locale switching still work correctly after this has been moved after onCreate()?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:329
(Diff revision 3)
> +
> +        // Track this so we can decide whether to show locale options.
> +        // See also the workaround below for Bug 1015209.
> +        localeSwitchingIsEnabled = BrowserLocaleManager.getInstance().isEnabled();
> +
> +        if (Versions.feature11Plus && onIsMultiPane()) {

Why did you add feature11Plus here? With minSdkVersion set to 15 everything should be 11+.

Comment 19

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/9fb7908df083b09946413585d48d02bba09de934
Bug 1205124 - use AppCompatActivity for GeckoPreferences r=sebastian

Comment 20

[Andrzej Hunt :ahunt]

[Tracking Requested - why for this release]: This fixes Bug 1250900, which is tracking 47.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fb7908df083

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

Approval Request Comment
[Feature/regressing bug #]: Bug 1227325
[User impact if declined]: Cannot set master password on 2.x and 4.x devices. Firefox crashes when selecting the master password preference.
[Describe test coverage new/current, TreeHerder]: Manual testing on 2.3 phone, 4.4 phone and tablet, 5.1 phone, 5.0 tablet and 6.0 tablet.
[Risks and why]: Low risk: theming on 4.x devices now matches 5.0 and newer. (Theming on 2.3 doesn't change with this patch.)
[String/UUID change made/needed]: None.

Note: this patch causes a regression in sync settings theming (old-style theming on 4.x devices, missing titles on >= 5.0 devices), this is fixed in Bug 1264138 which has just landed in fx-team, I'll request uplift on that once it's made it through a nightly or two.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I will approve the uplift on this one together with aurora uplift (when ready) for bug 1264138.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8735597 [details]
MozReview Request: Bug 1205124 - use AppCompatActivity for GeckoPreferences r?sebastian

Regression in setting master password on some devices, Aurora47+

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Teodora, could you please verify this issue is fixed as expected on a latest Nightly build? This caused regression 1264138 and since you verified that I thought I could your your help here as well. Thanks!

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Using 11-04 Nightly build, Firefox crashes when selecting the master password preference using
Alcatel One Touch (Android 4.1.2).
On 21-04 Nightly build, "Create Master Password" dialog is displayed when choosing "user master password" on Moto X (Android 4.4) and Alcatel one touch (Android 4.1.2)

Prompt & Dialog on latest Nightly:

Alcatel One Touch (Android 4.1.2)
http://i.imgur.com/MvvK2uM.png
http://i.imgur.com/ee7ZBrM.png

One plus one (Android 5.1.1)
http://i.imgur.com/JpRzqWR.png
http://i.imgur.com/YWzm2S3.png

HTC Desire HD (Android 5.0.2)
http://i.imgur.com/JpRzqWR.png
http://i.imgur.com/YWzm2S3.png

Samsung Tab S2 (Android  5.0.2)
http://i.imgur.com/ukqjz9Y.png
http://i.imgur.com/VvDhxon.png

Moto X (Android 4.4)
http://i.imgur.com/1oCAnsy.png
http://i.imgur.com/4WMzFi9.png

Nexus 9 (Android 6.0)
http://i.imgur.com/BC0m3Mg.png
http://imgur.com/OduKvjL

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Verified based on comment 28