Refactoring Geo-based Activities

[Dvik]

Problem description

This issue is related to the 7 Geo-based activities that we have in our codebase. While working on #491 , @lognaturel suggested that the geo activities require a better hierarchy and i couldn't agree more.

A lot of code like the code for checking location providers, code to return location etc are repeated in most of these activities. I suggest we should have a BaseMapActivity which will have common properties of all these activities. Also, some properties are repeated only in two activities like in GeoPointMapActivity and GeoPointOsmMapActivity. Having a common activity for them would also be a good idea.

We can also look to outsource some of the common logic to a Utils class. Also, the UI related tasks and logic related can be separated partially. Complete separation is only possible if we follow a design pattern like MVP.

Please suggest your views regarding these changes. @jnordling

I'll certainly not make all these changes in a single PR and will stick to small PRs.

[lognaturel]

Your basic refactoring proposal sounds great, @Dvik and I couldn't agree more with approaching this as a series of small PRs! @jnordling wrote a lot of this code and may have some good suggestions or warnings for you as well.

Can you please keep this issue narrowly focused on code structure and file the suggestion to use FusedLocationProviderApi as a separate issue? I think there will be code quality benefits to approaching those as two separate projects.

[Dvik]

@lognaturel Ok, yeah, i'll create a seperate issue for FusedLocationProviderApi .

[lognaturel]

@Dvik I think you're ready to start doing this incrementally if you would like. Perhaps you can do a first PR that sets up your proposed class hierarchy and very conservatively applies extract superclass. That means no other stylistic changes, optimizations, etc yet! I think moving towards MVP would be a good idea.

[Dvik]

@lognaturel i have not made much progress with this because i'm caught up with interviews and preparing for it and will be for the next few weeks.
Would be great if someone takes it up.

[lognaturel]

@Dvik Thanks for letting us know!

[shobhitagarwal1612]

@lognaturel Can I take this issue?

[supreme96]

@shobhitagarwal1612 Hi have you started working on this issue? I think we can both work on this code restructure together. It will get this done more quickly and with much more comprehensive covering of the possible edge cases.

[nribeka]

I'm gonna have a look at #508 and I think that will be related with this one 👍

[jnordling]

Let me know if you all need me on this..

[lognaturel]

I believe @nribeka will touch many of these classes with #508 so probably best to wait until that's in (I guess we've now swapped the order, that's fine too!). Then @shobhitagarwal1612 @supreme96 you can decide whether you want to pair or have one of you do it. If you have any doubts about intended functionality, @jnordling is your guy and you should tag him in any eventual pull requests that come out of this!

[MukundAnanthu]

For easy reference, GeoTraceGoogleMapActivity as G and GeoTraceOsmMapActivity as O). G extends FragmentActivity while O extends Activity. Since, FragmentActivity extends Activity, if we have our base class, GeoActivity extend FragmentActivity, we may be able to do the restructuring as proposed here:
Level 1 --> GeoActivity extends FragmentActivity
Level 2 --> GeoTraceGoogleMapActivity extends GeoActivity and GeoTraceOsmMapActivity extends GeoActivity.

But in the process, we would end up making O, which is actually an Activity, extend a FragmentActivity(So, it’s basically us terming it to be a Fragment when it is actually an activity). Is that okay? What are your thoughts on this?

Another approach(Again, probably get's the job done, but isn't a nice way to do it. I don't like this one either):
Level 1-> GeoActivity extends Activity
Level 2-> O extends GeoActivity
Level 3-> G extends O (and overrides a bunch of it's methods).

But again, even in this approach, we are basically making GoogleMapActivity have an is-a relationship with the OSM Activity.

[lognaturel]

Thanks for your careful analysis, @MukundAnanthu! I think your approach is a good one. I've looked further into this and I would recommend starting with GeoPointMapActivity and GeoPointOsmMapActivity since these are somewhat simpler and they are the "original" ones that the others were created from (here is the source).

What I would tend to do as a first pass is create an abstract GeoPointMapActivity with some methods to override in two concrete subclasses -- GeoPointGoogleMapActivity and GeoPointOsmMapActivity. So it's basically your first suggestion but with the point activities rather than the trace activities. I think it's just an accident of history that one of the trace activities extends Activity and the other extends FragmentActivity and it would be fine to change. But I think starting with the point activities will give you a good sense of what can be further abstracted out.

To give some specific examples, the GeoPointMapActivity abstract class would have a concrete onPause method since that's exactly the same between the two implementing classes. For a more interesting example, upMyLocationOverlayLayers looks like it's probably only accidentally different between the two classes -- one fixes a bug and the other doesn't. I think the version currently in GeoPointMapActivity (the Google one) should probably be a concrete method shared between the two. onCreate is another good example. There's some shared code but then the map setup is different between the two APIs. So you could have a concrete onCreate method that calls a new setupMap method that would be abstract in the GeoPointMapActivity concrete class but then implemented differently between the two concrete classes.

I hope these examples help! If you don't agree with the approach or it's significantly different from what you had in mind, let's talk more about it. And if all this seems obvious -- great! I just want to make sure we're talking about the same thing. As always, comments from everyone are very welcome!

[MukundAnanthu]

@lognaturel Thank you for the lucid explanation. The examples were indeed helpful!! Will issue a separate PR for the above changes as suggested earlier.

[lognaturel]

@MukundAnanthu Great! I can't emphasize enough how important it will be to think about whether differences in activities are deliberate or accidental. For another example that just came up, see #1084. Some of the activities allow for manual point entry by long press and others don't. They all should. In particular, the OSM-based geopoint does while the Google maps-based geopoint doesn't. If you could help us document these differences as you find them, that would be great! And when in doubt, I think it's better to make them share more code when you can. Fun!!

[MukundAnanthu]

@lognaturel Sure! Will keep an eye out for such differences.

[MukundAnanthu]

@lognaturel

These are the fields that were moved up from the GeoPointGoogleMapActivity.java and GeoPointOsmMapActivity.java to the abstract parent class GeoPointMapActivity.java

GoogleMap implementation:(GeoPointGoogleMapActivity.java)

Extracted:
private static final String LOCATION_COUNT = "locationCount";
private TextView locationStatus;
private LocationManager locationManager;
private Location location;
private Button reloadLocation; // refactored to reloadLocationButton
private boolean isDragged = false;
private Button showLocation;// refactored to showLocationButton
private boolean gpsOn = false;
private boolean networkOn = false;
private int locationCount = 0;
private MapHelper helper;
private AlertDialog zoomDialog;
private View zoomDialogView;
private Button zoomPointButton;
private Button zoomLocationButton;
private boolean setClear = false;
private boolean captureLocation = false;
private Boolean foundFirstLocation = false;
private int locationCountNum = 0;
private int locationCountFoundLimit = 1;
private Boolean readOnly = false;
private Boolean draggable = false;
private Boolean intentDraggable = false;
private Boolean locationFromIntent = false;

Not Extracted:
private GoogleMap map;
private MarkerOptions markerOptions;
private Marker marker; // different libs
private LatLng latLng;
private TextView locationInfo;

OsmMap implementation: (GeoPointOsmMapActivity.java)

Extracted:
private static final String LOCATION_COUNT = "locationCount";
private TextView locationStatus;
private LocationManager locationManager;
private Location location;
private Button reloadLocationButton;
private boolean isDragged = false;
private Button showLocationButton;
private boolean gpsOn = false;
private boolean networkOn = false;
private int locationCount = 0;
private MapHelper helper;
private AlertDialog zoomDialog;
private View zoomDialogView;
private Button zoomPointButton;
private Button zoomLocationButton;
private boolean setClear = false;
private boolean captureLocation = false;
private Boolean foundFirstLocation = false;
private int locationCountNum = 0;
private int locationCountFoundLimit = 1;
private Boolean readOnly = false;
private Boolean draggable = false;
private Boolean intentDraggable = false;
private Boolean locationFromIntent = false;

Not Extracted:
private MapView map;
private Marker marker; //different libs
private Handler handler = new Handler();
private GeoPoint latLng;
public MyLocationNewOverlay myLocationOverlay;

Method extraction from the GeoPointGoogleMapActivity.java and GeoPointOsmMapActivity.java to the abstract parent class GeoPointMapActivity.java:

Not Extracted:

1. onCreate(): The setContentView() called inside onCreate() differs
2. returnLocation(): refers to latlng in implementation. latlng belongs to different data types in the 2 classes.
3. setupMap(): It's google map specific. (Takes GoogleMap googleMap as an argument)
4. onLocationChanged(): Internally refers to latlng whose types are different in the 2 classes.
5. onMarkerDrag(): Marker variable has different types in the 2 implementations
6. onMarkerDragEnd(): Marker variable has different types in the 2 implementations
7. onMarkerDragStart(): Marker variable has different types in the 2 implementations
8. onMapLongClick(): Present in only one class as of now. Since it uses the latLng variable that belongs to different types in the 2 implementations, feel it should be kept unextracted
9. zoomToLocation(): Uses latLng in implementation. Made the method abstract in the GeoPointMapActivity parent class.
10. zoomToPoint(): Uses latLng in implementation. Made the method abstract in the GeoPointMapActivity parent class.
11. showZoomDialog(): Internal reference to latLng. Made the method abstract in the GeoPointMapActivity parent class.
12. longPressHelper(): Internal reference to marker, which has different types in the 2 implementations. Used only in the OSM class.
13. singleTapConfirmedHelper(): Used only in OSM class. Uses GeoPoint as parameter. GeoPoint is specific to OSM.
14. onResume(): Internally references map variable whose type is OSM specific. Not present in Google Map class.
15. onSaveInstanceState(): Present only in OSM implementation. We may want to save the instance state in GoogleMap class as well, in which case we can extract it to parent class.

[heyjamesknight]

@opendatakit-bot claim

[heyjamesknight]

Where we are now, we have six map based Activities, the cartesian product of two implementation sets (Google vs. OSM, Point vs. Shape vs. Trace).

• Google:
  • GeoPointMapActivity
  • GeoShapeGoogleMapActivity
  • GeoTraceGoogleMapActivity
• OSM:
  • GeoPointOsmMapActivity
  • GeoShapeOsmMapActivity
  • GeoTraceOsmMapActivity

My current plan is to collapse these into a single GeoActivity, which uses composition to implement the 6 feature sets instead of inheritance. This will make things super easy to reason about and test.

In the process, I'll be introducing the MVVM architecture to the app. We'll make use of the new Android Architecture Components and their View Model class to do this. We'll also make use of DataBinding, although this may not save us too much as the Map views handle most of the work for us.

The plan of attack goes like this:

1. Reimplement GeoPoint using an MVVM architecture and composition for the Point specific functionality.
2. Collapse GeoShape and GeoTrace features into GeoActivity.
3. Collapse the three OSM activities into GeoActivity.

[getodk-bot]

Hello @jknightco, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 10 days.

You can reclaim this issue or claim any other issue by commenting @opendatakit-bot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

[zestyping]

An update: work has been proceeding on this refactor in #2465 (GeoShape), #2553 (GeoTrace), and #2776 (GeoPoint). Each of these three pull requests merges the Google Maps version and the OSM version of an activity together into one activity. The differences in implementation between the Google Map display and the OSM display are encapsulated behind one interface, MapFragment, with two implementations: GoogleMapFragment and OsmMapFragment.

The final result will be three activities: GeoPointMapActivity, GeoTraceActivity, GeoShapeActivity.

[zestyping]

These pull requests, which unify the Geo activities into three, have now been merged and released in 1.20.

There is still common code between the three, which will be further factored into BaseGeoMapActivity.

[zestyping]

@yanokwa I think this issue can be closed now.