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

feat: scroll to new position after moving item in menu #552

Merged

Conversation

christoph-heinrich
Copy link
Contributor

When moving around items the scroll position doesn't update, which can be annoying when moving things around in a large playlist. Instead the menu should scroll with the moved item.

The problem with this is that scrolling itself causes a render, which means the menu gets rendered once with the updated scroll position but with the playlist item still in it's old position, and then again when the playlist itself gets updated, leading to a short visual artifact.

I can't think of a good solution to this.
It's possible to stop scrolling from requesting renders and instead request renders in other places instead, but that would still lead to problems when something else (e.g. playback position update) causes a render.
It's also possible to preemptively move the menu item in the menu itself, but then if the underlying data doesn't change for whatever reason there is a disconnect between what the menu shows and what's actually there.

@tomasklaen
Copy link
Owner

Can't think of anything either.

But I want this to do scroll_by(from_to_index_delta) instead of scroll_to(). In future, moving might be done with buttons, and the move item button should stay below cursor, but scroll_to() would always put it in the center.

@christoph-heinrich
Copy link
Contributor Author

Having it not stay in the middle is kinda weird for keyboard navigation, but I've changed it now.
Also as it turns out scroll_by() doesn't actually work in this case because the playlist getting updated causes a new scroll of distance 0, canceling out the previous scroll.

@tomasklaen tomasklaen merged commit 5e2c930 into tomasklaen:main May 5, 2023
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.

2 participants