Skip to content

fixing issue#2131: same description for all photos in multiple uploads #2132

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 2 commits into from
Dec 18, 2018

Conversation

cypherop
Copy link
Contributor

@cypherop cypherop commented Dec 16, 2018

Fixes #2131 Multiple Uploads: All pictures are having same description

What changes did you make and why?
Added a new setNewDescription() method to set description of the photo. Earlier when we were writing any description it was calling setDescriptionText method for all the photos in multiple uploads and all photos were having same description. Now setDescriptionText method is only called for the desired image and all images are not having the same description.

Tested on Motorola Moto G5 plus with API level 24

@cypherop
Copy link
Contributor Author

@domdomegg Well could you please let me know whether the changes that i had made are appropriate for the issue or not
Since I am new to open source. It would be a great pleasure to hear from you.

@domdomegg domdomegg self-requested a review December 17, 2018 21:48
@domdomegg
Copy link
Member

@sp2710 Am having a look now. Usually it can take some time for pull requests to be reviewed, especially if they have to do with uploading stuff as need to thoroughly test haven't broken anything there + actually need to upload files rather than just view some part of the app.

So far your commits have been good, and your work is definitely appreciated :)

A change that will be needed is formatting, we (and many other open source projects) follow the Google Java Style Guide. In some places whitespace needs to be added. First though I'll review fully to check this does fix the issue and doesn't create new ones.

@domdomegg
Copy link
Member

Tests performed

Unit tests pass

Upload multiple files works on
2.9.0-debug-pr-2132~42c530f4 on Galaxy Nexus (emulator) with API level 28

This does solve the issue.

Copy link
Member

@domdomegg domdomegg left a comment

Choose a reason for hiding this comment

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

Minor whitespace fixes in line with GSG 4.6.2

@domdomegg
Copy link
Member

I don't fully understand why this actually fixes the issue. I feel like this should be basically equivalent to what was already there. I'd like you either to explain what exactly wasn't working before that is now, or for someone else to review who does understand it.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@f897af0). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2132   +/-   ##
========================================
  Coverage          ?   5.69%           
========================================
  Files             ?     233           
  Lines             ?   11598           
  Branches          ?    1076           
========================================
  Hits              ?     660           
  Misses            ?   10883           
  Partials          ?      55
Impacted Files Coverage Δ
...r/free/nrw/commons/upload/DescriptionsAdapter.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 f897af0...e94df32. Read the comment docs.

@cypherop
Copy link
Contributor Author

Actually when i put some log messages inside the description Text Changed Listener i saw that the description object for which the setDescriptionText was called was not always the same for which it was desired. I put both description and descriptions inside that method as logs and seen the logs. I saw that description object was not always reffering to the same descriptions

@maskaravivek maskaravivek merged commit 1d80cba into commons-app:master Dec 18, 2018
@cypherop cypherop deleted the fixing#2131 branch December 20, 2018 18:43
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.

4 participants