search

liquidaty / zsv

zsv+lib: tabular data swiss-army knife CLI + world's fastest (simd) CSV parser
MIT License
209 stars 15 forks source link

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

Closed richiejp closed 1 week ago

richiejp commented 2 weeks ago

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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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.