Completely overhaul app backend (legacy code)

[misaochan]

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.

• Replace all usages of the deprecated org.mediawiki:api library with Retrofit (plus RxAndroid) for network calls, which will also remove our reliance on the outdated yuvi:http.fluent and org.apache.http.legacy libraries. User:DBrant_(WMF) has informed us that the Wikipedia app team are working on packaging their network code into a standalone library that adheres to these best practices and will be usable by other apps, so we will use their library to simplify this implementation once it is ready. (includes Migrate all APIs to retrofit #3026 ) Edit 13 Aug: due to difficulty of maintaining the Wikipedia data client library, we were advised to fork/copy the source code instead of using it directly.
• Refactor all AsyncTasks to use RxAndroid instead, for consistency
• We have already implemented a stricter process for including new libraries in pull requests, and we will now go through all our existing dependencies to see if we should keep or remove them, and refactor the code accordingly. Go through all our existing dependencies to see if we should keep or remove them #3128
• Refactor our content providers to adhere to modern standards, and eliminate hardcoded column positions Refactor our content providers to adhere to modern standards #3127
• Consolidate our multiple SharedPreferences databases into one
• Clean up all string literals in code and replace them with string resources, to better take advantage of TranslateWiki translations Clean up all string literals in code and replace them with string resources #3129
• Overhaul our authentication system, which has been the culprit of several failed upload issues (the issues have been mostly patched up, but still happen occasionally, i.e. the foundations are shaky). We will investigate how the authentication flow can be simplified, and take a look at how we are handling edit tokens (the usual cause of failures). -> Refactor network layer to use OkHttp. Edit 13 Aug: The switch to data client seems to have solved failed uploads in our tests. We will hold off on this until we release v2.12 and get feedback from users, since this may no longer be necessary. If anyone still experiences issues, we will then proceed with this.
• Ensure thorough test coverage for all overhauled components (for reference, our grant success metric is >25% code coverage)

Module progress:

• Upload process
• Network layer / authentication
• Notifications / achievements
• Explore / media details
• Nearby
• Peer review

[sivaraam]

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 :)

Screen shots

[neslihanturan]

You could look at #725 @sivaraam . It will be ready in 3 weeks hopefully. But we don't have a discussion about re-do login screen. Maybe you can start one.

[psh]

Several big pieces of the deeper parts of the app we have today -

• SQLite Database
  • A solid decision, and should stay in place, although the method we use to access it might evolve
  • Probably worth creating some documentation of what's actually in there
• Sync Adapter
  • Suggests that we might want to use the provided ContentProviderClient to connect with a ContentProvider to access data, but the documentation points out that its safe to ignore this parameter if you want to
  • Several moving parts which makes it harder to understand for newcomers (Sync adapter itself, Bound service, XML configuration, some data storage mechanism)
  • ✔️ Big plus - allows background syncing of data triggered by network availability without the app being open
• Content Providers
  • Use Cursor based data loaders for asynchronous access
  • Not always the clearest interface (take a look at the hard-coded column positions, or ContentValues in the app)
  • Kind of arcane callback mechanism to signal changes
  • ✔️ Big plus - write the DB access code as if it's synchronous let someone else handle threading
  • ✔️ Plus - works well with a sync adapter
• Bound Service
  • Part of the package of dealing with a sync adapter but doesn't need to be; could stand alone if we needed it to
  • ✔️ Plus - offers other communication mechanisms that we're not currently utilizing (we could subscribe for updates to know about upload progress by talking directly to the service and not indirectly through content provider notifications on a cursor row)
  • ✔️ Plus - Upload logic in one place

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 ContentProviderClient. So if I had to pick any part of this picture that seemed both optional and overly complicated (and I probably betrayed it in the description already) it's the ContentProvider classes we have. I believe that other ways are clearer for getting feedback from the bound service when its doing the work - just expose methods on the IBinder instance that's passed back into onServiceConnected() for instance.

@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 notifyChange() methods we would be free to explore Room (or another ORM) to get at the data in our SQLite database.

(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.)

[psh]

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

• MVC (Model / View / Controller) - everyone talks to everyone else, and can get messy
• MVP (Model / View / Presenter) - really clean separation, but often results in lots of classes and interfaces
• MVVM (Model / View / Viewmodel) - very similar to MVP, just depends where you want business logic to live
• TEA (The Elm Architecture) / Flux / Unidirectional data flow - most widely seen in React / Redux based Javascript webapps but also applied to Android
• The so-called "Clean Architecture" - leans toward MVP, nicely written up online: https://fernandocejas.com/2015/07/18/architecting-android-the-evolution/

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 ContributionsActivity - the combination of my upload count, and the list of actual contributions that come from the database. Oh, and did you notice that the ContributionsActivity loads that list twice right now, and just drops one of them on the floor?

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

• Profile activity (just a dumb view)
• Profile view model
• Profile repository (makes service calls to the wikimedia api to populate the model and save back to the server)
• Profile DAO (data access object) if we choose to cache to the local database

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 NearbyActivity which has several views (the list, the map and the footer) that all share a model. If the view subscribes to the model, and it subscribes to changes from the repository then you can incrementally feed data back to the GUI as you progressively load data from the wikidata backend in ever widening radii.

[misaochan]

@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!

[psh]

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 Intent

@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 content:// prefix, and we store it in the database inside the UploadService without any processing

@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 content:// Uris are inherently temporary - the picture was available for a while but I closed the app and restarted and now the record in our database is invalid because its Uri was pointing into space. If we intend to retry the upload at a later point, or support any kind of "limited connection mode" where uploads stay in a queue until Wifi is available, then we cannot have the local Uri expire like this.

I believe the fix is fairly simple - as soon as we receive the intent (in ShareActivity and MultipleShareActivity) we need to pull the available bytes and cache them, that is, cache the image. The "local uri" in the database then needs to be the path to the file.

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.

[psh]

@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,

I will read up more on the proposed architecture and come back with questions.

How has the reading gone? Do you have questions? Have you reached any conclusions?

[misaochan]

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!

[sivaraam]

I believe the fix is fairly simple - as soon as we receive the intent (in ShareActivity and MultipleShareActivity) we need to pull the available bytes and cache them, that is, cache the image. The "local uri" in the database then needs to be the path to the file.

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 FileInputStream class) to the files that need to be uploaded and use them when needed rather than having to cache the whole file which would be a bad idea if someone tries to upload around 20 high quality images. (Of course, we would have to close the file handles correctly after the job is done :-))

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.

[misaochan]

Our current plans for this (see also #1863 ):

• Replace all usages of the deprecated org.mediawiki:api library with Retrofit (plus RxAndroid) for network calls, which will also remove our reliance on the outdated yuvi:http.fluent and org.apache.http.legacy libraries. User:DBrant_(WMF) has informed us that the Wikipedia app team are working on packaging their network code into a standalone library that adheres to these best practices and will be usable by other apps, so we will use their library to simplify this implementation once it is ready.
• Refactor all AsyncTasks to use RxAndroid instead, for consistency
• We have already implemented a stricter process for including new libraries in pull requests, and we will now go through all our existing dependencies to see if we should keep or remove them, and refactor the code accordingly.
• Refactor our content providers to adhere to modern standards, and eliminate hardcoded column positions
• Consolidate our multiple SharedPreferences databases into one
• Clean up all string literals in code and replace them with string resources, to better take advantage of TranslateWiki translations
• Overhaul our authentication system, which has been the culprit of several failed upload issues (the issues have been mostly patched up, but still happen occasionally, i.e. the foundations are shaky). We will investigate how the authentication flow can be simplified, and take a look at how we are handling edit tokens (the usual cause of failures). -> Refactor network layer to use OkHttp

[maskaravivek]

User:DBrant_(WMF) has informed us that the Wikipedia app team are working on packaging their network code into a standalone library that adheres to these best practices and will be usable by other apps, so we will use their library to simplify this implementation once it is ready.

@dbrant This is one of the first tasks that we wish to pick up for IEG 2019 as our current network layer with ApacheHttpClient and XML based API is a source of bugs and is difficult to manage. Do you have an estimate about by when the library would be ready for first use? Also, it would be great if you could briefly outline the responsibilities of the library so that we can plan what things we need to revamp on our own. :)

Moreover, I checked out Wikipedia's codebase and the network layer(dataclient, auth, csrf modules) look quite modular. Do you suggest replicating these modules in Commons app instead of waiting for the library to be released?

[dbrant]

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

[misaochan]

Wonderful news @dbrant ! :)

[maskaravivek]

@dbrant Is the library ready for consumption?

[dbrant]

Yes, it is!
I've completed the bulk of the work, and here is the repo. Here is a rough sketch of how the integration with the library will actually work:

Include it in gradle, in the usual way:
implementation "com.dmitrybrant:wikimedia-android-data-client:0.0.5"

The library provides a class called AppAdapter which you must derive and implement, that contains a few hooks that are implementation-specific to your app (e.g. cookie storage, account credentials, etc).

Once that is done, you will be free to use the library's numerous affordances, such as the Service class that provides API bindings for use with Rx, as well as LoginClient for logging in, CsrfTokenClient for getting edit tokens, and the many Util classes that will simplify things even further. This will hopefully allow you to rip out a lot of legacy code.

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.

[misaochan]

Woohoo! \o/ Thanks so much, @dbrant .

[maskaravivek]

@dbrant Is there any specific reason for keeping minSdkVersion for the library as 19. Currently, our app targets 15 and above.

[dbrant]

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.

[misaochan]

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.

[dbrant]

while we only have 35 users on <19 now, as the app grows, the number may increase

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.

[maskaravivek]

@dbrant We have updated the minSdkVersion to 19. :)