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

sheet: Fix scrolling up when a buffer refill is required #271

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

richiejp
Copy link
Contributor

@richiejp richiejp commented Nov 8, 2024

There is a bug which happens when scrolling up requires a buffer refill. Essentially it appears to be off-by-one when deciding if the desired row is outside the buffers lower boundary. It can be reproduced by pressing G then C-u or k repeatedly until it gets stuck.

Adding the header_span value to the lower boundary appears to resolve the issue, but I'm not completely sure this is the correct solution.

There's another bug where scrolling down one page from the top then trying to scroll up one page does not work unless the arrow keys are used to go a bit further down first. It doesn't appear to be related to this one.

@liquidaty
Copy link
Owner

@richiejp thank you! Is there a trivial test we can add, that should pass with this change in place (but fail when this change is not applied)?

@richiejp
Copy link
Contributor Author

richiejp commented Nov 9, 2024

I added a test to scroll up from the bottom to the top (it skips a test number due to a conflict with my other PR). Thanks!

@liquidaty
Copy link
Owner

@richiejp great, thank you. It looks like the test requires a file "expected/test-sheet-9.out" that needs to be committed to the repo. Could you add that and then confirm that the CI tests pass?

@richiejp
Copy link
Contributor Author

Sorry that catches me out every time, I have added a .gitignore for that dir so that the .out files are not ignored there.

@richiejp richiejp force-pushed the scroll-buffer-fix branch 3 times, most recently from c835a6f to a65cd3a Compare November 10, 2024 13:00
Also remove *.out from .gitignore for the expected directory so that
the expected outputs don't have to be force added.
@richiejp
Copy link
Contributor Author

I had to add quite a large delay to the test for it to pass in CI. Probably it's best to poll the output until it settles for 0.5 of a second instead of setting absolute delays.

sleep 0.5 && \
tmux send-keys -t $@ "G" && \
tmux send-keys -t $@ -N 4096 "k" && \
sleep 2 && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may adjust -y (height) above to reduce the number of repeat count -N and delay here.

Change tmux-256color to TMUX_TERM to pass all OS tests
@liquidaty liquidaty merged commit fae69db into liquidaty:main Nov 11, 2024
11 checks passed
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.

3 participants