[android] - zoom to rounded levels #8299

tobrun · 2017-03-07T02:20:56Z

Follow up from #7630 (but now covers all interactions)
Android equivalent of iOS PR in #8027

Besides zooming to rounded zoom levels, this PR also fixes the issue of not getting the final value during OnCameraChange event. Without this change I was receiving zoom levels that just not hit the actual value ( eg. 0.9999988 instead of 1.0).

mention-bot · 2017-03-07T02:20:58Z

@tobrun, thanks for your PR! By analyzing this pull request, we identified @incanus, @jfirebaugh and @tmpsantos to be potential reviewers.

Guardiola31337 · 2017-03-09T19:38:26Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java

@@ -966,8 +966,10 @@ public void run() {
            mapboxMap.onPostMapReady();
          }
        });
+      } else if (change == DID_FINISH_RENDERING_FRAME || change == DID_FINISH_RENDERING_FRAME_FULLY_RENDERED) {


What do you think about decomposing the conditional to make the intention clearer?
We normally end up with long methods and the length of a method is in itself a factor that makes it harder to read (using conditions increase the difficulty even more).
The problem usually lies in the fact that the code tells you what happens but can easily obscure why it happens.
Decomposing the conditional you highlight the condition and make it clearly what you are branching on. You also highlight the reason for the branching.

this is high traffic code, every small performance gain we can get, we should leverage

What do you mean? Do you think that adding new methods could affect the performance?

yes, very much, would love to explore this by profiling and measuring the difference. For now, to get the release out the door, would love to get this PR merged. Feel free to ticket this separately so we don't lose track of it.

Guardiola31337 · 2017-03-09T19:42:37Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java

    trackingSettings.update();
    annotationManager.update();
  }

+  void onUpdateFullyRendered() {
+    CameraPosition cameraPosition = transform.invalidateCameraPosition();


What do you think of moving these lines into a private method?

It increases the chances that other methods can use a method when the method is finely grained.

It allows the higher-level methods to read more like a series of comments (public methods should look like pseudo code, without language statements, etc.).

not sure what you mean, this is as simplistic as a method can be:

void onUpdateFullyRendered() { CameraPosition cameraPosition = transform.invalidateCameraPosition(); if (cameraPosition != null) { uiSettings.update(cameraPosition); }

moving one line of code to another method is overthinking it + this is high traffic code, being able to not have additional method invocations is a win.

I strive to leave public and protected methods as clean as I can. BTW, in this particular case as you pointed out is not that bad so 👍

not sure if I follow this is neither a public or protected method..

Or default. Anyway, looks good to me.

Guardiola31337 · 2017-03-09T19:46:35Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java

@@ -381,14 +381,14 @@ public void setZoom(double zoom) {
    if (isDestroyedOn("setZoom")) {
      return;
    }
-    setZoom(zoom, 0);
+    setZoom(zoom, -1, -1, 0);


What do you think of replacing those magic numbers with a symbolic constant in order to increase readability?
Magic numbers are numbers with special values that usually are not obvious.
If the numbers might ever change, making the change is a nightmare.

let me patch this up

Guardiola31337 · 2017-03-09T19:50:23Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java

    if (isDestroyedOn("setZoom")) {
      return;
    }
-    nativeSetZoom(zoom, duration);
+    nativeSetZoom(zoom, cx == -1 ? cx : cx / pixelRatio, cy == -1 ? cy : cy / pixelRatio, duration);


What about extracting cx == -1 ? cx : cx / pixelRatio, cy == -1 ? cy : cy / pixelRatio into a query or a explaining variable?
It's really hard to read it and therefore to understand what's going on.

let me patch this up

Guardiola31337 · 2017-03-09T19:52:04Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java

-      mapView.scaleBy(2.0, x, y, MapboxConstants.ANIMATION_DURATION);
-    } else {
-      mapView.scaleBy(0.5, x, y, MapboxConstants.ANIMATION_DURATION);
+    CameraPosition cameraPosition = invalidateCameraPosition();


What do you think of extracting this code into a private method?

let me patch this up

Guardiola31337 · 2017-03-09T19:54:37Z

...TestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/DebugModeActivity.java

+        mapboxMap.setOnCameraChangeListener(new MapboxMap.OnCameraChangeListener() {
+          @Override
+          public void onCameraChange(CameraPosition position) {
+            textView.setText(String.format("Zoom : %s", position.zoom));


What do you think of replacing "Zoom : %s" magic number?

Strings aren't numbers AFAIK

Better said, magic numbers are literals, so strings are included ;)

when it comes to readability, it think showing where %s is located for String.format is much clearer else you need to jump to the constant field and check what the value is. Putting it in the constant is making readability worse. The correct solution would be to put this in strings.xml as android studio shows you the value directly in the IDE.

Agreed, but you could always split it into more literals which opens the possibility of explaining your intention clearer.
BTW, as you mentioned putting it in strings.xml is also a great solution.

Guardiola31337

johanlunds · 2017-03-14T18:43:26Z

In Android 5.0.0-beta.3 onCameraChange(CameraPosition position) is continously called. Could it be related to this change?

It does not happen on beta2. It does happen in today's SNAPSHOT.

johanlunds · 2017-03-14T18:57:59Z

I think it has something to do with rendering the map.

When I comment out Layer.setProperties() that we continously call to simulate an animation there are far less onCameraChange calls.

Still, the problem does not go away completely. Since in our onCameraChange callback, we load new data over the network and then call GeoJsonSource.setGeoJson(), the map seems to render again thereby triggering onCameraChange again (and the endless loop continues).

johanlunds · 2017-03-14T19:15:32Z

This temporary workaround seems to work for now:

    private CameraPosition mLastRefreshCameraPosition;

    @Override
    public void onCameraChange(CameraPosition position) {
        if (!position.equals(mLastRefreshCameraPosition)) {
            mLastRefreshCameraPosition = position;
            refreshData();
        }
    }

zugaldia · 2017-03-17T19:27:22Z

@johanlunds Thanks for the report. I want to make sure this doesn't fall through the cracks, could you check if this issue is still happening on 5.0 released today #8452 and if so, open a new ticket about it? Thanks!

tobrun added the Android Mapbox Maps SDK for Android label Mar 7, 2017

tobrun added this to the android-v5.0.0 milestone Mar 7, 2017

tobrun self-assigned this Mar 7, 2017

tobrun requested a review from ivovandongen March 7, 2017 02:20

tobrun mentioned this pull request Mar 7, 2017

[android] - rounded zoom when zooming with zoom button controller #7630

Closed

tobrun force-pushed the tvn-rounded-zoom branch from b20fb53 to 05bafe0 Compare March 7, 2017 02:22

tobrun requested a review from Guardiola31337 March 9, 2017 18:14

Guardiola31337 reviewed Mar 9, 2017

View reviewed changes

tobrun force-pushed the tvn-rounded-zoom branch from 05bafe0 to bf42cff Compare March 10, 2017 01:40

[android] - zoom to rounded levels

4037256

tobrun force-pushed the tvn-rounded-zoom branch from bf42cff to 4037256 Compare March 10, 2017 01:44

Guardiola31337 self-requested a review March 10, 2017 17:35

Guardiola31337 approved these changes Mar 10, 2017

View reviewed changes

zugaldia merged commit ed16a7d into release-ios-v3.5.0-android-v5.0.0 Mar 10, 2017

zugaldia deleted the tvn-rounded-zoom branch March 10, 2017 17:37

tobrun mentioned this pull request May 10, 2017

onMapChange() bug #8800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] - zoom to rounded levels #8299

[android] - zoom to rounded levels #8299

tobrun commented Mar 7, 2017

mention-bot commented Mar 7, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 10, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 10, 2017

Guardiola31337 Mar 10, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 9, 2017

Guardiola31337 Mar 9, 2017

tobrun Mar 10, 2017

Guardiola31337 Mar 10, 2017

Guardiola31337 left a comment

johanlunds commented Mar 14, 2017

johanlunds commented Mar 14, 2017 •

edited

Loading

johanlunds commented Mar 14, 2017

zugaldia commented Mar 17, 2017

[android] - zoom to rounded levels #8299

[android] - zoom to rounded levels #8299

Conversation

tobrun commented Mar 7, 2017

mention-bot commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

johanlunds commented Mar 14, 2017

johanlunds commented Mar 14, 2017 • edited Loading

johanlunds commented Mar 14, 2017

zugaldia commented Mar 17, 2017

johanlunds commented Mar 14, 2017 •

edited

Loading