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

Camera Change Listener v2.0 #8644

Merged
merged 0 commits into from
May 12, 2017

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Apr 5, 2017

Closes #6350.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Apr 5, 2017
@tobrun tobrun added this to the android-v5.1.0 milestone Apr 5, 2017
@tobrun tobrun self-assigned this Apr 5, 2017
@tobrun tobrun force-pushed the 6350-camera-notifications branch from 8aea249 to a7a2a39 Compare April 5, 2017 12:10
@tobrun tobrun force-pushed the 6350-camera-notifications branch from a7a2a39 to 328f898 Compare May 8, 2017 12:42
@tobrun
Copy link
Member Author

tobrun commented May 8, 2017

I took another stab at optimizing the onFling integration. This PR is now ready for review.

cc @Guardiola31337 @zugaldia

@tobrun tobrun requested review from zugaldia and Guardiola31337 May 8, 2017 12:44
@tobrun tobrun changed the title WIP - Camera Change Listener v2.0 Camera Change Listener v2.0 May 8, 2017
@tobrun tobrun force-pushed the 6350-camera-notifications branch 2 times, most recently from 55e77a1 to 2a3da10 Compare May 8, 2017 13:16
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

👍


@Override
public void onCameraMoveStarted(int reason) {
if (!idle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer one-liner guard clauses but it's a matter of taste.
IMO a one-liner guard clause makes your intention clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't a preference for one or the other, will fix this up.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, checkstyle doesn't approve this.. [ant:checkstyle] [ERROR] /home/tvn/Mapbox/mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/CameraChangeDispatcher.java:36: 'if' construct must use '{}'s. [NeedBraces]

Feel free to ticket out changing this setting and doing a cleanup of the codebase.

@@ -466,7 +478,7 @@ public boolean onScale(ScaleGestureDetector detector) {
}

// Cancel any animation
transform.cancelTransitions();
//transform.cancelTransitions();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not removing commented code?

@@ -634,7 +647,7 @@ public boolean onShove(ShoveGestureDetector detector) {
}

// Cancel any animation
transform.cancelTransitions();
//transform.cancelTransitions();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

public void onMapChanged(int change) {
Timber.e("CHANGE : %s", change);
if (change == MapView.REGION_DID_CHANGE_ANIMATED) {
mapView.removeOnMapChangedListener(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it order matters 🤔
In setZoom is defined mapView.removeOnMapChangedListener and cameraChangeDispatcher.onCameraIdle second but in moveBy is defined the other way around.

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't change behavior, but for consistency will pick that up.

@tobrun tobrun force-pushed the 6350-camera-notifications branch from 2a3da10 to 6fddb8d Compare May 12, 2017 13:09
@tobrun tobrun changed the base branch from master to release-android-v5.1.0-beta.2 May 12, 2017 13:11
@tobrun tobrun merged this pull request into release-android-v5.1.0-beta.2 May 12, 2017
@tobrun tobrun deleted the 6350-camera-notifications branch May 12, 2017 13:38
@tobrun tobrun restored the 6350-camera-notifications branch May 12, 2017 13:52
@tobrun tobrun mentioned this pull request May 20, 2017
@jfirebaugh jfirebaugh deleted the 6350-camera-notifications branch May 30, 2017 21:24
@tobrun tobrun mentioned this pull request Jul 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants