Fixes #2815 - Nominating for deletion is cancelled on leaving the media details #4295
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neslihanturan
approved these changes
Mar 19, 2021
There was a problem hiding this comment.
Approving by reading the code, tomorrow I will manually test it. Thanks @Pratham2305 :)
neslihanturan
requested changes
Apr 1, 2021
There was a problem hiding this comment.
Hey @Pratham2305 , thanks a lot for these changes. It mostly works but I have some additional ideas:
- Select image and jump on media details
- Click on nominate and nominate image for deletion, progressbar appeared as expected
- Click to back button and jump on contributions list
- Select the image again
- Expected behavior is to see a progressbar to describe a nomination for deletion is in progress here. What I see is just the nominate for deletion button with no progressbar.
- After image is nominated while I am staring at media details screen, expected behavior is the button (with progressbar) will be changed with banner as soon as the process completed. What currently happens is that I still see the button (not the banner) even after the image is nominated (process is done). I have to go back to contributions list and come back again to see "this image is nominated" banner.
This fix solves it functionally, however UI flows are not perfect yet.
|
Thanks for the review @neslihanturan, I am able to notice the issue and will try to resolve it ASAP. |
|
Hey @Pratham2305 , wanted to ping you to make sure you don't claim any other issues before this review is done :) |
Codecov Report
@@ Coverage Diff @@
## master #4295 +/- ##
============================================
- Coverage 10.22% 10.19% -0.04%
Complexity 469 469
============================================
Files 342 342
Lines 13084 13126 +42
Branches 1062 1064 +2
============================================
Hits 1338 1338
- Misses 11678 11720 +42
Partials 68 68
Continue to review full report at Codecov.
|
|
I have made the changes, @neslihanturan Can you please review them. Thanks! |
|
Hey @Pratham2305 , I keep getting the same/similar crash on this PR. Here is logcat: another one: another one: |
neslihanturan
requested changes
Apr 19, 2021
|
@neslihanturan Sorry but I am not able to reproduce any of these crashes. I think it can be again related to the number of contributions made, Have you also noticed these crashes on explore tab? |
|
I am able to reproduce it now while testing on an account with lots of contributions, but I have also noticed this same crash on the latest master Here the screen recording(prodDebug, latest Master): screen_recorder_app_crash.mp4I have made the changes that will fix the crashes, @neslihanturan can you please re-test this PR. Thanks! |
|
Hey @Pratham2305 , sorry for my delay to review. Can you please fix conflicts? |
Codecov Report
@@ Coverage Diff @@
## master #4295 +/- ##
============================================
- Coverage 10.28% 10.26% -0.03%
- Complexity 476 482 +6
============================================
Files 342 343 +1
Lines 13149 13301 +152
Branches 1076 1092 +16
============================================
+ Hits 1353 1365 +12
- Misses 11728 11865 +137
- Partials 68 71 +3
Continue to review full report at Codecov.
|
|
I have resolved the conflicts. @neslihanturan Can you please review the PR again. Thanks! |
nicolas-raoul
requested changes
May 31, 2021
app/src/main/java/fr/free/nrw/commons/bookmarks/BookmarkListRootFragment.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/bookmarks/BookmarkListRootFragment.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/category/CategoryDetailsActivity.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/explore/depictions/WikidataItemDetailsActivity.java
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/explore/media/PageableMediaFragment.kt
Show resolved
Hide resolved
| @@ -339,6 +350,12 @@ public void onResume() { | |||
| media = getArguments().getParcelable("media"); | |||
| } | |||
|
|
|||
| media = detailProvider.getMediaAtPosition(index); | |||
|
|
|||
| if(media != null && applicationKvStore.getBoolean("Nominating " + media.getDisplayTitle(), false)) { | |||
There was a problem hiding this comment.
The KV store is used to keep track of what files are being nominated, if I understand correctly?
How about a final static String rather than this inline String which seems to be duplicated several times below?
Also, does getDisplayTitle give the best identifier? We need something that does not change and is unique. I don't remember exactly but getDisplayTitle might return the same for several different images (screenshot showing pics I uploaded yesterday, same label displayed for two different pictures), and might also change if I change the locale?
There was a problem hiding this comment.
Thanks for the review, I will make the required changes and I got your point. How about we use ImageUrl instead of display title, I think that will be unique for each image.
app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.java
Show resolved
Hide resolved
nicolas-raoul
requested changes
Jun 3, 2021
| @@ -228,6 +231,8 @@ public static MediaDetailFragment forMedia(int index, boolean editable, boolean | |||
| */ | |||
| private int minimumHeightOfMetadata = 200; | |||
|
|
|||
| final static String NOMINATING_MEDIA = "Nominating %s"; | |||
There was a problem hiding this comment.
Would you mind making NOMINATING_FOR_DELETION_MEDIA = "Nominating for deletion %s"? It will be more understandable. Thanks!
nicolas-raoul
approved these changes
Jun 4, 2021
|
Thanks Pratham, great enhancement! |
ashishkumar468
pushed a commit
to ashishkumar468/apps-android-commons
that referenced
this pull request
Jul 5, 2021
…ing the media details (commons-app#4295) * fix issue with nominating for deletion * Fix UI issue * minor improvements * fix App crash * Added Javadoc and other minor improvements * Updated string name Co-authored-by: Pratham2305 <Pratham2305@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description (required)
Fixes #2815
What changes did you make and why?
The issue was because as on clicking back, all the added disposables cleared with compositeDisposable.clear()
So now I have not added the deletion disposable to the container and also added a progress bar to tell the user that the app is working on it as the process is kinda slow
Tests performed (required)
Tested betaDebug on Pixel 3 with API level 29.
Screenshots (for UI changes only)