-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Completely overhaul app backend (legacy code) #1092
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
Comments
I have long been thinking of suggesting a similar issue for the UI. Though the recent overhaul had made some significant improvements, I still thinks it would be better to completely overhaul the UI as I could still see some dominance of legacy UI even after some overhaul. As an motivator for why I think of a complete overhaul, consider the login screen. It was recently modified. The new design definitely seems better than the old one except for one thing the dynamism. The login page (username and password input fields) seems to shifting itself to remain in the center with respect to the available screen height. This movement doesn't seem to be a big deal if it only happens when the users keyboard pops in and pops out. In some cases the user's keyboard might suggest things as he types. These suggestions pop in and pop out above the keyboard itself as and when required. This results in the login page moving when the user types his username. This seems to be disturbing to the user as he notices something moving on his screen which generally doesn't happen in case of login screens. I find this to be a consequence of trying to revamp over the legacy UI. This is why I suggest a complete UI overhaul. I guess that comes to a "rewrite the app completely" to some extent :) |
Several big pieces of the deeper parts of the app we have today -
If we drop the sync adapter altogether I believe we sacrifice too much - people count on being able to take photos and have them update to the Commons server in the background. The sync adapter needs the bound service but not the @maskaravivek suggested in #1043 ("Introduce Room DB abstraction layer to our app") that we might want to look at a new abstraction to get data from the database. If we found a way to avoid the content provider (also worth noting - there's unused code in the content providers for handling bulk inserts that's not used in 2 of the 3 content providers. We should delete the dead code whatever else we do.) |
I think it's pretty much universally accepted that separating view, data model and business logic is a good thing these days. The exact way to do it is debated - you've probably heard people talking about
Full disclosure - I've been a believer in the passive view architecture pattern for a long time across a number of different tech stacks. I am currently challenging my own personal liking for MVP based on the excellent work in Google's architecture components - https://developer.android.com/topic/libraries/architecture/index.html - which lean toward MVVM with the acknowledgement that you might want to encapsulate logic in presenter classes if things get hairy doing it in the view model. The discipline of the "clean architecture" is appealing in an academic sense but in a production system it can feel a little "fussy", "complex" and/or "busy" with all the interfaces that you define to keep things decoupled. It's clear that there's a model hiding in plain sight in the The "contributions model" is shared between the activity, the list fragment, the detail fragment and the media detail view pager; extracting a model will allow us to decouple all these classes and get rid of some nasty type casting. The contributions model would be the one to subscribe for updates about upload progress, and in turn push changes to the view. Furthermore, decoupling the data loading from the model (as you will see in the "clean architecture" description) would let us load featured images into the model, and reused the grid view, detail view and view pagers plus all their interaction. In the clean architecture language, these would be our "repository" classes encapsulating where data actually comes from. If you're looking for where Room fits in, the data access objects would be utilized by the repository classes. Google architecture components wouldn't talk in terms of any repository classes, and would refer to the contributions model as a "view model". Although repositories add some complexity I believe the abstraction more than pays for itself in terms of allowing reuse of across "contributions" and "featured images". A simpler case might be a user profile screen. This would be broken into
All of these parts would be packaged together (that is packaging by feature) possibly with a Dagger module that wires them together. Another example is the |
@psh Thanks so much for the feedback! We are certainly contemplating adopting one of the commonly-used architectural patterns, but weren't sure which one. Your diagram is very helpful, I will read up more on the proposed architecture and come back with questions. :) @sivaraam this issue is mainly for discussing the backend overhaul though. Could you please create a new one for the login screen (and other proposed UI revamps)? Thanks! |
I've been doing some digging - I implemented most of an "Upload Queue" and had to test what a failed upload looked like, and if I could restart it. That's when I encountered a bug deep in the app, while running on an Emulator (but I expect real phones to act similarly). When photos are shared with our app, we pull them from the incoming @Override
protected void onAuthCookieAcquired(String authCookie) {
mwApi.setAuthCookie(authCookie);
Intent intent = getIntent();
if (intent.getAction().equals(Intent.ACTION_SEND_MULTIPLE)) {
if (photosList == null) {
photosList = new ArrayList<>();
ArrayList<Uri> urisList = intent.getParcelableArrayListExtra(Intent.EXTRA_STREAM);
for (int i = 0; i < urisList.size(); i++) {
Contribution up = new Contribution();
Uri uri = urisList.get(i);
up.setLocalUri(uri);
...
}
}
...
uploadController.prepareService();
}
} The incoming Uri has a @Override
public void queue(int what, Contribution contribution) {
switch (what) {
case ACTION_UPLOAD_FILE:
contribution.setState(Contribution.STATE_QUEUED);
contribution.setTransferred(0);
contributionDao.save(contribution);
...
}
} The problem is that I believe the fix is fairly simple - as soon as we receive the intent (in Even if we cache the image file we need to be aware that it could be deleted (say, stored on the external SD card, deleted by a user / card removed) and should cleanup the contributions database accordingly. |
@misaochan @nicolas-raoul - it's been a while since we visited the topic of software architecture and I was curious where your thinking has landed. On January 24th you wrote,
How has the reading gone? Do you have questions? Have you reached any conclusions? |
Hi @psh , apologies, we have been swamped with the current grant (and planning future ones), so have not had the time to do further reading on this. :/ I do plan to do so prior to us actually making a concrete decision for implementation, though. I do have a brief question, however: How many man-weeks (assuming 40 hours per week) do you think it would take to implement this overhaul? Is there a significant difference in the time needed, or difficulty of implementation, for different architecture? That will likely determine whether this task will go into the plans for the next grant or not (and consequently how soon we need to make a decision). :) Thanks so much! |
I suspect that the app doesn't have to cache the file (which seems to be an indicator that it's becoming memory hungry). In Linux based systems, once you open a file and get the file handle successfully you generally don't have to worry about file getting deleted. So IIUC, Android uses the Linux kernel and thus I think it's enough if we could just have open file handles (possibly using Note that, this isn't a solution for the "card removed" case. We would have to trade that off for not caching the images. To avoid issues, I think we could handle that by checking the validity of the file handle before actually using it. WARNING: I might be wrong as I'm not sure if Android does something in the middle to shatter my theory that "open file handles are enough to solve the issue of local files getting deleted". So, it might be better to verify this. |
Our current plans for this (see also #1863 ):
|
@dbrant This is one of the first tasks that we wish to pick up for IEG 2019 as our current network layer with Moreover, I checked out Wikipedia's codebase and the network layer( |
The work of splitting off our networking layer into a standalone library is now in progress, and should be done in a couple of weeks. https://phabricator.wikimedia.org/T215949 |
Wonderful news @dbrant ! :) |
@dbrant Is the library ready for consumption? |
Yes, it is! Include it in gradle, in the usual way: The library provides a class called Once that is done, you will be free to use the library's numerous affordances, such as the I've got a local branch going where I'm attempting to create the beginnings of this integration. In the coming days, watch for a PR that will kick things off. |
Woohoo! \o/ Thanks so much, @dbrant . |
@dbrant Is there any specific reason for keeping minSdkVersion for the library as 19. Currently, our app targets 15 and above. |
Sure, I can take the library minSdkVersion down to 15. It's currently set to 19 because that's what we are using in our app. Testing on ancient devices with API <19 is an enormous QA burden, with ever diminishing returns. Our policy is to support API versions for which Google still issues security updates. Is there a reason you still support such old APIs? You have a grand total of 35 installs on devices with API <19. |
We mostly try and support as many old API versions as possible, because we target users in developing countries that don't own PCs or have landlines (and hence cannot easily use the Upload Wizard). Theoretically many of them may be using secondhand phones or very old phones. So we are hesitant to increase the minimum because while we only have 35 users on <19 now, as the app grows, the number may increase. It is true that testing on older devices is a huge burden though. :/ I am not sure where it would be best to draw the line, to be honest. |
Keep in mind that even if you increase your minSdkVersion, the previous version of the app will still be available to older devices via the Play Store. So you wouldn't be depriving any users from accessing the app; you will simply be freeing yourself up to focus on meaningful features, rather than worrying about supporting extreme edge cases. (By the way, one of those 35 installs is me -- I found a really old device in my collection and installed the app on it, and was able to get it to crash within 2 minutes.) But I digress, and I'll gladly update the library with minSdk=15, so that you can make these decisions on your own time. |
@dbrant We have updated the |
Talked to @maskaravivek and @neslihanturan about this today. It feels like a lot of time is wasted trying to understand the legacy code in our app and getting new modifications to work well with it. Much of the time is spent patching up something that no one in our team is really familiar with and that was written more than 4 years ago (a long time in Android development). When I talked to the original developer, he said that he created the app when he was relatively new to Android development as well, so he didn't have an answer for some questions on why things are done the way they are. :)
This will be a very long haul task (and will probably not happen anytime soon), but we are considering allocating a few months in the future (possibly for the next grant) for completely overhauling the app backend. The intention is for it to all be modernized and coherent rather than playing patchwork with legacy code.
If we do this, we should make sure to include Javadocs for each class we create so that future developers don't encounter the same problem.
Thoughts/suggestions? What model/architecture should we adopt if we do this?
Edit:
Copying the current plan for the app overhual(from @misaochan's later comment) to this comment to make it easier to track.
Module progress:
The text was updated successfully, but these errors were encountered: