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(grid): fix DL/IL being ineffective without scrolling region #3382

Merged
merged 1 commit into from
May 27, 2024

Conversation

akinomyoga
Copy link
Contributor

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.

} else if clear_type == 2 {
self.set_scroll_region_to_viewport_size();
self.fill_viewport(char_to_replace);

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 akinomyoga/ble.sh#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
Copy link
Member

imsnif commented May 27, 2024

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 #3381.

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

@imsnif imsnif merged commit 3188e69 into zellij-org:main May 27, 2024
6 checks passed
@akinomyoga akinomyoga deleted the fix-nop-DL-and-IL branch May 27, 2024 15:36
@akinomyoga
Copy link
Contributor Author

Thanks for your quick review!

@akinomyoga
Copy link
Contributor Author

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!

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.

None yet

2 participants