Skip to content
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

Add Auto-Scroll Support for RecyclerView in Multiselect View #17792

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

Scapesfear
Copy link
Contributor

@Scapesfear Scapesfear commented Jan 10, 2025

Purpose / Description

Fixes the missing auto-scroll functionality when switching to multi-select mode in RecyclerView, ensuring a smoother user experience.

Fixes

Approach

This change adds the missing auto-scroll functionality, ensuring that the RecyclerView scrolls smoothly when entering multi-select mode, improving usability.

How Has This Been Tested?

Samsung F23 5G
https://github.com/user-attachments/assets/9b2dc926-2fbc-4a4c-b4df-7c44a461fc6f

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Outcome is good. Change seems much more complicated than it needs to be

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt Outdated Show resolved Hide resolved
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

A few comments

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jan 11, 2025
@Scapesfear Scapesfear force-pushed the cardBrowser2 branch 2 times, most recently from b309024 to 640eaaa Compare January 11, 2025 15:43
Mizokuiam

This comment was marked as spam.

@Scapesfear Scapesfear requested a review from mikehardy January 11, 2025 15:56
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

This should be done in the ViewModel (with tests)

It should be simpler than we have now:

  • Update onLongPress to provide the View as a parameter, so you don't need to look it up
  • Perform the operation in the ViewModel, likely passing in the topOffset
  • React to an event that the ViewModel emits

@Scapesfear

This comment was marked as resolved.

@Scapesfear

This comment was marked as resolved.

@david-allison
Copy link
Member

Also to note: this doesn't handle the inverse: removing the selection is also visually unstable

@Scapesfear
Copy link
Contributor Author

Also to note: this doesn't handle the inverse: removing the selection is also visually unstable

It works now , I fixed it.

@Scapesfear
Copy link
Contributor Author

test2.mp4

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Much simpler, great job! One minor nitpick

If a test or a @NeedsTest could be added, it'd be great

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt Outdated Show resolved Hide resolved
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Almost there! Code's good, the positioning isn't quite right, I have a list of cards, some of which are multi-line questions, on a tablet

I get instances during testing where I can long press a row, deselect it, and the row under the cursor has changed

@Scapesfear
Copy link
Contributor Author

Scapesfear commented Jan 15, 2025

Almost there! Code's good, the positioning isn't quite right, I have a list of cards, some of which are multi-line questions, on a tablet

I get instances during testing where I can long press a row, deselect it, and the row under the cursor has changed

@david-allison if could you send me a test video or something , it will be very useful.

@david-allison david-allison added Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Jan 15, 2025
@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge squash-merge The pull request currently requires maintainers to "Squash Merge" labels Jan 17, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks so much!!!

@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Jan 17, 2025
@lukstbit
Copy link
Member

@Scapesfear Please be patient and don't request reviews especially if the PR is not ready to be reviewed. You can see that the Github UI is telling you that there are conflicts, meaning code was added to the main branch while this branch was opened and now there are conflicts between the current application code on main and the changes that were done in this PR. Your job is to fix those conflicts and update the PR so it can be merged(also squash the commits into one commit).

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Jan 23, 2025
@Scapesfear Scapesfear force-pushed the cardBrowser2 branch 4 times, most recently from 9470d43 to e263db1 Compare January 23, 2025 14:25
@Scapesfear
Copy link
Contributor Author

Scapesfear commented Jan 23, 2025

@Scapesfear Please be patient and don't request reviews especially if the PR is not ready to be reviewed. You can see that the Github UI is telling you that there are conflicts, meaning code was added to the main branch while this branch was opened and now there are conflicts between the current application code on main and the changes that were done in this PR. Your job is to fix those conflicts and update the PR so it can be merged(also squash the commits into one commit).

done, fixed the merge conflicts and squash-merged the commits in one.

@david-allison david-allison removed Needs Author Reply Waiting for a reply from the original author squash-merge The pull request currently requires maintainers to "Squash Merge" labels Jan 28, 2025
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@BrayanDSO BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Feb 7, 2025
@BrayanDSO BrayanDSO added this pull request to the merge queue Feb 7, 2025
Merged via the queue into ankidroid:main with commit 8b1bed5 Feb 8, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Feb 8, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Feb 8, 2025
@mikehardy
Copy link
Member

Nice one!

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.

Browser: keep selection visually stable
6 participants