Skip to content

Fix: IndexOutOfBoundsException crash when removing last two images of multiupload #6124

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 5, 2025
Merged

Fix: IndexOutOfBoundsException crash when removing last two images of multiupload #6124

merged 2 commits into from
Apr 5, 2025
+106 −96

Conversation

rohit9625
Copy link
Contributor

Description (required)

Fixes #6122

What changes did you make and why?
When removing images from Thumbnails Row, the uploadableImages property inside ThubnailsAdapter was not updating. It results in the image being removed at an invalid index and sometimes IndexOutOfBoundsException.
So, I added the code to notify the adapter that the image was removed.

Tests performed (required)

Tested ProdDebug on Samsung A14 with API level 34.

Screenshots (for UI changes only)
Screencast after fix

After_Fix.mp4

@nicolas-raoul
Copy link
Member

Looks great, thanks!
Sorry I just merged a pull request with UploadActivity.java moved to Kotlin, would you mind porting the change?

@rohit9625
Copy link
Contributor Author

Done 👍

nicolas-raoul
nicolas-raoul requested changes Jan 17, 2025
View reviewed changes
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

I am getting this crash when trying to remove the middle picture, any idea what might be the issue? 🙂

APP_VERSION_NAME=5.1.2-debug-rohit9625-fix-multiupload
ANDROID_VERSION=15
PHONE_MODEL=Pixel 9 Pro
STACK_TRACE=kotlin.UninitializedPropertyAccessException: lateinit property basicKvStoreFactory has not been initialized
at fr.free.nrw.commons.upload.mediaDetails.UploadMediaPresenter.getBasicKvStoreFactory(UploadMediaPresenter.kt:54)
at fr.free.nrw.commons.upload.mediaDetails.UploadMediaPresenter.updateImageQualitiesJSON(UploadMediaPresenter.kt:366)
at fr.free.nrw.commons.upload.UploadPresenter.deletePictureAtIndex(UploadPresenter.kt:162)
at fr.free.nrw.commons.upload.UploadActivity.onThumbnailDeleted(UploadActivity.kt:776)
at fr.free.nrw.commons.upload.ThumbnailsAdapter$ViewHolder.bind$lambda$0(ThumbnailsAdapter.kt:72)
at fr.free.nrw.commons.upload.ThumbnailsAdapter$ViewHolder.$r8$lambda$h9qbE-GosgolnpmsfOlWqsny9oU(Unknown Source:0)
at fr.free.nrw.commons.upload.ThumbnailsAdapter$ViewHolder$$ExternalSyntheticLambda1.onClick(D8$$SyntheticClass:0)
at android.view.View.performClick(View.java:8081)
at android.view.View.performClickInternal(View.java:8058)
at android.view.View.-$$Nest$mperformClickInternal(Unknown Source:0)
at android.view.View$PerformClick.run(View.java:31517)
at android.os.Handler.handleCallback(Handler.java:991)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loopOnce(Looper.java:232)
at android.os.Looper.loop(Looper.java:317)
at android.app.ActivityThread.main(ActivityThread.java:8934)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:591)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:911)

@rohit9625
Copy link
Contributor Author

Doesn't happen on my device. What are the exact steps you followed?
Apart from that, I encountered another IndexOutOfBoundsException when deleting images from a 4-image upload. Using notifyDataSetChanged instead of notifyItemRemoved fixed the issue. Would you please try deleting images from a 4-image upload @nicolas-raoul?
I'll push the changes once confirmed.

@nicolas-raoul
Copy link
Member

I just tried the latest commit of this branch, I get the same crash:

screen-20250119-073306.mp4

@rohit9625
Copy link
Contributor Author

I just tried the latest commit of this branch, I get the same crash:

Did you try uninstalling and then installing again? Please try using a different triplet of images.

@rohit9625 rohit9625 mentioned this pull request Feb 25, 2025
Closed
@nicolas-raoul
Copy link
Member

Would you mind solving the conflict?

@rohit9625
Copy link
Contributor Author

Yeah sure :)

@nicolas-raoul
Copy link
Member

I just tried the latest commit of this branch, I get the same crash:
Did you try uninstalling and then installing again? Please try using a different triplet of images.

Well, I believe it should not crash for any group of picture, even without reinstalling. :-)

@rohit9625
Copy link
Contributor Author

rohit9625 commented Feb 25, 2025 •
edited
Loading

Well, I believe it should not crash for any group of pictures, even without reinstalling. :-)

Yes, you're right. But, the error was about basicKvStore so I thought reinstalling could worked.

@rohit9625
Copy link
Contributor Author

Hey @nicolas-raoul, would you mind testing it again?

nicolas-raoul
nicolas-raoul reviewed Mar 8, 2025
View reviewed changes
app/src/test/kotlin/fr/free/nrw/commons/upload/UploadActivityUnitTests.kt
)
method.isAccessible = true
method.invoke(activity)
}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for removing this unit test?

@nicolas-raoul
Copy link
Member

Any idea why the second pic gets removed here? I clicked X only for the last 2 pics.

screen-20250308-225937.mp4

@rohit9625
Copy link
Contributor Author

When we remove the last picture from the row, all of them are getting removed. I asked about this here in the issue

@nicolas-raoul
Copy link
Member

This branch is solving the crash. 👍
Have you observed the bug below? Thumbnails showing up on the depictions selection screen.
I can create a separate issue if you prefer.

screen-20250313-203751.mp4

@rohit9625
Copy link
Contributor Author

This branch is solving the crash. 👍 Have you observed the bug below? Thumbnails showing up on the depictions selection screen. I can create a separate issue if you prefer.

I couldn't reproduce this on my end. I just tried the repro steps on the main and this branch, and they work perfectly fine.

@rohit9625
Copy link
Contributor Author

rohit9625 commented Mar 13, 2025 •
edited
Loading

Ohh sorry, I misunderstood the repro steps. Yes, I can see the top card on the depicts screen, and I believe that was the reason someone added that condition when removing the last image from thumbnails. Don't worry, I am looking into this and will find a better way to handle that.

@rohit9625
Copy link
Contributor Author

Hey @nicolas-raoul, I fixed the thumbnail-card issue. Now, the top card doesn't hide when removing the last image and also doesn't show up on other fragments while deleting images, as addressed in this comment.

Fix_Top_Thumbnail_Card.mp4

@rohit9625
Copy link
Contributor Author

rohit9625 commented Apr 2, 2025 •
edited
Loading

Can a contributor squash their commits to ensure a clean commit history? I saw that you do this when merging our PRs.

@nicolas-raoul
Copy link
Member

We usually squash when merging, which is good enough I believe. :-)
I don't think you can do it within an existing pull request, I might be wrong though.

@rohit9625
fix: resolve IndexOutOfBounds error when removing images from top card
21ca968 
replace deprecated onBackPressed with onBackPressedCallback

remove unit test for deprecated onBackPressed method

remove if-check before deleting picture to prevent hiding top thumbnail card

hide the thumbnail card on fragments other than MediaDetailFragment
@rohit9625
Copy link
Contributor Author

We usually squash when merging, which is good enough I believe. :-) I don't think you can do it within an existing pull request, I might be wrong though.

Well, I squashed all my commits and force-pushed. Next time, if possible, I'll try to squash them before creating a PR. Please check this branch to see if it fixes both issues.

@rohit9625 rohit9625 requested a review from nicolas-raoul April 3, 2025 13:14
@nicolas-raoul
Copy link
Member

It is normal to have many commits in a pull request, because most PRs are not perfect at first and thus need more commits.
Force-pushing makes it harder for us to get your updates.

@rohit9625
Copy link
Contributor Author

Oh, okay got it. I'll keep that in my mind next time and avoid force pushing from now.

@nicolas-raoul
Copy link
Member

Thanks for your understanding :-)
I am testing this branch and will probably continue testing it during the weekend.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2025

✅ Generated APK variants!

nicolas-raoul
nicolas-raoul approved these changes Apr 5, 2025
View reviewed changes
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Seems to solve the issue.

In the future please perform refactoring in a separate pull request, thanks! :-)

@nicolas-raoul nicolas-raoul merged commit 7bf9276 into commons-app:main Apr 5, 2025
1 check passed
@rohit9625 rohit9625 deleted the fix-multiupload branch April 9, 2025 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-raoul nicolas-raoul nicolas-raoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IndexOutOfBoundsException crash when removing last two images of multiupload
2 participants
@rohit9625 @nicolas-raoul