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

UWP MapRenderer NRE and Location fixes #811

Merged
merged 5 commits into from Mar 20, 2017

Conversation

Projects
None yet
6 participants
@erdennis13
Contributor

erdennis13 commented Mar 13, 2017

Description of Change

  1. If UWP MapRenderer is navigated away from, before the user location
    is found, an uncaught NRE will be thrown.
  2. UWP MapRenderer updates are not enforced to be on UI thread
  3. Order of operations in MapRenderer would cause user location to never
    be shown.

Fixes:

  1. Added null checks in UpdateUserIsShowing and LoadUserPosition
    methods.
  2. Called map position updates on UI thread.
  3. Changed order of operations in OnElementChanged to allow for user
    geocoordinates to be shown.

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
Issues:
1. If UWP MapRenderer is navigated away from, before the user location
is found, an uncaught NRE will be thrown.
2. UWP MapRenderer updates are not enforced to be on UI thread
3. Order of operations in MapRenderer would cause user location to never
be shown.

Fixes:
1. Added null checks in UpdateUserIsShowing and LoadUserPosition
methods.
2. Called map position updates on UI thread.
3. Changed order of operations in OnElementChanged to allow for user
geocoordinates to be shown.
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Mar 13, 2017

@erdennis13,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Mar 13, 2017

@erdennis13,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

Show outdated Hide outdated Xamarin.Forms.Maps.UWP/MapRenderer.cs
@@ -62,8 +62,10 @@ public class MapRenderer : ViewRenderer<Map, MapControl>
if (mapModel.Pins.Any())
LoadPins();
await UpdateIsShowingUser();
await Control.Dispatcher.RunIdleAsync(async (i) => await MoveToRegion(mapModel.LastMoveToRegion, MapAnimationKind.None));
if (Control != null)

This comment has been minimized.

@rmarinho

rmarinho Mar 13, 2017

Member

maybe if(Control == null) return;

@rmarinho

rmarinho Mar 13, 2017

Member

maybe if(Control == null) return;

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 13, 2017

Member

Some issues with formatting, please only commit the changes needed.

Member

rmarinho commented Mar 13, 2017

Some issues with formatting, please only commit the changes needed.

@erdennis13

This comment has been minimized.

Show comment
Hide comment
@erdennis13

erdennis13 Mar 13, 2017

Contributor

@rmarinho code formatting issues are resolved

Contributor

erdennis13 commented Mar 13, 2017

@rmarinho code formatting issues are resolved

Show outdated Hide outdated Xamarin.Forms.Maps.UWP/MapRenderer.cs
@@ -256,15 +261,15 @@ void LoadUserPosition(Geocoordinate userCoordinate, bool center)
};
}
if (Control.Children.Contains(_userPositionCircle))
if (Control?.Children?.Contains(_userPositionCircle) == true)

This comment has been minimized.

@rmarinho

rmarinho Mar 14, 2017

Member

maybe it's better in the beginning of the method to do

if (Control == null || Element == null) return; 
@rmarinho

rmarinho Mar 14, 2017

Member

maybe it's better in the beginning of the method to do

if (Control == null || Element == null) return; 

This comment has been minimized.

@erdennis13

erdennis13 Mar 14, 2017

Contributor

Agreed. This is reflected with commit

@erdennis13

erdennis13 Mar 14, 2017

Contributor

Agreed. This is reflected with commit

@@ -235,6 +240,8 @@ void UpdateVisibleRegion()
void LoadUserPosition(Geocoordinate userCoordinate, bool center)
{
if (Control == null || Element == null) return;

This comment has been minimized.

@rmarinho

rmarinho Mar 17, 2017

Member

Sorry reviewing this again, can we do the same we did here on
so we can keep the code as it is there..

UpdateIsShowingUser

@rmarinho

rmarinho Mar 17, 2017

Member

Sorry reviewing this again, can we do the same we did here on
so we can keep the code as it is there..

UpdateIsShowingUser

@@ -176,20 +177,22 @@ void LoadPin(Pin pin)
LoadUserPosition(userPosition.Coordinate, moveToLocation);
}
if (Control == null || Element == null) return;

This comment has been minimized.

@erdennis13

erdennis13 Mar 18, 2017

Contributor

@rmarinho I'm happy to keep the null checks consistent. The reason I included a check on both lines 180 and 167, is because if the user exits the Map page while the geolocater is still operating, the check on line 167 would not be enough.

@erdennis13

erdennis13 Mar 18, 2017

Contributor

@rmarinho I'm happy to keep the null checks consistent. The reason I included a check on both lines 180 and 167, is because if the user exits the Map page while the geolocater is still operating, the check on line 167 would not be enough.

@jassmith jassmith merged commit 5ca7259 into xamarin:master Mar 20, 2017

2 checks passed

OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3744, ignored: 10
Details
@@ -62,9 +62,11 @@ public class MapRenderer : ViewRenderer<Map, MapControl>
if (mapModel.Pins.Any())
LoadPins();
await UpdateIsShowingUser();
if (Control == null) return;
await Control.Dispatcher.RunIdleAsync(async (i) => await MoveToRegion(mapModel.LastMoveToRegion, MapAnimationKind.None));

This comment has been minimized.

@BladeMF

BladeMF Jun 23, 2017

Guys, LastMoveToRegion can be null and it isn't checked anywhere here.

@BladeMF

BladeMF Jun 23, 2017

Guys, LastMoveToRegion can be null and it isn't checked anywhere here.

@@ -174,6 +177,8 @@ void LoadPin(Pin pin)
LoadUserPosition(userPosition.Coordinate, moveToLocation);

This comment has been minimized.

@BladeMF

BladeMF Jun 23, 2017

The above code (using myGeolocator) crashes if the location is not enabled. LocationStatus returns PositionStatus.NotInitialized.

@BladeMF

BladeMF Jun 23, 2017

The above code (using myGeolocator) crashes if the location is not enabled. LocationStatus returns PositionStatus.NotInitialized.

@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 Jul 3, 2018

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