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

[android] - move camera logic to dedicated transform class #6919

Merged
merged 2 commits into from
Nov 23, 2016

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Nov 4, 2016

WIP - closes #6532 - This PR moves camera logic to a dedicated java class.

Todo

After this we can look into integrating #4746

Review @ivovandongen.

@tobrun tobrun added Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 4, 2016
@tobrun tobrun added this to the android-v5.0.0 milestone Nov 4, 2016
@tobrun tobrun self-assigned this Nov 4, 2016
@mention-bot
Copy link

@tobrun, thanks for your PR! By analyzing the history of the files in this pull request, we identified @clydebarrow, @cammace and @vkurchatkin to be potential reviewers.

Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

Looks great. Only some JavaDoc on the new class/methods

@tobrun
Copy link
Member Author

tobrun commented Nov 7, 2016

Been looking into remove/decouple Mapbox/MapView dependencies from class while the class members were removed, I will not be able to exclude MapboxMap in total. Primary reason behind this is the usage of CameraUpdateFactory. This class takes in a MapboxMap and uses the following items from that class:

  • CameraPosition
  • UiSettings
  • TrackingSettings
  • Projection
  • minZoom
  • maxZoom

In a normal case, we would refactor those items into constructor and replace references. For this case, I'm thinking about just leaving MapboxMap and have these dependencies resolved from that object instead. The relation between CameraUpdateFactoy and MapboxMap is a bit too complex to extract and simplify.

@tobrun
Copy link
Member Author

tobrun commented Nov 7, 2016

Side note: I had to remove the unit tests related to camera. This is because we aren't able to mock out NativeMapView in the newly introduced Transform class. I recently wrote instrumentation tests on our camera API #6803, these will cover both the old unit tested implementation as the core interaction behind it.

@tobrun tobrun force-pushed the 6532-transform-refactor branch 2 times, most recently from aa8fc81 to 9cced7f Compare November 7, 2016 13:56
@tobrun tobrun added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 7, 2016
@tobrun tobrun force-pushed the 6532-transform-refactor branch 2 times, most recently from f4fdb76 to 5336ebb Compare November 9, 2016 14:05
@tobrun
Copy link
Member Author

tobrun commented Nov 10, 2016

2 more changes have been introduced:

  • wrap camera update around a runnable, this is needed to be able to call an new CameraUpdate from a onFinish callback from another CameraUpdate. What currently occurs is that the second animation is started but is automatically canceled because the other animation wasn't fully finished.
  • simplify the internal workings on transfrom: using 1 callback instead of 2, only one method for each nativecall.

@tobrun tobrun force-pushed the 6532-transform-refactor branch 2 times, most recently from 7c1c939 to 508973b Compare November 21, 2016 11:36
nativeMapView.addOnMapChangedListener(new MapView.OnMapChangedListener() {
@Override
public void onMapChanged(@MapView.MapChange int change) {
if (change == MapView.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.

We probably should batch this so that we do only one invalidate per render instead of one per marker added

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with 07ae553

@tobrun tobrun force-pushed the 6532-transform-refactor branch 2 times, most recently from 9e96312 to 2330870 Compare November 22, 2016 15:15
post camera updates to the message queue, this makes calling an new camera update in the on finish of another update possible. Simplify transform.java implementation.
@tobrun tobrun merged commit edb487b into master Nov 23, 2016
@tobrun tobrun deleted the 6532-transform-refactor branch November 23, 2016 09:53
onCameraChangeListener.onCameraChange(this.cameraPosition);
}
public final void moveCamera(final CameraUpdate update, final MapboxMap.CancelableCallback callback) {
mapView.post(new Runnable() {
Copy link

Choose a reason for hiding this comment

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

@tobrun this breaks the existing contract that the new camera position will be query-able immediately and might have weird consequences in regards to "when this event occurs relative to activity lifecycle" because of the way the main thread works (lifecycle events are posted as items in the main thread handler).

Do you have a recommended strategy then for monitoring when this camera move event has been completed?

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.

Transform.java refactor
5 participants