Fix infinite loading circular progress bar after nominating for deletion #6324
Conversation
There was a problem hiding this comment.
I tried this branch and unfortunately when I tap the red button the progress thing kept circling fo several minutes.
Also, the picture it was about did not get the nomination banner:
https://commons.m.wikimedia.org/wiki/File%3AOktamov_Rozimattilla.jpg
|
Which variant did you tried? I tried on prodDebug and it worked. Also, what about the Toast message? |
|
prodDebug too. |
Yes, the logs would be helpful here, thanks :) |
|
I just found out that @parneet-guraya was already working on the fix. I just missed it and looked at @neeldoshii's draft PR. However, both of them are not working for @nicolas-raoul. Could you please try to record a screencast while reproducing the issue and logs would be very helpful in fixing this problem? |
|
Here is all I see in logcat after entering a deletion reason ad tapping |
|
Do not hesitate to add much more logcat calls everywhere, then I can provide another log. :-) |
|
I've added some log statements depending on the expected flow of operation. Please try to repro the issue again and also a screencast as well. |
|
Thanks! Here is what I am getting on Pixel 7 after entering a deletion reason and tapping Strangely the app is otherwise working great, I even reinstalled it and it is loading all thumbnails very quickly from the Internet. Screencast taken at the same time as the logcat above: 395.mp4 |
|
Thanks for the logs @nicolas-raoul |
|
Both great ideas! 👍 |
|
Hi @nicolas-raoul, It's appended to the reason string which is passed when making a deletion request: As you mentioned, fetching achievements for a user with thousands of contributions is time-consuming. So, we should look for some other API that can get us From the logs: It seems that it took about 3 minutes before throwing The /**
* Disables Progress Bar and Update delete button text.
*/
private fun disableProgressBar() {
activity?.run {
runOnUiThread(Runnable {
binding.progressBarDeletion.visibility = View.GONE
})
} ?: return // Prevent NullPointerException when fragment is not attached to activity
} |
|
Great research, thanks! I don't think it is vital to know the number of articles using the image, to perform the nomination. So, in case a timeout happens, how about proceeding with the nomination, just with the number of articles being unknown? |
Hmm, I think that also works but we also need to have a lesser timeout because currently, it's like 3 minutes which is enough to frustrate the user. Do know the reason for appending |
|
Frankly, I don't really think we need that number. Do you know what commit added it? (you can use "git blame") |
Okay, I am looking for the reason why it was added. |
|
This PR added the I also found these methods which return the file usage of a file: And this one: Do you think we can utilize the above method which uses the |
|
Sorry for the delay!
|
|
Hi @nicolas-raoul For both, we are getting only one page using this image. However, the feedbackResponse says 4 articlesUsingThisImage and 2 uniqeUsedImages: I don't think that fetching achievements can be replaced with fetching file usages. What do you think? |
|
Also, I can see only one usage is displayed on the Media Details Page for the same, which I was trying to delete:
According to this, the file usage is correctly fetched via the Edit: The image you're trying to remove in this screencast doesn't have any usage, so it's clear that fetching our contributions is not a viable option. |
|
How about changing the wording to "at least"? |
|
That's a good idea. It will be consistent for both images either uploaded by us or someone else. Should I work on this? |
|
If you have time, sure! Thanks a lot for all of your hard work. 🙂 |
|
No worries at all, I'd love to work on this as this issue is been opened for quite a long. I'll make the changes today :) |
|
Hi @nicolas-raoul, I'm sorry I accidentally used It's only giving the pages that are outside of commons, i.e., 3 for my image, whether Now, it makes sense why the |
|
If easily doable, we should ignore usage on Commons. Reason: Most usage on Commons is just galleries, not useful to actual readers. |
Fetching achievements is a time consuming operation and globalFileUsage gives the similar result in optimal time
|
Hi @nicolas-raoul, I made the changes. Please record the logs again while testing the recent changes. Also, is there any place on the Commons web page where we can see who submitted a deletion request with what reason? |
|
I will fix the unit test and remove logs added for testing once the behavior is verified. |
|
I just tested on a picture with doubtful copyright status, it is working great! |
That's great, as I asked can we see the deletion request for a picture somewhere in the commons website because even though I replaced the API call there's some code which I think won't be sending the reason we were building so far. |
|
Hey @nicolas-raoul, I just found this page where I can see the deletion request that I made previously: https://commons.m.wikimedia.org/wiki/Commons:Deletion_requests/File:A_phone_holder.jpg It's clear that the template for deletion we expect is not sent with request not even before. |
Could you please send me the image file name or a screenshot so that I can verify? |
|
I'm so sorry for bothering you again and again, but I'm just trying to provide as much information as possible about this issue. So, I tweaked the code to include the reason we generated (which includes file usage and upload date) and used the exact value returned by the getReason() method. With this change, the final reason looks like this: As you can see, the formatting doesn’t look quite right — there’s no spacing or punctuation between the main reason and the additional metadata. Additionally, the phrase “Uploaded by myself” seems somewhat redundant, as the
Ideally, I would recommend this format for the deletion requests: The same format could be used when making deletion requests for images uploaded by others, but a custom reason entered by the user will be prepended before the metadata. If we still want to add the "Uploaded by myself" string to the reason, then we make sure it will be added for images uploaded by the current user, not when nominating files uploaded by others. Please feel free to suggest more formats as well. |
|
Some other formats:
|
Does that work even if we nominate someone else's work for deletion? On a side note, the changes can still be merged as they fix the issue and you can pick the formatting up as a follow-up PR :) |
Nope, doesn't work either :(
You're right, but these are pretty minor ones, and we already have enough context of the problem in this PR, so I thought we could solve this in this PR. Anyways, I am fixing the tests and removing test logs. Let me know if I should create a separate issue for that. |
Should we retain the user name then?
Works even in this PR. I suggested that because we still had a few more cases to explore like nomination by other users and we could take that up as a separate discussion. |
No, we directly pass the raw reason string entered by the user without any username, date, or file usages. However, we do construct the formatted reason by calling the
Yep, you're right, I'll create a separate issue for that. Let's make this PR limited to the progress bar issue only. |
|
Sorry, here is the file I nominated: |
|
Please let me know when ready, feel free to mark as draft for now. :-) |
|
✅ Generated APK variants! |
|
It's ready to merge @nicolas-raoul :) |
|
✅ Generated APK variants! |
There was a problem hiding this comment.
Tested with https://commons.m.wikimedia.org/wiki/File:Zainuri_Kamaruddin.jpg it works great. Thanks a lot Rohit!
Description (required)
Fixes #5531
What changes did you make and why?
Parneet fixed the issue in his PR #5610 by making changes in the
MediaDetailFragment.javafile. However, that file was migrated toKotlinand caused merge conflicts. So, I pull from his branch, copy the changes to the migrated file and refactor the related code for better code quality.Tests performed (required)
Tested prodDebug on Samsung A14 with API level 34.
Screenshots (for UI changes only)
After_Fix_Progress_Bar.mp4