Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Map circles #7401

Merged
merged 6 commits into from Jan 29, 2020
Merged

Map circles #7401

merged 6 commits into from Jan 29, 2020

Conversation

jcmanke
Copy link
Contributor

@jcmanke jcmanke commented Sep 5, 2019

Description of Change

Added a Circle MapElement to maps. On UWP, since there is no native circle overlay in Bing Maps, it is implemented as a 360-sided MapPolygon, borrowing from the Highlighting a Circular Area on a Map sample.

Also added GeographyUtils helper class and Distance.BetweenPoints method.

Issues Resolved

N/A

API Changes

Added:

  • Maps
    • public class Circle : MapElement
    • public static class GeographyUtils
    • public static Distance Distance.BetweenPositions(Position position1, Position position2)
  • Android MapRenderer
    • protected virtual CircleOptions CreateCircleOptions(Circle circle)
    • protected ACircle GetNativeCircle(Circle circle)
    • protected Circle GetFormsCircle(ACircle circle)
  • iOS MapRenderer
    • protected virtual MKCircleRenderer GetViewForCircle(MKCircle mkCircle)
  • UWP MapRenderer
    • protected virtual MapPolygon LoadCircle(Circle circle)

Platforms Affected

  • Maps (all platforms)
  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Use the MapElements page in the control gallery app and select Circle from the picker. Clicking the map will change the radius of the currently selected circle. When adding a new circle, the first click will set the center and the second click will set the radius.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@samhouts samhouts added this to In Review in v4.4.0 Sep 5, 2019
@samhouts samhouts added in-progress This issue has an associated pull request that may resolve it! and removed in-progress This issue has an associated pull request that may resolve it! labels Oct 2, 2019
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Captura de pantalla 2019-10-23 a las 10 24 42

@jcmanke
Copy link
Contributor Author

jcmanke commented Oct 23, 2019

Would it be possible to retarget this PR to 4.3.0 and get it included in a service release? I'm finding it difficult to add a subclass of MapElement into custom renderers in 3rd party code.

@samhouts samhouts removed this from In Review in v4.4.0 Nov 2, 2019
@samhouts samhouts added API-change Heads-up to reviewers that this PR may contain an API change t/enhancement ➕ labels Nov 4, 2019
@samhouts
Copy link
Member

samhouts commented Nov 4, 2019

@jcmanke I'm afraid we don't add new API to service releases, but please rebase on 4.4.0! Thanks!

formsPolyline.PropertyChanged -= MapElementPropertyChanged;
}
}
_polylines?.ForEach(p => p.Remove());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.ForEach() is not performant on Android. Can you find a way to write this with a for loop instead? Thanks!

_polylines?.ForEach(p => p.Remove());
_polylines = null;

_polygons?.ForEach(p => p.Remove());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.ForEach() is not performant on Android. Can you find a way to write this with a for loop instead? Thanks!

_polygons?.ForEach(p => p.Remove());
_polygons = null;

_circles?.ForEach(c => c.Remove());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.ForEach() is not performant on Android. Can you find a way to write this with a for loop instead? Thanks!


protected ACircle GetNativeCircle(Circle circle)
{
return _circles?.Find(c => c.Id == (string)circle.MapElementId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Find() is not performant on Android. Can you find a way to write this with a for loop instead? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny because you told someone else to use .Find() in this file a year ago. #3751 (comment)

I'll switch everything to loops.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is awesome. Yes, I will slowly but surely move all code to for loops. 😂

Xamarin.Forms.Maps/GeographyUtils.cs Show resolved Hide resolved
@samhouts samhouts added this to In Progress in v4.5.0 Nov 5, 2019
@jcmanke jcmanke changed the base branch from master to 4.4.0 November 5, 2019 16:42
@jcmanke jcmanke requested a review from samhouts November 6, 2019 16:20
@samhouts samhouts added this to In Progress in v4.4.0 Nov 8, 2019
@samhouts samhouts removed this from In Progress in v4.5.0 Nov 22, 2019
@jcmanke
Copy link
Contributor Author

jcmanke commented Dec 12, 2019

Looks like this missed the cut for 4.4.0...

@samhouts samhouts changed the base branch from 4.4.0 to master December 17, 2019 00:00
@samhouts samhouts added this to In Progress in v4.5.0 Dec 17, 2019
@samhouts samhouts removed this from In Progress in v4.4.0 Dec 17, 2019
@samhouts samhouts merged commit 3f84ee0 into xamarin:master Jan 29, 2020
v4.5.0 automation moved this from In Progress to Done Jan 29, 2020
@samhouts samhouts added this to Done in v4.6.0 Jan 29, 2020
@samhouts samhouts removed this from Done in v4.5.0 Feb 13, 2020
@samhouts samhouts added this to the 4.6.0 milestone Feb 28, 2020
@Transis-Felipe
Copy link
Contributor

Any plan to develop colorized pins?

@jcmanke jcmanke deleted the map_circles branch May 27, 2020 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
v4.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants