Skip to content
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

Upload overhaul #1796

Closed
wants to merge 97 commits into from
Closed

Conversation

ilgazer
Copy link
Collaborator

@ilgazer ilgazer commented Aug 4, 2018

Based on designs shared in #1128, this code introduces a new UploadActivity that aims to replace the old Share / Multi-Share activities. The basic categorization and upload functionalities are working but many features are still WIP. Also, anything and everything could be broken in the old ShareActivity but they will be gone soon anyway.

ujjwalagrawal17 and others added 30 commits May 1, 2018 15:58
…. Cards show and hide, plus titles are correct. Displayed thumbnails for the shared images
# Conflicts:
#	app/src/main/AndroidManifest.xml
#	app/src/main/java/fr/free/nrw/commons/category/CategorizationFragment.java
#	app/src/main/java/fr/free/nrw/commons/di/ActivityBuilderModule.java
#	app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java
#	app/src/main/res/values/strings.xml
…ory screen now displays GPS and recent categories when nothing is searched.
albendz and others added 8 commits September 6, 2018 21:40
…p#1849)

* Add button on image details to copy wikicode to clipboard

* Make copy wikicode button width the same as the nominate deletion button width by filling in background
…1859)

* Eliminate the use of Picasso.

This gets rid of the single use of the Picasso library (which was causing
the whole library to be imported and shipped) and replaces it with Glide.
TODO: replace this and the other instance(s) of Glide usage with Fresco,
or vice versa.

* Remove dependency on Glide.

This removes the dependency on Glide, as well as the SVG rendering
library, whose only purpose was to display a single SVG image in the
Notification activity. Unfortunately Android doesn't support SVG natively,
but Echo notifications have icons that are SVG formatted. Rather than
import a bunch of heavy libraries to support this single case of SVG
rendering, we can simply create a few local drawables that correspond to
the different types of notifications, and use them instead.

* Remove multidex!

Multidex is a killer of performance and should be avoided at all costs.

* Remove further unused bits.

* Remove final vestige of multidex.
* Fix issue#1772

Add the CDATA tag to welcome_help_button_text string. Set the text of the corresponding textview using Html.fromHtml() function.

* Fix issue#1772

Add the CDATA tag to nominated_see_more string. Set the text of the corresponding textview using Html.fromHtml() function.
app/src/main/res/values/strings.xml: Add the missing angle bracket.
* Added permission for Dexter, the runtime permission handling library

* [Preparing fir issue commons-app#1773] Added a utility function which would take the user to app settings screen where he could manually give us the required permission

* Added an alert dialog with positive and negative callback [Preparing fir issue commons-app#1773]

* Improvements in the way External Storage Permission is handled in MultipleShareActivity[Bug fix commons-app#1697]
1. Used dexter to handle the external storage permission
2. Behaviour changes : When user tries to share(uppload) images to commons via MultipleShareActivity, following decision tree is followed
	a. If the app has permission for external storage, normal upload operation is followed
	b. If the app does not has the permission for external storage, dexter is used to ask for the same
	c. If the user gives us the required permission, normal upload flow is proceeded
	d. If the doesnot gives us the required permission a rationale dialog is shown with the appropriate message to let him know why we need the permission
	e. If he presses okay, steps a-c are followed and if he presses cancel, we close the app.
	f. If while asking for permission, the user chooses never ask again, then next time he tries to upload an image via MSA, the rational dialog follows the app setting screen where he could manually give us the required permission and the onActivityResult of same is handled

* Added a Constants class to handle request and result codes from one place and other related constants common to the all app elements

* replaced hardcoded strings ok and cancel in DialogUtil to string resources

* init permission rationale dialog in activities onCreate

* Code formatting, updated access modifiers wherever required, added javadocs for new methods created

* *shifted constants to app class
*Added JavaDocs in PermissionUtils

* removed class REQUEST_CODES from CommonsApplication and instead put the enclosing constants in the App class itself
@maskaravivek
Copy link
Member

@ilgazer Am not sure if the PR is ready for review but just out of curiosity I was trying it out today and had trouble when I chose a photo from Google Photos. It kept showing "Receiving shared content, this may take a moment or two" and never proceeded.

When I clicked a photo from the camera, it worked well.

@ilgazer
Copy link
Collaborator Author

ilgazer commented Sep 7, 2018

I was trying it out today and had trouble when I chose a photo from Google Photos.

This should not be happening at all. I will look into it tonight. By the way, thanks for testing!

@ilgazer
Copy link
Collaborator Author

ilgazer commented Sep 10, 2018

@maskaravivek I failed to duplicate the bug, but this is after I made some changes to the received file caching mechanism. Could you please test again?

Also, as the new UI has achieved feature parity with the old one at this point, should I finally delete the old upload activities and their dependent classes? I have hit the MultiDex method limit already and had to exclude the old Activities from the build as a workaround. Deleting the old activities at this point would allow more flexibility in modifying stuff like FileProcessor and moving some upload stuff to more logical places.

@maskaravivek
Copy link
Member

Thanks for the update @ilgazer. I will try to test the PR tonight.

Also, as the new UI has achieved feature parity with the old one at this point, should I finally delete the old upload activities and their dependent classes?
Yes, you can delete the old classes. Just make sure to keep your branch updated with master so that large merge conflicts can be avoided.

If the PR has achieved feature parity with master, should I test it extensively and list down any issues that I come across?

@ilgazer
Copy link
Collaborator Author

ilgazer commented Sep 15, 2018

If the PR has achieved feature parity with master, should I test it extensively and list down any issues that I come across?

I would love that!

@maskaravivek
Copy link
Member

maskaravivek commented Sep 16, 2018

@ilgazer the PR works wells for me. I tried uploading photos using all the different flows ie.

  • using camera upload button
  • using gallery upload button
  • nearby upload with successful wiki data edit
  • sharing single image from the Google photos app
  • sharing multiple images from the Google photos app

In the process I also tested the following things:

  • trying to upload a dark image shows appropriate warning
  • an image with a description in multiple languages
  • tap to zoom image

There are a few issues that need some tweaking:

  • The plus (+) button is not aligned properly when the view bottom view is collapsed
  • IMO there could be some background for the plus (+) button as it's not very clear as with a white background
  • The map view button at the middle of the screen wasn't intuitive for me. For most of my uploads, nothing was happening on clicking it. Finally, for one of the images, it opened the map. Am not sure what coordinates it shows on opening the map. Am unable to recollect what was its purpose earlier.
  • While trying to share from the Google photos app/gallery, the app immediately closes after clicking on Submit in the final step. For one moment I thought that the app crashed but upon checking the logs it became apparent that images have been queued for upload. It might be helpful to show a toast if queuing of uploads is successful.
  • On the final screen where the user selects the license, the description below it should have a link(according to latest master) to the license.
  • Title edit text should have(according to latest master) a info(i) button that shows a popup
  • The images that I was uploading apparently had .HEIC extension. They got uploaded as .HEIC.jpg. For eg. https://commons.wikimedia.org/wiki/File:Monkey_sitting_near_Shivanasamudram_falls.HEIC.jpg

Also, feel free to remove the dead code and refactor pieces that should be taken up in this PR itself.

@ilgazer
Copy link
Collaborator Author

ilgazer commented Sep 16, 2018

The plus (+) button is not aligned properly when the view bottom view is collapsed

It was supposed to be hidden. Fixed!

While trying to share from the Google photos app/gallery, the app immediately closes after clicking on Submit in the final step. For one moment I thought that the app crashed but upon checking the logs it became apparent that images have been queued for upload. It might be helpful to show a toast if queuing of uploads is successful.

Added a toast.

On the final screen where the user selects the license, the description below it should have a link(according to latest master) to the license.

Done!

Title edit text should have(according to latest master) a info(i) button that shows a popup

Added!

The map view button at the middle of the screen wasn't intuitive for me. For most of my uploads, nothing was happening on clicking it. Finally, for one of the images, it opened the map. Am not sure what coordinates it shows on opening the map. Am unable to recollect what was its purpose earlier.

The map view button shows the EXIF coordinates obtained from the file. The same feature was present under the expanding FAB in the Single Share activity. I will make it so that if there are no coordinates attached to the picture, the button will be hidden. Other than that, how could I make it more intuitive?

@ilgazer
Copy link
Collaborator Author

ilgazer commented Sep 16, 2018

I'm failing to duplicate the HEIC bug as the HEIC image isn't even recognized as an image. Could you tell me the steps you took between taking the photo on an iphone x and uploading them to commons?

Added "Starting Upload" toast.
Added link to the license on final screen.
Made it so that the map button is hidden when image lacks gps coords.
@maskaravivek
Copy link
Member

Thanks, @ilgazer. The above fixes work well for me.

Just one more feedback. Right now, there is no check for an empty title while uploading. This check is there on the master. Can you add it?

Also, it would be nice if you can include the latest changes from the master and make the PR up to date. This PR might stay open for a while before it is finally merged with master. :)

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.