Issue 29341235: Issue 3820 - Selecting Settings within ABB for Android causes the app to crash. (Closed)
|
Created:
May 11, 2016, 2:48 p.m. by diegocarloslima Modified:
Oct. 17, 2016, 3:04 p.m. Reviewers:
René Jeschke CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 3820 - Selecting Settings within ABB for Android causes the app to crash.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Performing changes that were discussed on IRCCloud #MessagesTotal messages: 6
https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... File mobile/android/base/resources/values-v21/themes.xml (right): https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... mobile/android/base/resources/values-v21/themes.xml:27: <item name="colorPrimary">@color/primary_material_dark</item> Is this line really necessary? Also, this item already appears, with fully qualified namespace, at the top of the style definition.
https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... File mobile/android/base/resources/values-v21/themes.xml (right): https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... mobile/android/base/resources/values-v21/themes.xml:27: <item name="colorPrimary">@color/primary_material_dark</item> On 2016/05/17 10:21:13, René Jeschke wrote: > Is this line really necessary? Also, this item already appears, with fully > qualified namespace, at the top of the style definition. This line isn't necessary (in the means of preventing the crash) once we're setting Theme.AppCompat.Light as parent. I added this for sort of a temporary styling fix on the action bar background. Without that, the action bar is light gray with white text and its really hard to read. Even though the fully qualified name and the compatibility name in the end means the same thing (i.e. same color attribute), if we use a theme that doesn't define the compatibility attributes on its hierarchy (i.e. Theme.Material.Light), we're vulnerable to some crashes like this because of weird behaviours of some appcompat stuff. In this particular case, the crash happens when PreferenceActivity gets inflated and tries to read the colorPrimary styling attribute (not the android:colorPrimary one), and since it could find it in the current theme, it throws an exception
On 2016/05/17 12:19:53, diegocarloslima wrote: > https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... > File mobile/android/base/resources/values-v21/themes.xml (right): > > https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... > mobile/android/base/resources/values-v21/themes.xml:27: <item > name="colorPrimary">@color/primary_material_dark</item> > On 2016/05/17 10:21:13, René Jeschke wrote: > > Is this line really necessary? Also, this item already appears, with fully > > qualified namespace, at the top of the style definition. > > This line isn't necessary (in the means of preventing the crash) once we're > setting Theme.AppCompat.Light as parent. I added this for sort of a temporary > styling fix on the action bar background. Without that, the action bar is light > gray with white text and its really hard to read. > > Even though the fully qualified name and the compatibility name in the end means > the same thing (i.e. same color attribute), if we use a theme that doesn't > define the compatibility attributes on its hierarchy (i.e. > Theme.Material.Light), we're vulnerable to some crashes like this because of > weird behaviours of some appcompat stuff. In this particular case, the crash > happens when PreferenceActivity gets inflated and tries to read the colorPrimary > styling attribute (not the android:colorPrimary one), and since it could find it > in the current theme, it throws an exception Okay, but as we have a strict policy for not doing unrelated changes when working on an issue, this second fix should go into a separate issue, as this issue is just about fixing the crash, not a design issue.
On 2016/05/30 14:04:42, René Jeschke wrote: > On 2016/05/17 12:19:53, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... > > File mobile/android/base/resources/values-v21/themes.xml (right): > > > > > https://codereview.adblockplus.org/29341235/diff/29341236/mobile/android/base... > > mobile/android/base/resources/values-v21/themes.xml:27: <item > > name="colorPrimary">@color/primary_material_dark</item> > > On 2016/05/17 10:21:13, René Jeschke wrote: > > > Is this line really necessary? Also, this item already appears, with fully > > > qualified namespace, at the top of the style definition. > > > > This line isn't necessary (in the means of preventing the crash) once we're > > setting Theme.AppCompat.Light as parent. I added this for sort of a temporary > > styling fix on the action bar background. Without that, the action bar is > light > > gray with white text and its really hard to read. > > > > Even though the fully qualified name and the compatibility name in the end > means > > the same thing (i.e. same color attribute), if we use a theme that doesn't > > define the compatibility attributes on its hierarchy (i.e. > > Theme.Material.Light), we're vulnerable to some crashes like this because of > > weird behaviours of some appcompat stuff. In this particular case, the crash > > happens when PreferenceActivity gets inflated and tries to read the > colorPrimary > > styling attribute (not the android:colorPrimary one), and since it could find > it > > in the current theme, it throws an exception > > Okay, but as we have a strict policy for not doing unrelated changes when > working on > an issue, this second fix should go into a separate issue, as this issue is just > about fixing the crash, not a design issue. Could you then please, as discussed on IRC, change the theme here to the one that Mozilla uses in the same revision (FENNEC_42_0_2_RELEASE). Remember that we use a light action bar. If possible, also remove the 'colorPrimary' line then, except Mozilla also uses it, or the crash still occurs. If you happen to see other theme mismatches (which is likely), please open another issue to resolve them.
LGTM. 
|
