fix(grid): fix DL/IL being ineffective without scrolling region

akinomyoga commented 1 month ago

Summary

In the current main branch of Zellij, the standard control functions DL (\e[%dM) and IL (\e[%dL) are no-op until any other control function directly or indirectly sets the scrolling region.

Problem

Steps to reproduce: The following is a reduced case. Please run the command in a new session of Zellij [before any other terminal applications send ED (\e[...J) or DECSTBM (\e[...r). To make sure, you could use a simple shell such as Bash or Dash].

$ seq 10; printf '\e[10A\e[10MHello, world!\n'

What I expect: The output of the seq command should be removed by \e[10M (delete 10 lines). Then, Hello, world! is printed.
What I get in the main branch: Nothing happens with \e[10M. The output of the seq command remains in the terminal screen, and the message Hello, world! is printed over the output of the seq command.

Those control functions DL and IL defined by the ANSI standard (ANSI X 3.64 and corresponding ECMA-48) should operate on the entire viewport if the scrolling region is not defined. In the first place, the ANSI standard doesn't define the scroll region, so they should work even without specifying the scrolling regions (DECSTBM and DECSLRM).

The problem disappears when any other terminal application sends another standard control function ED(2) (\e[2J) because this control function is designed to set the scroll region to the entire viewport.

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

The control function IL has the same issue. This PR fixes DL and IL so that they operate on the entire viewport by setting the scroll region to the entire viewport (in a similar manner as ED(2)).

For the present behavior in the main branch, I suspect commit cd5ddd1, but I haven't tested it and haven't carefully looked at the change either.

This fixes the issue originally reported at https://github.com/akinomyoga/ble.sh/issues/450.

Remarks

This is unrelated to the present fix, but with the current implementation of ED, the scroll region set by DECSTBM is lost when the terminal application sends ED(2) (\e[2J). Is this intended?

imsnif commented 1 month ago

Hey @akinomyoga - thanks for this as well. I tested it and it looks good. The set_scroll_region_to_viewport_size function is a hack I added around the issues of the problematic scroll_region representation mentioned in https://github.com/zellij-org/zellij/pull/3381.

This whole section of legacy code is badly in need of an overhaul.

akinomyoga commented 1 month ago

Thanks for your quick review!

akinomyoga commented 1 month ago

The set_scroll_region_to_viewport_size function is a hack I added around the issues of the problematic scroll_region representation mentioned in #3381.

This whole section of legacy code is badly in need of an overhaul.

Hmm. Thanks for explaining the background!

zellij-org / zellij