Fix null when region null #347
Conversation
dothanhbinh41
commented
Aug 16, 2018
•
dothanhbinh41
commented
- When MapRegion is null, would throw an exception in Method "UpdateMapRegion()"
Dev -> Master
Dev -> Master
- check null when update map region - update package for android/shared
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <Import Project="..\..\..\packages\Xamarin.Build.Download.0.4.11\build\Xamarin.Build.Download.props" Condition="Exists('..\..\..\packages\Xamarin.Build.Download.0.4.11\build\Xamarin.Build.Download.props')" /> | |||
There was a problem hiding this comment.
was this upgrade necessary to solve the null issue?
There was a problem hiding this comment.
ok man, i will rollback to old package! but i think package too old, we should update them 👍
| @@ -1,9 +1,9 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
| <uses-sdk android:minSdkVersion="15" /> | |||
| <uses-sdk android:minSdkVersion="15" android:targetSdkVersion="27" /> | |||
There was a problem hiding this comment.
we should be able to do without the target SDK version
| @@ -17,6 +17,14 @@ public MapSpan(Position center, double latitudeDegrees, double longitudeDegrees) | |||
| LongitudeDegrees = Math.Min(Math.Max(longitudeDegrees, MinimumRangeDegrees), 180.0); | |||
| } | |||
|
|
|||
| public MapSpan(Position ne, Position sw) | |||
There was a problem hiding this comment.
this might be best as a separate proposal. I've thought about having this same thing before and I think there are potential pitfalls with it.
There was a problem hiding this comment.
Why native code Region/Span been limit by some point but in TK Region/Span been limit by Center and Radius. What difference with Region and Map Center + Zoom Level ?
There was a problem hiding this comment.
the issue is that it's really easy to pass in parameters that won't behave as you would want.
-
how does it behave if the distance between N + S is 200 Meters and the distance between E + W is 200 Kilometers?
-
how does it behave if someone passes in nw + se by accident?
Essentially if you pick a single point and then a radius, it'll be far less prone to error.
There was a problem hiding this comment.
- NS EW or Radius Center ... i think they are not different, they are in a rectangle.
There was a problem hiding this comment.
plug this in and tell me what happens
// this IS a valid rectangle that fits the scenario above
var ne = new Position {
Latitude = 47.34,
Longitude = -88.97
};
var sw = new Position {
Latitude = 47.33,
Longitude = -121.31
};// this is an INVALID rectangle based on the scenario above
var ne = new Position {
Latitude = 47.33,
Longitude = -88.97
};
var sw = new Position {
Latitude = 47.34,
Longitude = -121.31There was a problem hiding this comment.
Your example like Center and Radius = 0 or radius so tiny
There was a problem hiding this comment.
yep, but as a user I might expect it to round up to the radius of East/West. The point is that the behavior is subject interpretation. With Center/Radius there's no subjectivity.
| <PackageReference Include="Newtonsoft.Json" Version="10.0.3" /> | ||
| <PackageReference Include="Xamarin.Forms" Version="2.5.0.91635" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="11.0.2" /> | ||
| <PackageReference Include="Xamarin.Forms" Version="3.2.0.729530-pre2" /> |
There was a problem hiding this comment.
definitely don't want to be depending on a pre-release version here.
|
overall I'm seeing a number of changes here that while value added, they're beyond the scope of the bug. Are you able to PR the bug fix here and open an issue regarding the dependency updates? |
|
when call method UpdateMapRegion() in TKCustomMapRenderer (android), FormsMap.MapRegion.Equals( We should check null it and this way by me |
|
Did you Rebase off of master? there was a closed PR earlier with a similar solution. |
|
Could you close this PR, i would create new PR :D that's the simplest way |
