zellij-org / zellij

A terminal workspace with batteries included
https://zellij.dev
MIT License
19.42k stars 611 forks source link

fix(grid): fix a problem that the default bottom line of the scrolling region (DECSTBM) is placed outside the terminal screen #3381

Closed akinomyoga closed 1 month ago

akinomyoga commented 1 month ago

Summary

When Zellij receives the control sequence \e[1;r (DECSTBM with the parameter string 1;) from a terminal application, it uses "the terminal height" (self.height) for the second parameter (which is supposed to be the bottom-line 1-based index of the scrolling region).

https://github.com/zellij-org/zellij/blob/c72f3a712bfa92a4a80b4c1ad1dbe7669892a324/zellij-server/src/panes/grid.rs#L1570

However, the internal index of the bottom line should be "(the terminal height) - 1" (self.height - 1) because the internal index is 0-based.

Problem

$ printf '\e[1;r'; seq 999; printf '\e[r'

This doesn't happen when the terminal height is directly specified to the parameter string of DECSTBM as

$ printf '\e[1;%dr' "$LINES"; seq 999; printf '\e[r'

because the index base is properly converted on the following line:

https://github.com/zellij-org/zellij/blob/c72f3a712bfa92a4a80b4c1ad1dbe7669892a324/zellij-server/src/panes/grid.rs#L2733

cf In another location that sets the default scrolling region, the subtraction is properly performed:

https://github.com/zellij-org/zellij/blob/c72f3a712bfa92a4a80b4c1ad1dbe7669892a324/zellij-server/src/panes/grid.rs#L1579-L1581

Note: The problem was found while investigating https://github.com/akinomyoga/ble.sh/issues/450, (though this PR doesn't still fix the original problem).

Remarks

There should still be problems when the terminal window height is changed or when the terminal application explicitly specifies the bottom line to be outside the terminal window. However, solving them would require changing the internal representation of the scroll region, so I avoid performing the large change within this PR. I think they should be separately fixed if needed.

imsnif commented 1 month ago

Thanks for this - good catch!

I agree the internal representation of the scrolling region needs to change (specifically it shouldn't be an Option), and I also agree it's out of scope here.

akinomyoga commented 1 month ago

Thanks!