Optimize `vterm`: adopt data types and add annotations. Fix tests #547

penguinolog · 2023-04-30T17:20:33Z

TermModes - make dataclass as it's used
TermScroller - deprecate and use deque instead
(faster both side access, automatic size limit control)
utf8_buffer - use bytearray as mutual storage for bytes
(no re-allocation if no crazy size changes)
CSI_COMMANDS - use namedtuple types for better type management
use native string escape sequences instead of hex encoded one
(easier to read and understand, internally the same)
not elif after return (follow actual static checkers rules)
_target_encoding = 'ascii' as default (no re-cast)
add extra verbose version of tests for scrolling region

Checklist

I've ensured that similar functionality has not already been implemented
I've ensured that similar functionality has not earlier been proposed and declined
I've branched off the master or python-dual-support branch
I've merged fresh upstream into my branch recently
I've ran tox successfully in local environment

penguinolog · 2023-04-30T17:24:02Z

urwid/tests/test_vterm.py

@@ -238,6 +243,58 @@ def test_erase_display(self):
        self.write(r'98765\e[8D\e[1Jx')
        self.expect('   x5a98765')

+    def test_scrolling_region_simple_incremental(self):


this extra verbose tests maybe removed or replace "short" versions.
@wardi which approach is better?

penguinolog · 2023-04-30T17:25:47Z

urwid/util.py

@@ -74,7 +74,7 @@ def detect_encoding() -> str:
 else:
    assert 0, "It worked!"

-_target_encoding = None
+_target_encoding = 'ascii'


it's set implicit, make explicit

penguinolog · 2023-04-30T17:26:12Z

urwid/vterm.py


 KEY_TRANSLATIONS = {
-    'enter':     chr(13),
+    'enter':     "\r",


more human-readable with the same internals

penguinolog · 2023-04-30T17:26:48Z

urwid/vterm.py

@@ -98,7 +101,19 @@
    'f5':        f"{ESC}[15~",
 }

-CSI_COMMANDS = {
+
+class CSIAlias(typing.NamedTuple):


named tuples less complex to deal with

penguinolog · 2023-04-30T17:28:12Z

urwid/vterm.py

-        self.utf8_eat_bytes = None
-        self.utf8_buffer = b''
+        self.utf8_eat_bytes: int | None = None
+        self.utf8_buffer = bytearray()


bytearray changed in-place
bytes is immutable and new object on any change

penguinolog · 2023-04-30T17:30:23Z

urwid/vterm.py

-        if self.term_modes.lfnl and key == "\x0d":
-            key += "\x0a"
-
-        key = key.encode(self.encoding, 'ignore')


do not change type of variable just for 1 line, make in-place during call

urwid/tests/test_vterm.py

penguinolog · 2023-05-01T13:39:30Z

urwid/tests/test_vterm.py

@@ -140,7 +145,7 @@ def expect(
        else:
            desc += '\n'
        desc += f'Expected:\n{what!r}\nGot:\n{got!r}'
-        self.assertEqual(got, what, desc)
+        self.assertEqual(what, got, desc)


Most unittest parsers expect, that expected value is first

penguinolog · 2023-05-01T13:40:42Z

urwid/vterm.py

@@ -1690,16 +1755,14 @@ def keypress(self, size: tuple[int, int], key: str) -> str | None:
                key = chr(ord(key[-1]) - ord('A') + 1)
        else:
            if self.term_modes.keys_decckm and key in KEY_TRANSLATIONS_DECCKM:
-                key = KEY_TRANSLATIONS_DECCKM.get(key)
+                key = KEY_TRANSLATIONS_DECCKM[key]


we already know that it in KEY_TRANSLATIONS_DECCKM

ulidtko · 2023-05-01T17:38:53Z

urwid/tests/test_vterm.py

@@ -60,6 +63,7 @@ def read(self, size: int) -> bytes:

    def write(self, data: bytes) -> None:
        os.write(self.writer, data)
+        sleep(0.025)


Not fond of this. Never really fixes flaky tests — merely shifts flake probability.

Did you try os.fdopen(self.writer).flush() instead ?

Or better yet, disabling buffering completely with `unbuffered_pipe = os.fdopen(self.writer, 'wb', 0)

We can try unbuffered pipe, but stdout will be still buffered

os.fdopen fails in 10 from 10 runs with Bad file descriptor
os.fsync also disagree to deal with pipe

That can be disabled too.

In a unit test though, why is there a dependency onto sys.stdout at all? Can't it allocate a private PTY ?

... Even that seems redundant, as .read() passes the output of Terminal.render() to asserts directly:

urwid/urwid/tests/test_vterm.py

Lines 118 to 129 in bac20db

def read(self, raw: bool = False, focus: bool = False) -> bytes | list[list[bytes, typing.Any, typing.Any]]:

self.term.wait_and_feed()

rendered = self.term.render(self.termsize, focus=focus)

if raw:

is_empty = lambda c: c == (None, None, b' ')

content = list(rendered.content())

lines = (tuple(dropwhile(is_empty, reversed(line))) for line in content)

return [list(reversed(line)) for line in lines if line]

else:

content = rendered.text

lines = (line.rstrip() for line in content)

return b'\n'.join(lines).rstrip()

I don't see any need to use sys.stdout at all.

Render spawn process, which execute pty.fork().
Data read from fork stdout and rendered.

ulidtko

The naming of added tests is incomprehensible.

test_scrolling_region_simple_incremental
test_cursor_scrolling_region_incremental
test_scrolling_region_simple_with_focus_incremental
test_cursor_scrolling_region_with_focus_incremental

When seen side-by-side — one can sort-of-make-out-maybe-guesstimate how they differ.

When seen pages apart — no chance 😕

penguinolog · 2023-05-01T18:08:29Z

The naming of added tests is incomprehensible.

test_scrolling_region_simple_incremental

test_cursor_scrolling_region_incremental

test_scrolling_region_simple_with_focus_incremental

test_cursor_scrolling_region_with_focus_incremental

When seen side-by-side — one can sort-of-make-out-maybe-guesstimate how they differ.

When seen pages apart — no chance 😕

Tests was added only for debug purposes and now it's possible to decide: keep or drop

penguinolog · 2023-05-01T18:22:29Z

Without huge mocking, `Terminal.spawn` call `pty.fork`, which accept 0 arguments. Op ma 1 mei 2023 20:14 schreef Maxim Ivanov ***@***.***>:

…

***@***.**** commented on this pull request. ------------------------------ In urwid/tests/test_vterm.py <#547 (comment)>: > @@ -60,6 +63,7 @@ def read(self, size: int) -> bytes: def write(self, data: bytes) -> None: os.write(self.writer, data) + sleep(0.025) That can be disabled too. In a unit test though, why is there a dependency onto sys.stdout at all? Can't it allocate a private PTY ? ... Even that seems redundant, as .read() passes the output of Terminal.render() to asserts directly: https://github.com/urwid/urwid/blob/bac20db87eaeb0b7a0026807d7e6dbe391b08580/urwid/tests/test_vterm.py#L118-L129 I don't see any need to use sys.stdout at all. — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD2KMM2HHK25XI3XWHNVPHDXD74QLANCNFSM6AAAAAAXRBQW2M> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ulidtko · 2023-05-02T07:41:24Z

Terminal.spawn call pty.fork, which accept 0 arguments.

Yes, that's the Terminal's internal pty. This is fine and expected.

What I'm talking about is DummyCommand in test_vterm touching sys.stdout. It a dummy "command" running in the terminal and doing IO to its PTY just for tests, why does it need sys.stdout?

penguinolog · 2023-05-02T08:20:09Z

Terminal.spawn call pty.fork, which accept 0 arguments.

Yes, that's the Terminal's internal pty. This is fine and expected.

What I'm talking about is DummyCommand in test_vterm touching sys.stdout. It a dummy "command" running in the terminal and doing IO to its PTY just for tests, why does it need sys.stdout?

DummyCommand sys.stdout output points to PTY, not to system real stdout

* `TermModes` - make `dataclass` as it's used * `TermScroller` - deprecate and use `deque` instead (faster both side access, automatic size limit control) * `utf8_buffer` - use `bytearray` as mutual storage for bytes (no re-allocation if no crazy size changes) * `CSI_COMMANDS` - use `namedtuple` types for better type management * use native string escape sequences instead of hex encoded one (easier to read and understand, internally the same) * not `elif` after `return` (follow actual static checkers rules) * `_target_encoding = 'ascii'` as default (no re-cast)

penguinolog · 2023-06-12T17:59:29Z

Removed test changes: at this time I have lack time for refactoring it

penguinolog commented Apr 30, 2023

View reviewed changes

penguinolog requested review from wardi, ulidtko and tonycpsu April 30, 2023 17:25

penguinolog commented Apr 30, 2023

View reviewed changes

urwid/vterm.py

KEY_TRANSLATIONS = {

'enter': chr(13),

'enter': "\r",

Copy link

Collaborator Author

penguinolog Apr 30, 2023

Choose a reason for hiding this comment

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

more human-readable with the same internals

penguinolog commented Apr 30, 2023

View reviewed changes

urwid/tests/test_vterm.py Outdated Show resolved Hide resolved

penguinolog changed the title ~~ptimize vterm: adopt data types and add annotations. Fix tests~~ Optimize vterm: adopt data types and add annotations. Fix tests Apr 30, 2023

penguinolog force-pushed the vterm_annotations_optimizations branch 2 times, most recently from b980cec to f40c05e Compare May 1, 2023 12:15

penguinolog added the refactoring label May 1, 2023

penguinolog commented May 1, 2023

View reviewed changes

ulidtko requested changes May 1, 2023

View reviewed changes

ulidtko reviewed May 1, 2023

View reviewed changes

penguinolog force-pushed the vterm_annotations_optimizations branch from f40c05e to f00e7ba Compare May 5, 2023 08:52

wardi approved these changes Jun 12, 2023

View reviewed changes

penguinolog merged commit 2bd0ead into urwid:master Jun 12, 2023
5 checks passed

penguinolog deleted the vterm_annotations_optimizations branch June 12, 2023 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize `vterm`: adopt data types and add annotations. Fix tests #547

Optimize `vterm`: adopt data types and add annotations. Fix tests #547

penguinolog commented Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog Apr 30, 2023

penguinolog May 1, 2023

penguinolog May 1, 2023

ulidtko May 1, 2023

penguinolog May 1, 2023

penguinolog May 1, 2023

ulidtko May 1, 2023

penguinolog May 1, 2023

ulidtko left a comment

penguinolog commented May 1, 2023

penguinolog commented May 1, 2023 via email

ulidtko commented May 2, 2023

penguinolog commented May 2, 2023

penguinolog commented Jun 12, 2023

	def read(self, raw: bool = False, focus: bool = False) -> bytes \| list[list[bytes, typing.Any, typing.Any]]:
	self.term.wait_and_feed()
	rendered = self.term.render(self.termsize, focus=focus)
	if raw:
	is_empty = lambda c: c == (None, None, b' ')
	content = list(rendered.content())
	lines = (tuple(dropwhile(is_empty, reversed(line))) for line in content)
	return [list(reversed(line)) for line in lines if line]
	else:
	content = rendered.text
	lines = (line.rstrip() for line in content)
	return b'\n'.join(lines).rstrip()

Optimize vterm: adopt data types and add annotations. Fix tests #547

Optimize vterm: adopt data types and add annotations. Fix tests #547

Conversation

penguinolog commented Apr 30, 2023

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulidtko left a comment

Choose a reason for hiding this comment

penguinolog commented May 1, 2023

penguinolog commented May 1, 2023 via email

ulidtko commented May 2, 2023

penguinolog commented May 2, 2023

penguinolog commented Jun 12, 2023

Optimize `vterm`: adopt data types and add annotations. Fix tests #547

Optimize `vterm`: adopt data types and add annotations. Fix tests #547