Skip to content

Refactor ShareActivity #1552

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

Merged
merged 67 commits into from
Jun 5, 2018
Merged

Conversation

misaochan
Copy link
Member

@misaochan misaochan commented May 25, 2018

Fixes #1091 . Tested by uploading via app and "share" on prodDebug build, real device running Android 7.0.

Changes made:

  • Directed all upload entry points to ShareActivity. MultipleShareActivity.java will be removed completely in a future PR This redirection cannot be done yet if we are to release 2.8 with this PR.
  • Added Javadocs to methods of ShareActivity
  • Reduced number of lines in ShareActivity from 1000+ to 600+ by extracting file processing functions to FileProcessor.java (if they made sense to be associated with an object) or to FileUtils.java (if they made sense to be static), and extracting zoom functions to Zoom.java.
  • Unified the FileUtils in Upload package and the FileUtils in Utils package, as there is no point in having 2 different FileUtils and it was causing a lot of confusion
  • Simplified permission logic in ShareActivity, there is no point having storage permissions requested in the snackbar as well as at the Submit button, as it is mandatory. The snackbar is now used solely for the optional Location permission
  • Removed as much unnecessary code as possible from onCreate(), reducing number of lines by more than half

misaochan added 30 commits May 1, 2018 01:23
@commons-app commons-app deleted a comment May 29, 2018
@commons-app commons-app deleted a comment May 29, 2018
@commons-app commons-app deleted a comment May 29, 2018
@commons-app commons-app deleted a comment Jun 4, 2018
@codecov-io
Copy link

codecov-io commented Jun 4, 2018

Codecov Report

Merging #1552 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1552      +/-   ##
=========================================
+ Coverage    3.86%   3.87%   +0.01%     
=========================================
  Files         149     150       +1     
  Lines        7576    7556      -20     
  Branches      721     708      -13     
=========================================
  Hits          293     293              
+ Misses       7266    7246      -20     
  Partials       17      17
Impacted Files Coverage Δ
...n/java/fr/free/nrw/commons/CommonsApplication.java 30.61% <ø> (ø) ⬆️
.../java/fr/free/nrw/commons/nearby/NearbyPlaces.java 0% <ø> (ø) ⬆️
...free/nrw/commons/upload/MultipleShareActivity.java 0% <ø> (ø) ⬆️
...fr/free/nrw/commons/settings/SettingsFragment.java 30% <ø> (ø) ⬆️
...ain/java/fr/free/nrw/commons/upload/FileUtils.java 3.22% <0%> (-2.53%) ⬇️
...java/fr/free/nrw/commons/upload/FileProcessor.java 0% <0%> (ø)
...java/fr/free/nrw/commons/upload/ShareActivity.java 0% <0%> (ø) ⬆️
...src/main/java/fr/free/nrw/commons/upload/Zoom.java 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32d45b2...a9ae072. Read the comment docs.

@commons-app commons-app deleted a comment Jun 4, 2018
@misaochan misaochan changed the title [WIP] Refactor ShareActivity and unify upload entry points Refactor ShareActivity Jun 4, 2018
@commons-app commons-app deleted a comment Jun 4, 2018
@misaochan
Copy link
Member Author

@maskaravivek @neslihanturan This is now ready for review. :) I believe it will make fixing #1485 easier after this is merged, as ShareActivity will be more manageable.

@misaochan misaochan mentioned this pull request Jun 4, 2018
16 tasks
…pload-overhaul-fork

# Conflicts:
#	app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java
@misaochan misaochan mentioned this pull request Jun 5, 2018
@commons-app commons-app deleted a comment Jun 5, 2018
Copy link
Member

@maskaravivek maskaravivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested both Nearby uploads and normal upload flow. It works perfectly. Thanks, @misaochan for taking up the refactoring. :)

@maskaravivek maskaravivek merged commit b5a4b58 into commons-app:master Jun 5, 2018
@misaochan misaochan deleted the upload-overhaul-fork branch June 12, 2018 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants