Apps-android-commons: Decouple upload logic from fragments/activities, unify upload entry points

[misaochan]

As mentioned at #1060 , this probably needs to be done prior to implementing multiple uploads. Currently upload logic is coupled to various entry points - ShareActivity.java, MultipleShareActivity.java, and soon to be NearbyMapFragment and PlaceRenderer as well when direct uploads from Nearby are implemented. We would like to get the direct Nearby uploads out first so that users can use the feature, but after that is done we should buckle down and get this sorted.

Scope of this task:

• Remove upload logic from the various entry points and have it all in one reusable module

[maskaravivek]

@misaochan I would like to take it up after the crash fixes if you guys agree. I already have some changes locally relating to this refactoring and i can own it up. :)

[misaochan]

@maskaravivek Great! By all means. :)

[misaochan]

@whym @psh or anyone else: Suggestions on how we can best organize the upload logic, especially in preparation for #604 ?

[misaochan]

Also, I think this should go on a feature (upload-overhaul) branch, so that we can get a complete implementation of the following, before we roll out to users:

• Upload code overhaul (this)
• Multiple uploads from within the app (#604 )
• Upload tests (#705 )

[neslihanturan]

@maskaravivek do you still want to have it? If yes, I will assign to you.

[misaochan]

@neslihanturan I think Vivek mentioned on Hangouts that he wanted to swap this with me (so he is doing Wikidata edits now and I am doing this) :)

[misaochan]

Looking through ShareActivity.java today (which has 1000+ lines!!!!), these are my plans at the moment:

• To simplify permission logic, I don't think it's necessary to have Storage permissions requested via both Snackbar AND dialog (when user taps the submit button). The Snackbar should be used solely for optional permissions like Location, IMO.
• Functions like Zoom and Find other images should have their own classes.
• Both <action android:name="android.intent.action.SEND_MULTIPLE" /> and <action android:name="android.intent.action.SEND" /> intent filters should be directed to ShareActivity.java, and within ShareActivity.java itself we will handle things differently depending on whether 1 or many images are selected.

[psh]

@misaochan There was a discussion a little while ago: #1128 - "Thinking about a combined share GUI" - with design mockups online.

If that's the direction you're going with creating a "combined" GUI that merges the single and multiple uploads then I have a large portion of it already coded and working that you could use as a baseline to start from - see my "combined-upload" branch which implements a new UploadActivity following a Model-View-Presenter style. Happy to drop that as a PR onto a feature branch. I also extracted a "categories model" from the categorization fragment as I wanted to share code with my new upload, that helped a lot with code complexity.

Where I stopped -

• Integrating "quality" checks (picture too dark for instance) and adding a "yes/no" for including bad pictures in the upload batch
• Snagging GPS coordinates from other (recent) pictures - the whole implementation is in view code right now and needs to be teased apart
• Pulling data directly from incoming shared content:// URIs into a local cache to support "offline" uploads and restarting failed uploads

Also, looking at my repo, I think I need to push code from my laptop at home. I'll rebase on master and push tonight when I get home.

[misaochan]

@psh That would be wonderful, thanks so much! :) I was initially planning for the new upload UI to be a future enhancement (the aim of this issue was just to unify the upload entry points), but given that you appear to have already done a substantial amount of work on it, I could indeed try to build on what you've done and perhaps the goal of #1128 might be realized sooner than expected. :) I'll take a look at the PR as soon as you submit it.

Have the xml files for the new layout already been added as well, or only the backend?

@maskaravivek @neslihanturan What do you guys think? This might add a bit more work for us, but it makes more sense than overhauling the old code, when the new mockups and code structure are much better IMO.

[maskaravivek]

It would be great to have @psh's contribution in overhauling the upload flow. Building upon his changes would be quite nice.

[maskaravivek]

Copying my comment from the PR for the keeping the conversation at one place.

Although we appreciate the efforts put into making these changes, I am not in favor of merging it as:

• the mentioned PR is still a WIP. It's sometimes difficult to build upon code that has changed 1000s of lines and might break/undo a few functionalities. Analysing and fixing all those issues would take a lot of time as our testing process is still manual.
• the PR in itself tries to address two issues: refactoring, adding combined uploads. A PR should ideally address just one issue unless the changes can't be segregated
• It would add a substantial amount of workload on the team if the combined upload is picked up along with the refactoring at this moment. Originally the plan was to pick up just the refactoring for now.

[misaochan]

Having gone through the PR, I agree that it would be too difficult to build on @psh 's work at this point of time - the scope for his changes is much larger than what we originally intended, and it will be difficult to get it to work within our grant deadlines. (I probably titled this issue wrongly at the start - we are not "overhauling" the uploads per se, we just want upload logic in 1 reusable module to unify upload entry points before we can enable multiple uploads).

However, I'd like to float the possibility of picking it up in the next grant. The upload UI and process is very much due an overhaul, and that PR provides a good foundation.