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

Keep scroll position if not at the top of the list #95

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

JL102
Copy link
Contributor

@JL102 JL102 commented Apr 4, 2023

I found it difficult to click on the folder path of an item when lots of things were being downloaded, making the list fly by. This will auto-increment the list's scroll value if it's nonzero when a new item is added, such that the visual scroll position of the list does not change.

@bpozdena
Copy link
Owner

bpozdena commented Apr 5, 2023

@JL102 , unfortunately this does not work. I'm not even sure how it's meant to work or how it was tested.

You are currently incrementing the scrollbar value by 1 each time a new list item is added/deleted/updated. This causes the scrollbar to reach the end after several events, which makes it even more janky then it currently is.

https://i.imgur.com/ODH637N.gif

@JL102
Copy link
Contributor Author

JL102 commented Apr 5, 2023

Oops, I assumed it was an integer that went from 0 to the maximum item in the list, not a percent. I guess we could multiply it by 100 / (the # of items in the list) ?

Does it only stop working as expected when there's a really really long list? During all of my testing, it worked perfectly as expected.

@bpozdena
Copy link
Owner

bpozdena commented Apr 5, 2023

Sry, I removed the % symbol. It was not meant to be there. The currentScrollValue should correspond to the item index.

The issue is mostly caused by the fact that the first item is often being deleted and then re-added with updated values (different download progress, different state (downloading|done|deleted), etc.). I guess I did it this way because I was lazy and it was easier then changing values in the last item.

@bpozdena bpozdena merged commit da34d54 into bpozdena:main Apr 5, 2023
@bpozdena
Copy link
Owner

bpozdena commented Apr 5, 2023

@JL102, I applied a small patch to it. All good now.

Thanks for the PR!

P.S. If you plan on adding more code in the future, please use Black formatter and set --line-length to 150.

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.

None yet

2 participants