Drawing boundaries feature #306

eotin · 2019-06-25T11:37:26Z

No description provided.

- Add MapBox annotations plugins - Drawing Manager encapsulating the Circle/Line/Fill Manager - Smart listeners

Refactor KujakuLayer TODO : - Include KujakuLayer OnClick & OnLongClick in the drawing Manager - Add Unit tests

Refactor DrawingManager and KujakuMapview to encapsulate APIs

Update the Manifest.xml

Fix build sample app

coveralls · 2019-06-25T13:15:31Z

Pull Request Test Coverage Report for Build 1450

336 of 498 (67.47%) changed or added relevant lines in 11 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+12.5%) to 50.512%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
library/src/main/java/io/ona/kujaku/layers/ArrowLineLayer.java	0	1	0.0%
library/src/main/java/io/ona/kujaku/plugin/switcher/layer/BaseLayer.java	1	2	50.0%
library/src/main/java/io/ona/kujaku/manager/KujakuCircleOptions.java	23	25	92.0%
library/src/main/java/io/ona/kujaku/helpers/PermissionsHelper.java	3	11	27.27%
library/src/main/java/io/ona/kujaku/layers/KujakuLayer.java	25	40	62.5%
library/src/main/java/io/ona/kujaku/layers/FillBoundaryLayer.java	16	33	48.48%
library/src/main/java/io/ona/kujaku/helpers/wmts/WmtsHelper.java	31	49	63.27%
library/src/main/java/io/ona/kujaku/views/KujakuMapView.java	6	27	22.22%
library/src/main/java/io/ona/kujaku/layers/BoundaryLayer.java	14	40	35.0%
library/src/main/java/io/ona/kujaku/manager/DrawingManager.java	198	251	78.88%

Files with Coverage Reduction	New Missed Lines	%
library/src/main/java/io/ona/kujaku/views/KujakuMapView.java	3	23.63%

Totals
Change from base Build 1418:	12.5%
Covered Lines:	2613
Relevant Lines:	5173

💛 - Coveralls

Create ShadowCircleLayer class

Isolate KujakuLayer private function from KujakuMapView class

…henPassedFalse' fails now (not locally)

…w class

ekigamba · 2019-07-01T10:55:00Z

build.gradle

    buildToolsVersion = "28.0.3"
-    compileSdkVersion = 27
+    compileSdkVersion = 28


Can you test to make sure the app does not break due to https://developer.android.com/about/versions/pie/android-9.0-changes-28#apache-p and https://developer.android.com/about/versions/pie/android-9.0-changes-28#tls-enabled?

You can also confirm this on the code.

Confirm and test the following also:

https://developer.android.com/about/versions/pie/android-9.0-changes-all#fant-required Which affects the activity that is started by the TrackingService from the background

I confirm that everything is working fine

ekigamba · 2019-07-04T15:22:13Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+
+        Feature feature = Feature.fromGeometry(polygon);
+        FeatureCollection collection = FeatureCollection.fromFeature(feature);
+        FillBoundaryLayer layer = new FillBoundaryLayer.Builder(collection)


So can we reuse the same FillBoundaryLayer. Seems that this creates multiple FillBoundaryLayers

Yes sure, we can create one if it is a new draw, and update the existing one if it is an edition

ekigamba · 2019-07-05T09:29:39Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+
+            kujakuMapView.addLayer(layer);
+        }
+        this.currentLayer = null;


The issue is still evident on the updated code. I am guessing it's because of nullifying this layer

I don't understand what issue is still present ? If we use the drawing mamanger by clicking the start drawing button, we want to draw a new Layer. That is why we create a new one here.
Please advise

If that's the aim of the method, then it's OK

ekigamba · 2019-07-05T09:34:17Z

sample/src/main/java/io/ona/kujaku/sample/activities/DrawingBoundariesActivity.java

+            @Override
+            public void onClick(View view) {
+                // start Drawing from scratch
+                if (! drawingManager.isDrawingEnabled()) {


Check for null drawingManager in case the map was openned without internet connection. The style will not load and the DrawingManager will not be created

ekigamba · 2019-07-05T10:09:39Z

sample/src/main/java/io/ona/kujaku/sample/activities/DrawingBoundariesActivity.java

+                            @Override
+                            public void onCircleClick(@NonNull Circle circle) {
+                                Toast.makeText(DrawingBoundariesActivity.this,
+                                        String.format("Circle clicked"),Toast.LENGTH_SHORT).show();


Remove this call to String.format and add the strings to strings.xml

ekigamba · 2019-07-05T10:10:05Z

sample/src/main/java/io/ona/kujaku/sample/activities/DrawingBoundariesActivity.java

+                            @Override
+                            public void onCircleNotClick(@NonNull LatLng latLng) {
+                                Toast.makeText(DrawingBoundariesActivity.this,
+                                        String.format("Circle NOT clicked"),Toast.LENGTH_SHORT).show();


Remove this call to String.format and add the strings to strings.xml

ekigamba · 2019-07-05T10:10:55Z

sample/src/main/java/io/ona/kujaku/sample/activities/DrawingBoundariesActivity.java

+                            @Override
+                            public void onCircleLongClick(@NonNull Circle circle) {
+                                Toast.makeText(DrawingBoundariesActivity.this,
+                                        String.format("Circle long clicked"),Toast.LENGTH_SHORT).show();


Remove this call to String.format and add the strings to strings.xml

ekigamba · 2019-07-05T10:11:03Z

sample/src/main/java/io/ona/kujaku/sample/activities/DrawingBoundariesActivity.java

+                            @Override
+                            public void onCircleNotLongClick(@NonNull LatLng point) {
+                                Toast.makeText(DrawingBoundariesActivity.this,
+                                        String.format("Circle NOT long clicked"),Toast.LENGTH_SHORT).show();


Remove this call to String.format and add the strings to strings.xml

ekigamba · 2019-07-05T10:15:23Z

library/src/main/java/io/ona/kujaku/views/KujakuMapView.java

@@ -817,6 +702,9 @@ protected VisibleRegion getCurrentBounds() {
    private void enableFeatureClickListenerEmitter(@NonNull MapboxMap mapboxMap) {
        mapboxMap.removeOnMapClickListener(this);
        mapboxMap.addOnMapClickListener(this);
+
+        mapboxMap.removeOnMapLongClickListener(this);
+        mapboxMap.addOnMapLongClickListener(this);


I am not sure why the remove and add ClickListener follow one another. Do you have an idea?

I'm not sure too i took the example with the removeOnMapClickListener above

ekigamba · 2019-07-05T11:18:20Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+            return true;
+        }
+
+        Geometry geometry = kujakuLayer.getFeatureCollection().features().get(0).geometry();


I think you can add a check to make sure features() is not null. The IDE is complaining that there might be an NPE

I don't understand my IDE here ..

features() has been annotated as returning a nullable value. So calling get(0) on a null will introduce an NPE. Check that features is not null and reuse the validated variable to get feature at index 0

As you can see above, I check if features is not null and if the size >= 1.

Cool, then the IDE has not way of figuring that out since you did not use store it in a variable and use the null-checked variable. This is OK

ekigamba · 2019-07-05T11:22:53Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+     * @param kujakuLayer
+     * @return
+     */
+    public boolean startDrawingKujakuLayer(@Nullable KujakuLayer kujakuLayer) {


Do you think we should the general KujakuLayer specifically?

Is this supposed to work with all KujakuLayers or BoundaryLayer or FillBoundaryLayer?

Good question, Do you want to restrict to FillBoundaryLayers only ?

I think you should restrict the drawing, right? I am not sure what it is supposed to work with but we also have BaseLayer which is a KujakuLayer. That layer basically has raster. I guess some KujakuLayers might not have feature collections and would return null when getFeatureCollection is called. Probably restrict it to what you would want it to be used on. What do you think?

Yes, i agree. I will update the code to restrict to the FillBoundaryLayer only

ekigamba · 2019-07-05T13:46:17Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+            }
+        }
+
+        circleManager.update(circle);


I can see that you update the circle here. Should you also update the previousCircle and nextCircle?

the next and previous circle are deleted if they are not null above

ekigamba · 2019-07-08T08:26:31Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+            return true;
+        }
+
+        Geometry geometry = kujakuLayer.getFeatureCollection().features().get(0).geometry();


features() has been annotated as returning a nullable value. So calling get(0) on a null will introduce an NPE. Check that features is not null and reuse the validated variable to get feature at index 0

ekigamba · 2019-07-08T08:29:19Z

library/src/main/java/io/ona/kujaku/manager/DrawingManager.java

+     * @param kujakuLayer
+     * @return
+     */
+    public boolean startDrawingKujakuLayer(@Nullable KujakuLayer kujakuLayer) {


I think you should restrict the drawing, right? I am not sure what it is supposed to work with but we also have BaseLayer which is a KujakuLayer. That layer basically has raster. I guess some KujakuLayers might not have feature collections and would return null when getFeatureCollection is called. Probably restrict it to what you would want it to be used on. What do you think?

Rename startDrawingKujakuLayer to startDrawing that return the Polygon

eotin added 10 commits June 25, 2019 13:19

Add Drawing activity with

ea048c9

- Add MapBox annotations plugins - Drawing Manager encapsulating the Circle/Line/Fill Manager - Smart listeners

Refactoring drawing feature

79db6c1

Refactor Drawing Manager

e0d8af3

Refactor KujakuLayer TODO : - Include KujakuLayer OnClick & OnLongClick in the drawing Manager - Add Unit tests

Refactor Click listeners

6f53f6b

Create DrawingManagerIsNullException class

79ca54c

Refactor DrawingManager and KujakuMapview to encapsulate APIs

Refactor Drawing Manager

9547be5

Update the Manifest.xml

Add FillBoundaryLayerTest class

6586829

Refactor ang begin unit tests

192d66c

Fix Merge from onaio master

215d363

Fix Codacy Review

127b203

ekigamba changed the title ~~Drawing boundaries clean~~ Drawing boundaries feature Jun 25, 2019

eotin added 2 commits June 25, 2019 14:12

Fix Codacy Review

163ac85

Fix build sample app

Update android SDK version in travis.yml

52da0b0

eotin added 10 commits June 25, 2019 17:55

Create DrawingManagerTest class

11c617b

Create ShadowCircleLayer class

Add Drawing manager unit tests

53b0b53

Improve Test coverage

1d9e879

Remove unused import

5c0bb97

Fix issue when running test with relinker version 1.3.1

965e03b

Isolate Wtms functions from KujakuMapView class

b7781af

Isolate Wtms functions from KujakuMapView class

13e3ec2

Isolate Wtms functions from KujakuMapView class

3f205bd

Isolate KujakuLayer private function from KujakuMapView class

Isolate Permissions check from KujakuMapView class

4c9fb12

Revert refactoring as the test 'enableAddPointShouldHideMarkerLayoutW…

1b55eb4

…henPassedFalse' fails now (not locally)

ekigamba self-requested a review July 1, 2019 11:49

eotin added 2 commits July 1, 2019 17:53

Refactor Drawing Manager to not add too much APIs in the kujakuMapVie…

ff78e86

…w class

Fixing codacy checks

7f68839

ekigamba suggested changes Jul 2, 2019

View reviewed changes

eotin added 2 commits July 2, 2019 09:42

Remove DrawingManagerIsNullException exception class

27c039c

Add @nonnull and @nullable annotations

7febe2c

Apply @ephraim changes.diff file

6f4438a

ekigamba suggested changes Jul 4, 2019

View reviewed changes

Update Layer properties instead of create each time a new Layer

bd995c3

ekigamba suggested changes Jul 5, 2019

View reviewed changes

Fix some reviews

3c60e56

ekigamba suggested changes Jul 8, 2019

View reviewed changes

eotin added 4 commits July 8, 2019 16:09

Restrict drawing with FillBoundaryLayer

14895ed

Restrict drawing with FillBoundaryLayer

c473c1e

Rename startDrawingKujakuLayer to startDrawing that return the Polygon

Fix drawing not start in the activity

17dae7c

Delete/update comments referencing KujakuLayer

92499f1

ekigamba approved these changes Jul 10, 2019

View reviewed changes

ekigamba merged commit 2c8c1a4 into onaio:master Jul 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drawing boundaries feature #306

Drawing boundaries feature #306

eotin commented Jun 25, 2019

coveralls commented Jun 25, 2019 •

edited

Loading

ekigamba Jul 1, 2019

eotin Jul 2, 2019

ekigamba Jul 4, 2019

eotin Jul 4, 2019

eotin Jul 5, 2019

ekigamba Jul 5, 2019

eotin Jul 5, 2019

ekigamba Jul 9, 2019

ekigamba Jul 5, 2019

ekigamba Jul 5, 2019

ekigamba Jul 5, 2019

ekigamba Jul 5, 2019

ekigamba Jul 5, 2019

ekigamba Jul 5, 2019

eotin Jul 5, 2019

ekigamba Jul 5, 2019

eotin Jul 5, 2019

ekigamba Jul 8, 2019

eotin Jul 8, 2019

ekigamba Jul 9, 2019

ekigamba Jul 5, 2019

eotin Jul 5, 2019

ekigamba Jul 8, 2019

eotin Jul 8, 2019

ekigamba Jul 5, 2019

eotin Jul 5, 2019

ekigamba Jul 8, 2019

ekigamba Jul 8, 2019

Drawing boundaries feature #306

Drawing boundaries feature #306

Conversation

eotin commented Jun 25, 2019

coveralls commented Jun 25, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1450

💛 - Coveralls

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

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

coveralls commented Jun 25, 2019 •

edited

Loading