[android] - Refactor dependencies, introduce focused components #7189

tobrun · 2016-11-25T13:17:17Z

WIP to refactor all gesture related code into a separate wrapper.

extract code from MapView and make it work
minimize internal dependencies on GestureDectector
remove MapView related methods, talk to NativeMapView directly.

update: intent has changed from initial creation see below

mention-bot · 2016-11-25T13:17:21Z

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

ivovandongen · 2016-11-29T14:22:46Z

@tobrun There are indeed a lot of inter-dependecies. And also some 2-way dependencies. It might be helpful for the clarity of the architecture to abstract some things a little bit more. For example,

The MapGestureDetector has a dependency on MapView, which actually is only passed to the ZoomButtonsController as far as I see which only expects a View, not a concrete class like MapView.
In case of the MapboxMap dependency, it's actually the Transform, Projection and UISettings you're after.
The NativeMapView dependency is needed for 2 things; to get the pixelRatio/pitch and to control and to effect the gestures on the Map. You might want to abstract the second repsonsibility in an Interface.

There might be a couple of places where interfaces/choosing the highest applicable abstraction might help to clear up the dependencies a bit.

tobrun · 2016-11-30T14:33:05Z

The MapGestureDetector has a dependency on MapView, which actually is only passed to the ZoomButtonsController as far as I see which only expects a View, not a concrete class like MapView.

Thinking about this again, the zoombutton controller is more like a widget shown on top of the map, I moved this back to MapView with the other widgets. These will be extracted as part of #6570.

In case of the MapboxMap dependency, it's actually the Transform, Projection and UISettings you're after.

I tried to limit the amount of arguments but not assigning mapbox to field in the MapGestureDetector. I have now changed this to take each dependency seperatly.

The NativeMapView dependency is needed for 2 things; to get the pixelRatio/pitch and to control and to effect the gestures on the Map. You might want to abstract the second repsonsibility in an Interface.

I'm now working to remove the NativeMapView in total, the second responsibility should be made a part of the transform class since they transform the map.

Thanks for some initial 👀, will ping again for second round.

tobrun · 2016-12-02T11:56:28Z

The goal of this PR has diverged from it's initial intent, changes made cascaded in changes elsewhere. This PR is now more focused on a general refactor and taking apart the big dependencies into smaller single focused dependencies. This PR adds a lot of todo to the code but current state is acceptable to merge. (we are only missing the snapshot feature, the jni code needs to be refactored and can be picked up with #6238). Focus of Review would be MapboxMap, MapGestureDetector, MapKeyListener, Transform, TrackingSettings and UiSettings. To make the refactor possible I had to remove some unit tests.

ivovandongen

Great work @tobrun! Cleans things up nicely. Just a few minor details.

If at all possible, it would be nice to re-implement the removed unit tests on top of the new abstractions.

ivovandongen · 2016-12-02T12:14:50Z

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/MarkerViewManager.java

-    public MarkerViewManager(@NonNull MapboxMap mapboxMap, @NonNull MapView mapView) {
-        this.mapboxMap = mapboxMap;
+    public MarkerViewManager(@NonNull ViewGroup container) {
+        this.markerViewContainer = container;
        this.markerViewAdapters = new ArrayList<>();


The initializations that have no dependencies might as well be done in the field declaration. That also enables us to make them final and maybe skip some null checks here and there.

ivovandongen · 2016-12-02T12:15:32Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java

-        this.iconManager = iconManager;
-        this.infoWindowManager = manager;
+        this.iconManager = new IconManager(nativeMapView);
+        this.infoWindowManager = new InfoWindowManager();


Initialize in field declaration

ivovandongen · 2016-12-02T12:16:08Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java

        if (view != null) {
            // null checking needed for unit tests
            view.addOnMapChangedListener(this);
        }
    }

+    // TODO refactor MapboxMap out for Projection and Transform


Let's note these things on #6912 as well. I'm planning to pick that up soon.

#6912 (comment)

ivovandongen · 2016-12-02T12:17:08Z

...m/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java

+     */
+    boolean onGenericMotionEvent(MotionEvent event) {
+        // Mouse events
+        //if (event.isFromSource(InputDevice.SOURCE_CLASS_POINTER)) { // this is not available before API 18


Stray comment?

it seems not.. after that line it indicates: // this is not available before API 18, so it's there waiting for us to bump the minSDK requirments? IDK, this code is from before my time.

ivovandongen · 2016-12-02T12:22:31Z

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

        if (isInEditMode()) {
            // if we are in an editor mode we show an image of a map
            LayoutInflater.from(context).inflate(R.layout.mapbox_mapview_preview, this);
            return;
        }

+        // TODO distill into singular purpose methods/classes
        initialLoad = true;
        onMapReadyCallbackList = new ArrayList<>();


Initialize in declaration

tobrun · 2016-12-02T12:50:00Z

I have worked in the requested changes and working now on getting tests back up and running where possible. BTW biggest issue I had with this was not being able to mock final classes, looking into upgrading to mockito 2 for that.

tobrun · 2016-12-02T14:18:38Z

I got the important tests back to run by using mockito 2 configuration for mocking final classes. Though I wasn't able to bring the MapboxMap test since they rely on NativeMapView (which relies on loading the native library). I'm thinking of porting those tests to an instrumentation test equivalent. I think we are covered for now. @ivovandongen can you do another review round?

ivovandongen · 2016-12-05T09:39:13Z

I wasn't able to bring the MapboxMap test since they rely on NativeMapView (which relies on loading the native library). I'm thinking of porting those tests to an instrumentation test equivalent. I think we are covered for now.

Let's open a follow-up ticket for that then.

ivovandongen · 2016-12-05T09:40:59Z

...android/MapboxGLAndroidSDKTestApp/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.java

@@ -31,512 +34,482 @@
 import static org.mockito.Mockito.when;

 public class MapboxMapTest {
-


Why not just use @ignore on the class for now? Or does that make it crash. Makes it more visible that we need to do some work here.

tobrun · 2016-12-05T16:33:52Z

@ivovandongen I have actually now have ported all the unit MapboxMapTest to an instrumentation equivalent. Can you review?

update: need to revisit this implementation since they occasionally fail. The reason seems related to the issue we have had with add/remove source/layer test.

tobrun · 2016-12-06T09:45:18Z

I'm running the Android job multiple times on Bitrise to ensure we aren't introducing any flaky tests.

update: seeing flaky tests, going to revisit the implementation.

tobrun · 2016-12-06T10:24:38Z

Not seeing any immediate fixes, going to ignore the MapboxMapTest and create a follow up ticket.

tobrun added Android Mapbox Maps SDK for Android refactor ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 25, 2016

tobrun added this to the android-v5.0.0 milestone Nov 25, 2016

tobrun self-assigned this Nov 25, 2016

tobrun force-pushed the tobrun-refactor-gestures branch from a9bf943 to 667affe Compare November 28, 2016 14:17

tobrun force-pushed the tobrun-refactor-gestures branch 2 times, most recently from 0858b86 to 09dd4b8 Compare December 2, 2016 11:52

tobrun changed the title ~~[android] - introduce GestureDetector wrapper~~ [android] - Refactor dependencies, introduce focused components Dec 2, 2016

tobrun removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Dec 2, 2016

ivovandongen suggested changes Dec 2, 2016

View reviewed changes

tobrun mentioned this pull request Dec 2, 2016

Convert annotations API to use peer model #6912

Closed

tobrun force-pushed the tobrun-refactor-gestures branch from 09dd4b8 to a3c048f Compare December 2, 2016 14:14

This was referenced Dec 2, 2016

Expose Map::getStyleUrl #7275

Closed

Inject MapboxMap with NativeMapView #4100

Closed

Encapsulate gesture and tracking mode behaviour #6557

Closed

tobrun force-pushed the tobrun-refactor-gestures branch from a3c048f to 477ebd3 Compare December 2, 2016 15:02

cammace approved these changes Dec 2, 2016

View reviewed changes

ivovandongen reviewed Dec 5, 2016

View reviewed changes

tobrun force-pushed the tobrun-refactor-gestures branch from f215d6f to 33b04c7 Compare December 5, 2016 16:43

[android] - refactor dependencies

bcfcd3f

tobrun force-pushed the tobrun-refactor-gestures branch from 76fff1a to bcfcd3f Compare December 6, 2016 08:17

ignore tests

66a132a

ivovandongen approved these changes Dec 6, 2016

View reviewed changes

tobrun merged commit 9a9e797 into master Dec 6, 2016

tobrun deleted the tobrun-refactor-gestures branch December 6, 2016 13:05

This was referenced Dec 12, 2016

Reimplement Snapshot API Callback #7384

Closed

MapboxAccountManager refactor, removal deprecated accesstoken #7462

Merged

tobrun mentioned this pull request Dec 20, 2016

[android] - update components with latest camera values #7499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] - Refactor dependencies, introduce focused components #7189

[android] - Refactor dependencies, introduce focused components #7189

tobrun commented Nov 25, 2016 •

edited

Loading

mention-bot commented Nov 25, 2016

ivovandongen commented Nov 29, 2016

tobrun commented Nov 30, 2016

tobrun commented Dec 2, 2016

ivovandongen left a comment

ivovandongen Dec 2, 2016

tobrun Dec 2, 2016

ivovandongen Dec 2, 2016

tobrun Dec 2, 2016

ivovandongen Dec 2, 2016

tobrun Dec 2, 2016

ivovandongen Dec 2, 2016

tobrun Dec 2, 2016

ivovandongen Dec 2, 2016

tobrun Dec 2, 2016

tobrun commented Dec 2, 2016 •

edited

Loading

tobrun commented Dec 2, 2016

ivovandongen commented Dec 5, 2016

ivovandongen Dec 5, 2016

tobrun commented Dec 5, 2016 •

edited

Loading

tobrun commented Dec 6, 2016 •

edited

Loading

tobrun commented Dec 6, 2016

		@@ -31,512 +34,482 @@
		import static org.mockito.Mockito.when;

		public class MapboxMapTest {

[android] - Refactor dependencies, introduce focused components #7189

[android] - Refactor dependencies, introduce focused components #7189

Conversation

tobrun commented Nov 25, 2016 • edited Loading

mention-bot commented Nov 25, 2016

ivovandongen commented Nov 29, 2016

tobrun commented Nov 30, 2016

tobrun commented Dec 2, 2016

ivovandongen left a 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

tobrun commented Dec 2, 2016 • edited Loading

tobrun commented Dec 2, 2016

ivovandongen commented Dec 5, 2016

Choose a reason for hiding this comment

tobrun commented Dec 5, 2016 • edited Loading

tobrun commented Dec 6, 2016 • edited Loading

tobrun commented Dec 6, 2016

tobrun commented Nov 25, 2016 •

edited

Loading

tobrun commented Dec 2, 2016 •

edited

Loading

tobrun commented Dec 5, 2016 •

edited

Loading

tobrun commented Dec 6, 2016 •

edited

Loading