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

Convert Android maps to GetMapAsync call #824

Merged
merged 2 commits into from Mar 21, 2017

Conversation

Projects
None yet
7 participants
@jassmith
Member

jassmith commented Mar 20, 2017

Description of Change

Move to GetMapAsync API for better forwards compatibility with map renderer on Android

Bugs Fixed

None

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

@jassmith jassmith changed the title from Getmapasync to Convert Android maps to GetMapAsync call Mar 20, 2017

@@ -81,7 +80,8 @@ protected override void Dispose(bool disposing)
NativeMap.SetOnCameraChangeListener(null);
NativeMap.InfoWindowClick -= MapOnMarkerClick;
NativeMap.Dispose();
}
NativeMap = null;
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

your keyboard is auto-inserting spaces randomly.

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

your keyboard is auto-inserting spaces randomly.

@StephaneDelcroix StephaneDelcroix merged commit 8725484 into master Mar 21, 2017

3 checks passed

Android-UITests-C8 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-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: 3749, ignored: 10
Details

@StephaneDelcroix StephaneDelcroix deleted the getmapasync branch Mar 21, 2017

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

Convert Android maps to GetMapAsync call (#824)
* [A] Move map renderer to calling GetMapAsync

* [A] Make sure Map initializes correctly with Async map fetching
@kentcb

This comment has been minimized.

Show comment
Hide comment
@kentcb

kentcb Mar 28, 2017

Contributor

Just tried this. Whilst it does indeed fix the crash, now none of the standard stuff works, like HasZoomEnabled, HasScrollEnabled, and MoveToRegion. @jassmith Is this intentional? Is it being fixed in two stages?

Contributor

kentcb commented Mar 28, 2017

Just tried this. Whilst it does indeed fix the crash, now none of the standard stuff works, like HasZoomEnabled, HasScrollEnabled, and MoveToRegion. @jassmith Is this intentional? Is it being fixed in two stages?

@kentcb

This comment has been minimized.

Show comment
Hide comment
@kentcb

kentcb Mar 28, 2017

Contributor

Actually, the problem is more subtle. We have a custom map renderer that extends the in-built one. However, we also need to know when the map is ready, so we too implement IOnMapReadyCallback and call GetMapAsync in our OnElementChanged. This seems to prevent the base class OnMapReady from executing. I suspect our renderer is doing the wrong thing and am gonna overhaul it - will report back.

Contributor

kentcb commented Mar 28, 2017

Actually, the problem is more subtle. We have a custom map renderer that extends the in-built one. However, we also need to know when the map is ready, so we too implement IOnMapReadyCallback and call GetMapAsync in our OnElementChanged. This seems to prevent the base class OnMapReady from executing. I suspect our renderer is doing the wrong thing and am gonna overhaul it - will report back.

@kentcb

This comment has been minimized.

Show comment
Hide comment
@kentcb

kentcb Mar 28, 2017

Contributor

Yeah, on closer inspection I think it would be best if we just didn't inherit the built-in renderer at all. It doesn't seem to be designed for extensibility. For example, we need greater control over icons used for the pins, but there is no hook for the generation of MarkOptions. I'm assuming this is why we created a custom renderer in the first place (not sure because I didn't write it).

Will try using the built-in renderer code as a starting point for our own renderer instead.

Contributor

kentcb commented Mar 28, 2017

Yeah, on closer inspection I think it would be best if we just didn't inherit the built-in renderer at all. It doesn't seem to be designed for extensibility. For example, we need greater control over icons used for the pins, but there is no hook for the generation of MarkOptions. I'm assuming this is why we created a custom renderer in the first place (not sure because I didn't write it).

Will try using the built-in renderer code as a starting point for our own renderer instead.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 28, 2017

Member

@kentcb feel free to make a pr to add for example a virtual method so one could override and extend on the custom renderer a marker creation.

Member

rmarinho commented Mar 28, 2017

@kentcb feel free to make a pr to add for example a virtual method so one could override and extend on the custom renderer a marker creation.

@kentcb

This comment has been minimized.

Show comment
Hide comment
@kentcb

kentcb Mar 28, 2017

Contributor

@rmarinho no time right now (release due next week). I might be able to put something together afterwards, if no one else has in the meantime.

Another problem I'd like to see fixed is Pin being sealed makes it impossible to extend in order to add your own app-specific data to it (which the renderer could then pick up and apply).

Contributor

kentcb commented Mar 28, 2017

@rmarinho no time right now (release due next week). I might be able to put something together afterwards, if no one else has in the meantime.

Another problem I'd like to see fixed is Pin being sealed makes it impossible to extend in order to add your own app-specific data to it (which the renderer could then pick up and apply).

@kentcb

This comment has been minimized.

Show comment
Hide comment
@kentcb

kentcb Mar 28, 2017

Contributor

I've managed to get this working for our scenario. Had to resort to reflection to invoke several internal members. As a note to myself, these are the important hooks as I see it:

  • make Pin extensible
  • expose a hook that converts a Pin to a Marker
  • expose a hook for when the map is ready
  • allow consumers to forgo InfoWindowClick and instead use MarkerClick

Also, some bugs I noticed (some may be due to the specifics of our scenario - just want to note for later):

  • an initially set location is not moved to? I had to fix this in OnMapReady
  • initially set pins are not displayed? Again, had to fix in OnMapReady
Contributor

kentcb commented Mar 28, 2017

I've managed to get this working for our scenario. Had to resort to reflection to invoke several internal members. As a note to myself, these are the important hooks as I see it:

  • make Pin extensible
  • expose a hook that converts a Pin to a Marker
  • expose a hook for when the map is ready
  • allow consumers to forgo InfoWindowClick and instead use MarkerClick

Also, some bugs I noticed (some may be due to the specifics of our scenario - just want to note for later):

  • an initially set location is not moved to? I had to fix this in OnMapReady
  • initially set pins are not displayed? Again, had to fix in OnMapReady
@jcmanke

This comment has been minimized.

Show comment
Hide comment
@jcmanke

jcmanke Mar 29, 2017

Contributor

@kentcb I've created a fork to work on this, feel free to contribute when you can.

https://github.com/jcmanke/Xamarin.Forms/tree/map-renderer-extensibility

Also, Evolution forum post and pull request #844

Contributor

jcmanke commented Mar 29, 2017

@kentcb I've created a fork to work on this, feel free to contribute when you can.

https://github.com/jcmanke/Xamarin.Forms/tree/map-renderer-extensibility

Also, Evolution forum post and pull request #844

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

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