[android] - harden offline region creation / update LatLngBounds #8517
Conversation
tobrun
force-pushed
the
tvn-latlngbounds
branch
from
f0b5a12
to
5d092fd
Compare
| @@ -114,6 +116,7 @@ public void run() { | |||
| }).start(); | |||
| } | |||
|
|
|||
| @UiThread | |||
There was a problem hiding this comment.
@tobrun Why are we requiring @UiThread for methods here? I'm thinking of issues if the developer wants to download regions from a background thread, say from a Service.
There was a problem hiding this comment.
I thought this was a requirement as the manager itself must be loaded from main thread afaik, let me fix this up.
There was a problem hiding this comment.
@tobrun Is it currently possible to get the manager instance from the main thread but then call the other methods in a separate thread?
| @@ -34,47 +35,40 @@ public void beforeTest() { | |||
| .build(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testSanity() { | |||
There was a problem hiding this comment.
@tobrun Why is necessary to remove this test?
There was a problem hiding this comment.
A sanity test becomes obsolete when there other tests that indirectly test the same thing (this is a common test driven development practice).
There was a problem hiding this comment.
I always keep those, well, to make sure I keep my sanity ;)
There was a problem hiding this comment.
I'm all down for keeping you sane! Will keep them.
| @@ -194,6 +211,66 @@ public void testOuterUnion() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void northWest() { | |||
There was a problem hiding this comment.
@tobrun For all these new methods, could we add specific tests for cases where latitude and/or longitude are crossing their limits (i.e International Date Line)?
There was a problem hiding this comment.
Not sure if there is a reason for it as it's valid to construct a bounds that is not found inside of the world bounds (eg. no wrapping should be enforced by LatLngBounds). The current LatLngBounds logic now matches what core exposes. FWIW this is also the reason why this PR only validates creation of an offline region instead of enforcing this with LatLngBounds.
There was a problem hiding this comment.
I'm worried about developers using this class outside offline cases. If wrapping is not tested at the core level either, then it's fine to leave it there but I'd make a note in Javadoc.
There was a problem hiding this comment.
added this in class javadoc
tobrun
force-pushed
the
tvn-latlngbounds
branch
2 times, most recently
from
3719e71
to
ff0e27f
Compare
…e world bounds, update tests
tobrun
force-pushed
the
tvn-latlngbounds
branch
from
ff0e27f
to
4daac2d
Compare
Closes #8514
Closes #4321