Implement support for atomic regions in mistty #61
Conversation
Atomic updates feature (also sometimes called synchronized updates) allows terminal programs to delay repaint by a short time so that the state of the terminal can atomically transition from one to another without users seeing flicker or incoherent intermediate states. See https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036
|
This is really cool. Thanks! Sorry for the delay; I thought this was a feature request and not a patch. I'll get right on looking into this change. |
There was a problem hiding this comment.
Thank you for implementing this! This looks like a nice feature!
Did you find some application or command that issues these sequences to test with?
I have a few comments, but feel free to skip any comment not marked required.
required Please add "feat: " to the commit description; I'm following conventional commit to build a changelog.
optional Would you mind adding a test to mistty-test.el that exercise atomic region?
Here's an idea (but anything that'll test the feature would do.) The test could:
- send a printf call with
process-send-stringto enter atomic region mode and display some string - call
accept-process-outputwith a small delay - make sure that the string isn't displayed in the buffer
- send another printf command with
process-send-stringto leave atomic region mode - call
(mistty-wait-for-output :str ...)to make sure the special string does appear
| (?l 0 8)] ; 15: saw \e[?2026 | ||
| "State table for atomic update parser.") | ||
|
|
||
| (defun mistty--substring-fast (string start end) |
There was a problem hiding this comment.
optional mistty--substring-fast would be very much at home in mistty-util.el
| @@ -2358,6 +2373,9 @@ This command is available in fullscreen mode." | |||
| (fire-and-forget (or mistty--forbid-edit | |||
| (string-match "^[[:graph:]]+$" translated-key))) | |||
| (positional (or positional (mistty-positional-p key)))) | |||
| (mistty--with-live-buffer mistty-work-buffer | |||
There was a problem hiding this comment.
required Should sending a key really flush the buffer? If atomic region is used for reasonably-sized changes, it would make sense that a user would send a command that requires a refresh, then start typing just after. Flushing for every key press would reveal changes that should be atomic.
Are you worried about the screen freezing, without the user knowing what to do about it?
In mistty--post-command, there is code that reacts to the user pressing C-g (keyboard-quit). There would be a good place for flushing, I think.
| :group 'mistty) | ||
|
|
||
| ;; State table: (expected-char next-state-if-match next-state-if-no-match) | ||
| (defconst mistty--atomic-state-table |
There was a problem hiding this comment.
just an observation This duplicates logic that is supposed to be handled by the processors.
Ideally you should be able to just do:
(mistty--add-processor accum '(seq CSI "?2026l") (lambda ...))
(mistty--add-processor accum '(seq CSI "?2026h") (lambda ...))
And the processor logic would take care of incomplete commands.
That wouldn't work in this case, because, a processor cannot act as a buffer the way a pre-process does. Ideally, however, it should be possible to use them even in this case - I just don't know how at this time.
I'm not telling you to change anything in this pull request, just pointing out something that I see might be worth changing in the future..
|
Thanks for the feedback! I was actually going to add testing, but the stock test suite doesn't pass for me, so I don't have great confidence I'd be able to distinguish atomic bugs from anything else. |
|
The tests are not passing because you're running Emacs 31? I'm not convinced MisTTY is even usable on Emacs 31; it's not just some harmless tests not passing. Writing tests would be welcome, but as mentioned, it's optional. I can just do it later. It'd be enough for me if you just address the two required comments:
|
I forgot to mention: all tests pass with your current changes on 29 and 30, see below "all checks have passed" from the github actions. If you decide to write a test and manage to write a new test that passes on 31 and update the pull request, github actions will run the CI again and will be able to guarantee it also runs on Emacs 29-30 before I commit. |
Thanks. Fair enough. I'd suggest taking a look at the test failures sooner rather than later though. Eli says Emacs 31 is only 3-4 months from a branch cut, and if there's something horribly wrong with replace-region-contents (which Stefan recently tweaked IIRC) best to know now. I hope it's just some kind of test harness issue. FWIW, mistty itself is usable on master. Occasionally I get an arguments out of bounds error that I haven't been able to repro, but doesn't seem version specific and doesn't block general usability. |
Atomic updates feature (also sometimes called synchronized updates) allows terminal programs to delay repaint by a short time so that the state of the terminal can atomically transition from one to another without users seeing flicker or incoherent intermediate states.
See https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036