-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactoring Geo-based Activities #507
Comments
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 |
@lognaturel Ok, yeah, i'll create a seperate issue for |
@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. |
@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. |
@Dvik Thanks for letting us know! |
@lognaturel Can I take this issue? |
@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. |
I'm gonna have a look at #508 and I think that will be related with this one 👍 |
Let me know if you all need me on this.. |
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! |
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: 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): But again, even in this approach, we are basically making GoogleMapActivity have an is-a relationship with the OSM Activity. |
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 What I would tend to do as a first pass is create an abstract To give some specific examples, the 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! |
@lognaturel Thank you for the lucid explanation. The examples were indeed helpful!! Will issue a separate PR for the above changes as suggested earlier. |
@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!! |
@lognaturel Sure! Will keep an eye out for such differences. |
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: Not Extracted: OsmMap implementation: (GeoPointOsmMapActivity.java) Extracted: Not Extracted: Method extraction from the GeoPointGoogleMapActivity.java and GeoPointOsmMapActivity.java to the abstract parent class GeoPointMapActivity.java: Not Extracted:
|
@opendatakit-bot claim |
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).
My current plan is to collapse these into a single 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:
|
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 Thanks for your contributions, and hope to see you again soon! |
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. |
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. |
@yanokwa I think this issue can be closed now. |
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 inGeoPointMapActivity
andGeoPointOsmMapActivity
. 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.
The text was updated successfully, but these errors were encountered: