bookmarks: Remove broken position plumbing and dead getItemId - #2044
Merged
svillar merged 1 commit intoAug 14, 2026
Merged
Conversation
Bookmark positions were hardcoded to 0 since the kotlin.UInt change, and their only consumer was a getItemId that never runs with stable ids disabled. Item identity already comes from GUIDs via DiffUtil.
Contributor
Author
|
@svillar PTAL !! |
svillar
approved these changes
Aug 14, 2026
svillar
left a comment
svillar
left a comment
Member
There was a problem hiding this comment.
OK seems a good cleanup. Too risky for a release though.
Contributor
Author
|
thanks !! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #2043
What
Removes
Bookmark.mPosition(hardcoded to 0),Bookmark.getPosition(), and theBookmarkAdapter.getItemId()override — 16 deletions, no additions — resolving the three linked TODOs atBookmark.java:35-37,Bookmark.java:81andBookmarkAdapter.java:325.Why
getPosition()always returns the hardcoded 0 (the TODO notes upstream moved tokotlin.UInt, which Java cannot consume directly), and its only caller isgetItemId(), which never runs: the adapter setssetHasStableIds(false)(BookmarkAdapter.java:61), so RecyclerView never consults it.Restoring the position via a Kotlin interop shim — what the TODOs contemplate — would not actually fix
getItemId(): the display list flattens several folders into one list, so position-in-parent repeats across folders (duplicate ids) and changes when items move, while stable ids must identify the item itself. The adapter already has the right identity mechanism — itsDiffUtilcallback compares GUIDs inareItemsTheSame()(:107-110) — which is what drives list updates today.No behavior change:
hasStableIdsisfalsebefore and after, so the removed override was unreachable, and the removed field had no other consumer (verified by search overapp/src).If stable ids are ever wanted for the bookmarks list,
getItemId()should be derived from the GUID, matching the DiffUtil callback.Testing
./gradlew assembleNoapiArm64GeckoGenericDebug./gradlew testNoapiArm64GeckoGenericDebugUnitTest