New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible crash on API 21+ at launch when using Holo theme and FormsApplicationActivity #961

Merged
merged 3 commits into from Jun 22, 2017

Conversation

Projects
None yet
6 participants
@jimmgarrido
Collaborator

jimmgarrido commented Jun 2, 2017

Description of Change

The changes made in #631 and #835 were causing an InvalidCastException when using the Holo theme on Lollipop or higher since that theme does not use a Toolbar.

This fixes it by first checking if there is the older ActionBarTextView on all Android versions and if not, then checking for a Toolbar. This also includes a null check for the Toolbar for situations were it is not present, e.g. using a NoActionBar theme.

Bugs Fixed

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
Show outdated Hide outdated Xamarin.Forms.Platform.Android/Platform.cs
@@ -847,29 +847,30 @@ void SetActionBarTextColor()
Color navigationBarTextColor = CurrentNavigationPage == null ? Color.Default : CurrentNavigationPage.BarTextColor;
TextView actionBarTitleTextView = null;
if(Forms.IsLollipopOrNewer)
int actionBarTitleId = _context.Resources.GetIdentifier("action_bar_title", "id", "android");

This comment has been minimized.

@jassmith

jassmith Jun 2, 2017

Member

the only thing I don't like about this change is it changes the preference order of action_bar_title/toolbar usage to be inverse what it was before. It would be good to retain the preference ordering just in case there are any oddball scenarios out there.

@jassmith

jassmith Jun 2, 2017

Member

the only thing I don't like about this change is it changes the preference order of action_bar_title/toolbar usage to be inverse what it was before. It would be good to retain the preference ordering just in case there are any oddball scenarios out there.

@jimmgarrido

This comment has been minimized.

Show comment
Hide comment
@jimmgarrido

jimmgarrido Jun 2, 2017

Collaborator

@jassmith Re-did the fix so it maintains the original preference order 👍

Collaborator

jimmgarrido commented Jun 2, 2017

@jassmith Re-did the fix so it maintains the original preference order 👍

Show outdated Hide outdated Xamarin.Forms.Platform.Android/Platform.cs
}
}
}
}
else
if(actionBarTitleTextView == null)

This comment has been minimized.

@Therzok

Therzok Jun 3, 2017

Member

Nit: if (

@Therzok

Therzok Jun 3, 2017

Member

Nit: if (

Show outdated Hide outdated Xamarin.Forms.Platform.Android/Platform.cs
{
actionBarTitleTextView = (TextView)toolbar.GetChildAt(i);
break;
if (toolbar.GetChildAt(i) is TextView)

This comment has been minimized.

@Therzok

Therzok Jun 3, 2017

Member

GetChildAt(i) is being called twice. Maybe use c#7:
GetChildAt(i) is TextView textView and use that in the inner block.

@Therzok

Therzok Jun 3, 2017

Member

GetChildAt(i) is being called twice. Maybe use c#7:
GetChildAt(i) is TextView textView and use that in the inner block.

This comment has been minimized.

@rmarinho

rmarinho Jun 21, 2017

Member

yeah let's fix this and do only 1 getChild

@rmarinho

rmarinho Jun 21, 2017

Member

yeah let's fix this and do only 1 getChild

Show outdated Hide outdated Xamarin.Forms.Platform.Android/Platform.cs
{
actionBarTitleTextView = (TextView)toolbar.GetChildAt(i);
break;
if (toolbar.GetChildAt(i) is TextView)

This comment has been minimized.

@rmarinho

rmarinho Jun 21, 2017

Member

yeah let's fix this and do only 1 getChild

@rmarinho

rmarinho Jun 21, 2017

Member

yeah let's fix this and do only 1 getChild

Show outdated Hide outdated Xamarin.Forms.Platform.Android/Platform.cs
Toolbar toolbar = (Toolbar)((Activity)_context).FindViewById(actionbarId);
for( int i = 0; i < toolbar.ChildCount; i++ )
var toolbar = (ViewGroup)((Activity)_context).FindViewById(actionbarId);

This comment has been minimized.

@rmarinho

rmarinho Jun 21, 2017

Member

as ViewGroup instead of explicit cast here please..

@rmarinho

rmarinho Jun 21, 2017

Member

as ViewGroup instead of explicit cast here please..

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jun 22, 2017

Member

this is failing @jimmgarrido

Member

rmarinho commented Jun 22, 2017

this is failing @jimmgarrido

@rmarinho

rmarinho requested changes Jun 22, 2017 edited

Build is failing, no c# 7 please :)

@jimmgarrido

This comment has been minimized.

Show comment
Hide comment
@jimmgarrido

jimmgarrido Jun 22, 2017

Collaborator

@rmarinho done 👍

Collaborator

jimmgarrido commented Jun 22, 2017

@rmarinho done 👍

@rmarinho rmarinho merged commit 330b515 into xamarin:master Jun 22, 2017

4 checks passed

Android-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3801, ignored: 10
Details
Windows-Release-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Release Unit Tests : Tests passed: 3801, ignored: 10
Details

rmarinho added a commit that referenced this pull request Jun 22, 2017

Fix possible crash on API 21+ at launch when using Holo theme and For…
…msApplicationActivity (#961)

* Fix possible crash on API 21+ at launch

* Do not use an explicit cast

* Do not use C# 7 pattern matching

assemhakmeh added a commit to assemhakmeh/Xamarin.Forms that referenced this pull request Jul 28, 2017

Merge branch '2.3.5' into ios-fastrenderers
* 2.3.5:
  [UWP] Fixes for usage of XF with .net native toolchain (#1024)
  [UWP] Make sure to update HitTestVisible when IsEnable changes (#1015)
  [Android] Dispose check before setting properties on Button (#1013)
  Add missing member variable to FormsApplicationActivity
  Fix NRE when background color of button set in FormsApplicationActivity (#1010)
  Fix border on android buttons  (#941)
  [iOS] ListView with UnevenRows and Cell Heights will no longer be slow to load (#994)
  Set the Id field for Android Views created by Forms #1004
  Fix build
  Fix possible crash on API 21+ at launch when using Holo theme and FormsApplicationActivity (#961)
  [Android] Remove the ". " on empty labels (Accessibility) on Fastrenderers (#915)
  Remove debug outputs (#1008)
  Add check for instance of UITableView (#885)
  [XamlC] fix release builds of Xaml Unit Tests
  Dispose check on ButtonRenderer (#975)
  [previewer] make sure we do not crash even if the previewer doesn't s… (#946)
  [XamlC] fix build
  Remove VisualElement finalizer (#918)
  [XamlC] process symbols if DebugType is set (#925)

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment