Map renderer extensibility #844

Merged
merged 2 commits into from Apr 11, 2017

Conversation

Projects
None yet
7 participants
@jcmanke
Contributor

jcmanke commented Mar 29, 2017

Description of Change

Makes the Xamarin.Forms.Maps projects more extensible, by unsealing the Pin class and adding hooks in the MapRenderer classes for customization of map and pins.

API Changes

Added:

  • protected virtual void MapRenderer.OnMapReady(GoogleMap map) (Android)
  • protected virtual MarkerOptions MapRenderer.CreateMarker(Pin pin) (Android)
  • protected virtual IMKAnnotation MapRenderer.CreateAnnotation(Pin pin) (iOS)
  • Pin.LabelProperty

Changed:

  • IOnMapReadyCallback.OnMapReady(GoogleMap map) calls protected OnMapReady for map initialization
  • public sealed class Pin => public class Pin
  • Pin.Label now uses backing BindableProperty LabelProperty

Behavioral Changes

No behavioral changes to existing functionality, reduces need for workarounds when subclassing MapRenderer.

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
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Mar 29, 2017

@jcmanke,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Mar 29, 2017

@jcmanke,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@dnfclas dnfclas added the cla-signed label Mar 29, 2017

@pauldipietro

This comment has been minimized.

Show comment
Hide comment
@pauldipietro

pauldipietro Mar 29, 2017

Member

I think this should go through the Xamarin.Forms Evolution forum first.

Member

pauldipietro commented Mar 29, 2017

I think this should go through the Xamarin.Forms Evolution forum first.

@jcmanke

This comment has been minimized.

Show comment
Hide comment
@jcmanke

jcmanke Mar 29, 2017

Contributor

Apologies if I'm doing things out-of-order. Idea posted in Evolution forum.

Contributor

jcmanke commented Mar 29, 2017

Apologies if I'm doing things out-of-order. Idea posted in Evolution forum.

@jcmanke jcmanke referenced this pull request Mar 29, 2017

Merged

Convert Android maps to GetMapAsync call #824

3 of 4 tasks complete
@jassmith

Approved pending review of other renderers to make sure unseal doesn't negatively impact them.

@@ -51,6 +51,24 @@ public override SizeRequest GetDesiredSize(int widthConstraint, int heightConstr
{
return new SizeRequest(new Size(Context.ToPixels(40), Context.ToPixels(40)));
}
+
+ public virtual void OnMapReady(GoogleMap map)

This comment has been minimized.

@jassmith

jassmith Mar 29, 2017

Member

Can you please convert this to a public non-virtual method which them calls a protected virtual method?

There are a series of issues with public virtual methods, mostly centering around the fact that you cant sanitize inputs/outputs if issues crop up in the future. By moving to a 2 method solution with a protected virtual we retain the required flexibility.

@jassmith

jassmith Mar 29, 2017

Member

Can you please convert this to a public non-virtual method which them calls a protected virtual method?

There are a series of issues with public virtual methods, mostly centering around the fact that you cant sanitize inputs/outputs if issues crop up in the future. By moving to a 2 method solution with a protected virtual we retain the required flexibility.

This comment has been minimized.

@jassmith

jassmith Mar 29, 2017

Member

In fact now that I think of it, we should leave the explicity implementation of this method, and then have that call the protected virtual method. This reduces the external API surface area while retaining the desired effect.

@jassmith

jassmith Mar 29, 2017

Member

In fact now that I think of it, we should leave the explicity implementation of this method, and then have that call the protected virtual method. This reduces the external API surface area while retaining the desired effect.

This comment has been minimized.

@jcmanke

jcmanke Mar 29, 2017

Contributor

Would you prefer to have the protected virtual method take the GoogleMap parameter, or expect overrides to use the NativeMap property?

@jcmanke

jcmanke Mar 29, 2017

Contributor

Would you prefer to have the protected virtual method take the GoogleMap parameter, or expect overrides to use the NativeMap property?

This comment has been minimized.

@jassmith

jassmith Mar 29, 2017

Member

I much prefer needed data passed to methods rather than fetched from properties. The more idempotent the function, the better.

@jassmith

jassmith Mar 29, 2017

Member

I much prefer needed data passed to methods rather than fetched from properties. The more idempotent the function, the better.

@@ -2,17 +2,15 @@
namespace Xamarin.Forms.Maps
{
- public sealed class Pin : BindableObject
+ public class Pin : BindableObject

This comment has been minimized.

@jassmith

jassmith Mar 29, 2017

Member

For the moment I think I'm okay with unsealing this class. We will need to evaluate the other renderers to make sure there isn't a reason this could go wrong.

@jassmith

jassmith Mar 29, 2017

Member

For the moment I think I'm okay with unsealing this class. We will need to evaluate the other renderers to make sure there isn't a reason this could go wrong.

jcmanke and others added some commits Mar 29, 2017

Added extensibility features to Maps
Android MapRenderer:
- Moved map initialization to protected virtual method OnMapReady, called by explicit implementation of IOnMapReady
-  Added protected virtual method CreateMarker for customization of pins

iOS MapRenderer:
- Added protected virtual method CreateAnnotation for customization of pins

Pin:
- Unsealed the Pin class
- Made Label a bindable property

@rmarinho rmarinho merged commit dfda41b into xamarin:master Apr 11, 2017

3 of 6 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 faile…
Details
iOS8-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8
Details
iOS9-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9
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: 3768, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details

@jcmanke jcmanke deleted the jcmanke:map-renderer-extensibility branch Apr 12, 2017

@jcmanke jcmanke referenced this pull request May 2, 2017

Closed

[iOS Maps] Pin customization in iOS MapRenderer #893

2 of 4 tasks complete
@vrassouli

This comment has been minimized.

Show comment
Hide comment
@vrassouli

vrassouli May 3, 2017

I'm trying to extend the Map and MapRenderer to support custom tile providers.
But when I inherit from MapRenderer, some features don't work. Like MoveToRegion method, or IsShowingUser property.

I'm trying to extend the Map and MapRenderer to support custom tile providers.
But when I inherit from MapRenderer, some features don't work. Like MoveToRegion method, or IsShowingUser property.

@jcmanke jcmanke referenced this pull request Jul 24, 2017

Merged

[iOS Maps] Pin rendering customization #1065

4 of 4 tasks complete

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

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

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