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

Implement scroll up and down #103

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zblz
Copy link
Contributor

@zblz zblz commented Nov 21, 2017

This PR implements handling of the SU and SD CSI commands in pyte.Stream and the corresponding scroll_up and scroll_down methods in pyte.Screen.

@superbobry
Copy link
Collaborator

Thanks for the contribution! Could you point me to documentation for these escape sequences?

@zblz
Copy link
Contributor Author

zblz commented Dec 10, 2017

They are (sparsely) documented here: http://invisible-island.net/xterm/ctlseqs/ctlseqs.html

The most common use of this is when you call clear inside of a tmux session with a status bar. Instead of clearin everything and redrawing the status bar, tmux will scroll the non-status-bar lines out of the screen using scroll up. We also looked at vttest requirements to check its behaviour.

@mvilim
Copy link

mvilim commented Feb 7, 2019

Any chance this can be merged?

I ran into the lack of scroll support when using the Python curses module. Scrolling can be invoked implicitly in ncurses (I believe this is due to hardscroll.c, which appears to detect 'scroll-like' updates, even if a scroll was not explicitly requested.)

Here is a script that demonstrates psuedo-scroll behavior. With TERM set to a terminal with scrolling capability (e.g. xterm-256color), this script fails to update the 0, 0 cell to 1 (i.e. the first assert should fail) because ncurses decides to send a scroll command. With TERM set to linux the script passes.

import curses
import pyte
import os


def psuedo_scroll(stdscr):
    stdscr.addstr(0, 0, '0')
    stdscr.addstr(2, 0, '1')
    stdscr.refresh()
    stdscr.addstr(0, 0, '1')
    stdscr.addstr(2, 0, '2')
    stdscr.refresh()


pid, fd = os.forkpty()
if pid == 0:
    curses.wrapper(psuedo_scroll)
else:
    screen = pyte.Screen(80, 24)
    stream = pyte.ByteStream(screen)
    # pass through bytes directly
    stream.select_other_charset('@')
    while True:
        try:
            output = os.read(fd, 1024)
            stream.feed(output)
        except OSError:
            break
    assert(screen.buffer[0][0].data == '1')
    assert(screen.buffer[2][0].data == '2')

I don't know of any easy way to set the term capabilities with greater granularlity than TERM (to disable scrolling but leave support for other features (e.g. color)), so I would prefer to not use TERM=linux. Additionally, merging this would save some time for others who run into the issue (it took some debugging to trace observed incorrect terminal state to the scroll behavior).

I tested this pull request and it solves both my original issue and causes the above test to pass.

self.buffer[y] = self.buffer[y + lines].copy()
else:
self.buffer[y].clear()
self.dirty = set(lines_to_scroll)

Choose a reason for hiding this comment

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

Should update the dirty set rather than replace it.

Suggested change
self.dirty = set(lines_to_scroll)
self.dirty.update(lines_to_scroll)

@@ -1043,6 +1048,26 @@ def debug(self, *args, **kwargs):
By default is a noop.
"""

def scroll_up(self, lines):
"""Scroll up `lines` lines."""
lines_to_scroll = range(self.margins.top, self.margins.bottom + 1)

Choose a reason for hiding this comment

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

We should handle when margins is None.

Suggested change
lines_to_scroll = range(self.margins.top, self.margins.bottom + 1)
margins = self.margins or Margins(0, self.lines - 1)
lines_to_scroll = range(margins.top, margins.bottom + 1)

Comment on lines +1055 to +1058
if y + lines in set(lines_to_scroll):
self.buffer[y] = self.buffer[y + lines].copy()
else:
self.buffer[y].clear()

Choose a reason for hiding this comment

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

Scrolling is similar to indexing multiple times, is it not? If we follow its example (and it may be more efficient) we can simply move the lines we want to move and pop the lines we want to erase, rather than copying and clearing the dictionaries. We can also improve efficiency by checking the bottom margin rather than creating a set to test for set inclusion.

Suggested change
if y + lines in set(lines_to_scroll):
self.buffer[y] = self.buffer[y + lines].copy()
else:
self.buffer[y].clear()
if y + lines <= margins.bottom:
self.buffer[y] = self.buffer[y + lines]
else:
self.buffer.pop(y, None)

SU = "S"

#: *Scroll down*
SD = "T"

Choose a reason for hiding this comment

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

While the xterm documentation states that 'T' is the correct code for Scroll Down (SD), the xterm code is implemented to handle the incorrect code '^' and treats 'T' as either mouse tracking or SD (depending on the arguments).
I'm not sure how many (or if any) applications still use the incorrect code.

Comment on lines +1051 to +1052
def scroll_up(self, lines):
"""Scroll up `lines` lines."""
Copy link

@altoidbox altoidbox Jul 8, 2020

Choose a reason for hiding this comment

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

The documentation states that a scroll with no parameters scrolls 1 line.

Suggested change
def scroll_up(self, lines):
"""Scroll up `lines` lines."""
def scroll_up(self, lines=None):
"""Scroll up `lines` lines.
:param lines: number of lines to scroll up. defaults to 1 if unspecified.
"""
lines = lines or 1

Comment on lines +1061 to +1069
def scroll_down(self, lines):
"""Scroll down `lines` lines."""
lines_to_scroll = range(self.margins.bottom, self.margins.top - 1, -1)
for y in lines_to_scroll:
if y - lines in set(lines_to_scroll):
self.buffer[y] = self.buffer[y - lines].copy()
else:
self.buffer[y].clear()
self.dirty = set(lines_to_scroll)
Copy link

@altoidbox altoidbox Jul 8, 2020

Choose a reason for hiding this comment

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

Similar to scroll_up, this should also default to 1 line if the argument is unspecified. However, an argument of 0 is used to reset mouse tracking (may be xterm specific). In any case, if we get an argument of 0, we don't want to scroll at all. And if we don't get an argument we do want to scroll exactly 1 line. However, looking at the current implementation of _parser_fsm, we may be unable to differentiate between receiving no arguments and receiving a 0 as the first argument. Reading vt100.net, they seem to indicate that for their purposes there is no need to distinguish between the value 0 and an unspecified argument (i.e., an explicit 0 still implies the default). Perhaps this is an xterm specific issue in this case.

Any other changes made to scroll_up should be appropriately mirrored here in scroll_down.

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.

5 participants