Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] - zoom to rounded levels #8299

Merged
merged 1 commit into from
Mar 10, 2017
Merged

[android] - zoom to rounded levels #8299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ public void onSurfaceTextureUpdated(SurfaceTexture surface) {
if (destroyed) {
return;
}
mapboxMap.onUpdate();
mapboxMap.onUpdateRegionChange();
}
}

Expand Down Expand Up @@ -969,8 +969,10 @@ public void run() {
mapboxMap.onPostMapReady();
}
});
} else if (change == DID_FINISH_RENDERING_FRAME || change == DID_FINISH_RENDERING_FRAME_FULLY_RENDERED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

mapboxMap.onUpdateFullyRendered();
} else if (change == REGION_IS_CHANGING || change == REGION_DID_CHANGE || change == DID_FINISH_LOADING_MAP) {
mapboxMap.onUpdate();
mapboxMap.onUpdateRegionChange();
}
}

Expand Down
12 changes: 8 additions & 4 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,18 @@ void onPostMapReady() {
/**
* Called when the user
*/
void onUpdate() {
CameraPosition cameraPosition = transform.invalidateCameraPosition();
uiSettings.update(cameraPosition);
// FIXME introduce update method with camera position
void onUpdateRegionChange() {
trackingSettings.update();
annotationManager.update();
}

void onUpdateFullyRendered() {
CameraPosition cameraPosition = transform.invalidateCameraPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Or default. Anyway, looks good to me.

if (cameraPosition != null) {
uiSettings.update(cameraPosition);
}
}

// Style

/**
Expand Down
19 changes: 15 additions & 4 deletions ...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ final class NativeMapView {
// Listener invoked to return a bitmap of the map
private MapboxMap.SnapshotReadyCallback snapshotReadyCallback;

private static final int CENTER_XY_VALUE = -1;

//
// Static methods
//
Expand Down Expand Up @@ -382,14 +384,23 @@ public void setZoom(double zoom) {
if (isDestroyedOn("setZoom")) {
return;
}
setZoom(zoom, 0);
setZoom(zoom, CENTER_XY_VALUE, CENTER_XY_VALUE, 0);
}

public void setZoom(double zoom, long duration) {
public void setZoom(double zoom, double cx, double cy, long duration) {
if (isDestroyedOn("setZoom")) {
return;
}
nativeSetZoom(zoom, duration);

if (cx != CENTER_XY_VALUE) {
cx = cx / pixelRatio;
}

if (cy != CENTER_XY_VALUE) {
cy = cy / pixelRatio;
}

nativeSetZoom(zoom, cx, cy, duration);
}

public double getZoom() {
Expand Down Expand Up @@ -1019,7 +1030,7 @@ private native void nativeInitialize(NativeMapView nativeMapView, FileSource fil

private native double nativeGetScale();

private native void nativeSetZoom(double zoom, long duration);
private native void nativeSetZoom(double zoom, double cx, double cy, long duration);

private native double nativeGetZoom();

Expand Down
12 changes: 8 additions & 4 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,17 @@ void zoom(boolean zoomIn, float x, float y) {
// Cancel any animation
cancelTransitions();

if (zoomIn) {
mapView.scaleBy(2.0, x, y, MapboxConstants.ANIMATION_DURATION);
} else {
mapView.scaleBy(0.5, x, y, MapboxConstants.ANIMATION_DURATION);
CameraPosition cameraPosition = invalidateCameraPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

let me patch this up

if (cameraPosition != null) {
zoom(cameraPosition, zoomIn, x, y);
}
}

private void zoom(@NonNull CameraPosition cameraPosition, boolean zoomIn, float x, float y) {
int newZoom = (int) Math.round(cameraPosition.zoom + (zoomIn ? 1 : -1));
mapView.setZoom(newZoom, x, y, MapboxConstants.ANIMATION_DURATION);
}

void setZoom(double zoom) {
mapView.setZoom(zoom);
}
Expand Down
14 changes: 14 additions & 0 deletions ...tApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/DebugModeActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
import android.support.design.widget.FloatingActionButton;
import android.support.v7.app.AppCompatActivity;
import android.view.View;
import android.widget.TextView;

import com.mapbox.mapboxsdk.camera.CameraPosition;
import com.mapbox.mapboxsdk.constants.Style;
import com.mapbox.mapboxsdk.maps.MapView;
import com.mapbox.mapboxsdk.maps.MapboxMap;
Expand Down Expand Up @@ -47,9 +49,21 @@ protected void onCreate(Bundle savedInstanceState) {
@Override
public void onMapReady(@NonNull MapboxMap map) {
mapboxMap = map;

mapboxMap.getUiSettings().setZoomControlsEnabled(true);

// show current zoom level on screen
final TextView textView = (TextView) findViewById(R.id.textZoom);
mapboxMap.setOnCameraChangeListener(new MapboxMap.OnCameraChangeListener() {
@Override
public void onCameraChange(CameraPosition position) {
textView.setText(String.format(getString(R.string.debug_zoom), position.zoom));
}
});
}
});


FloatingActionButton fabDebug = (FloatingActionButton) findViewById(R.id.fabDebug);
fabDebug.setOnClickListener(new View.OnClickListener() {
@Override
Expand Down
13 changes: 12 additions & 1 deletion platform/android/MapboxGLAndroidSDKTestApp/src/main/res/layout/activity_debug_mode.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,18 @@
<com.mapbox.mapboxsdk.maps.MapView
android:id="@+id/mapView"
android:layout_width="match_parent"
android:layout_height="match_parent"/>
android:layout_height="match_parent"
app:mapbox_uiAttribution="false"
app:mapbox_uiCompass="false"
app:mapbox_uiLogo="false"/>

<TextView
android:id="@+id/textZoom"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="bottom|start"
android:layout_margin="8dp"
android:textSize="14sp"/>

<android.support.design.widget.FloatingActionButton
android:id="@+id/fabDebug"
Expand Down
1 change: 1 addition & 0 deletions platform/android/MapboxGLAndroidSDKTestApp/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,4 +160,5 @@
<string name="dynamic_marker_chelsea_snippet">Stamford Bridge</string>
<string name="dynamic_marker_arsenal_title">Arsenal</string>
<string name="dynamic_marker_arsenal_snippet">Emirates Stadium</string>
<string name="debug_zoom">Zoom: %s</string>
</resources>
4 changes: 2 additions & 2 deletions platform/android/src/native_map_view.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,8 @@ jni::jdouble NativeMapView::getScale(jni::JNIEnv&) {
return map->getScale();
}

void NativeMapView::setZoom(jni::JNIEnv&, jni::jdouble zoom, jni::jlong duration) {
map->setZoom(zoom, mbgl::AnimationOptions{mbgl::Milliseconds(duration)});
void NativeMapView::setZoom(jni::JNIEnv&, jni::jdouble zoom, jni::jdouble x, jni::jdouble y, jni::jlong duration) {
map->setZoom(zoom, mbgl::ScreenCoordinate{x,y}, mbgl::AnimationOptions{mbgl::Milliseconds(duration)});
}

jni::jdouble NativeMapView::getZoom(jni::JNIEnv&) {
Expand Down
2 changes: 1 addition & 1 deletion platform/android/src/native_map_view.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class NativeMapView : public View, public Backend {

jni::jdouble getScale(jni::JNIEnv&);

void setZoom(jni::JNIEnv&, jni::jdouble, jni::jlong);
void setZoom(jni::JNIEnv&, jni::jdouble, jni::jdouble, jni::jdouble, jni::jlong);

jni::jdouble getZoom(jni::JNIEnv&);

Expand Down