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(compatibility): do not set scroll buffer in alternate screen #1032

Merged
merged 8 commits into from
Feb 2, 2022

Conversation

tlinford
Copy link
Contributor

@tlinford tlinford commented Jan 27, 2022

fix #952.

The 0/1 problem was due to creating the viewport for the alternate screen app with an empty line, and then in fill_viewport pushing current lines to lines above.

I also found that if resizing a window containing a full-screen tui app, the various calls to transfer_rows_from_viewport_to_lines_above were causing the pane to become scrollable.

I'm still a bit unsure of how to handle things in change_size, but have not been able to see any misbehaving apps so far.

@tlinford tlinford requested a review from imsnif January 27, 2022 19:34
@tlinford tlinford marked this pull request as draft January 28, 2022 07:25
@tlinford tlinford force-pushed the fix/alternate-buffer-scrolling branch from 994a7ed to 8ae259a Compare January 30, 2022 21:07
@tlinford
Copy link
Contributor Author

in alternate screen mode change size now handles width changes by truncating extra characters, and height changes by dropping excess lines. @imsnif does that make sense to you?

@tlinford tlinford marked this pull request as ready for review January 31, 2022 17:25
@tlinford tlinford changed the title wip fix(compatibility): do not set scroll buffer in alternate screen fix(compatibility): do not set scroll buffer in alternate screen Feb 2, 2022
@tlinford tlinford merged commit 143bbfb into main Feb 2, 2022
@tlinford tlinford deleted the fix/alternate-buffer-scrolling branch February 2, 2022 11:56
@ahkrr
Copy link

ahkrr commented Feb 11, 2022

@tlinford. After this commit, calling something like fzf crashes zellij.
After investigating, I found that emptying the viewport in this line

// zellij-server/src/panes/grid.rs +1871
let current_viewport = std::mem::take(&mut self.viewport);

leads to
Error: thread 'screen' panicked at 'called `Option::unwrap()` on a `None` value': zellij-server/src/panes/grid.rs:
because

// zellij-server/src/panes/grid.rs +1145
fn pad_current_line_until(&mut self, position: usize) {
    let current_row = self.viewport.get_mut(self.cursor.y).unwrap();

tries to index into an empty viewport.

When I reverted mem::take back to

let current_viewport = std::mem::replace(
     &mut self.viewport,
     vec![Row::new(self.width).canonical()],
);

the crash was gone.

@tlinford tlinford mentioned this pull request Feb 12, 2022
@tlinford
Copy link
Contributor Author

@ahkrr Thanks for figuring this out, glad you caught it before release. I merged the fix and also added a test case for fzf.

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.

Extra Character Row for full Screen TUIs ("Scroll 0/1")
2 participants