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

Fix(scroll): Prevent scroll buffer overwrite #655

Merged
merged 4 commits into from
Oct 1, 2021
Merged

Fix(scroll): Prevent scroll buffer overwrite #655

merged 4 commits into from
Oct 1, 2021

Conversation

kunalmohan
Copy link
Member

@kunalmohan kunalmohan commented Aug 21, 2021

Fix #306
Currently, if a command is producing output (eg tail), then scrolling up causes the command to overwrite the scroll buffer.
This PR fixes it. Now when a pane is scrolled up, new vte events are buffered and when we exit scroll, the events are played at once.

@kunalmohan kunalmohan requested a review from imsnif August 21, 2021 15:50
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Very cool - happy to see you getting into these parts of Zellij as well. :)

A few things:

  1. I'm a little afraid of the buffer being uncapped. If I leave a pane scrolled up with input running overnight I might build up quite the memory utilization by morning... do you think we can cap it somehow and then if it exceeds that cap we reset the scroll? not the best ux, but I think it's better than now, no? Or do you maybe have a different idea?
  2. When we play the pending vte events, at least on my machine, there's quite a long period where things are stuck. I assume it's when the events are interpreted. Do you think we might be able to add some renders in the middle of this somehow? So the user sees that something is happening?

@kunalmohan
Copy link
Member Author

kunalmohan commented Sep 25, 2021

@imsnif I've incorporated your suggestions and updated the PR. For now, I've kept the buffer size to 7000 events per pane and the tab will render after processing every 100 pending events. I can update the numbers as per your (or anyone else's) suggestions.

Also, this works at all times- inside scroll mode or outside (when scrolling with the mouse).

@imsnif
Copy link
Member

imsnif commented Sep 28, 2021

@kunalmohan - I think this isn't working for me, but maybe I'm doing something wrong.

What I did:

  1. Started an infinite loop that prints the date every 0.1 seconds (while true; echo (date); sleep 0.1; end)
  2. Scrolled up (ctrl+s and k a few times)
  3. Pressed ENTER to leave scroll mode
  4. I'm still scrolled up and not at the bottom of the pane
  5. When I scroll down it updates but I think there's a gap there? I'm not sure about this one

@kunalmohan
Copy link
Member Author

I've addressed the review comments and made the required updates. Things work for me as expected, therefore, merging this now.
Any further changes can be made later based on user experience and suggestions.

@kunalmohan kunalmohan merged commit 3c05f26 into main Oct 1, 2021
@kunalmohan kunalmohan deleted the scroll-fix branch October 1, 2021 16:50
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.

scrolling when text updates
2 participants