Support for terminal synchronized output

[danschwarz]

The attached patch is an update for UrwidImageScreen to support the terminal sychronized output spec

sync.patch

I thought it might help the Kitty scroll flicker issue and maybe it does - a little? But the flicker definitely is still there.

Give it a try and see for yourself if it helps anything. Should be harmless on terminals that do not support the spec - but I know that recent versions of Kitty and iTerm2 do.

[AnonymouX47]

I see. This is actually potentially very useful, even beyond just the urwid widget. I wasn't aware of this mode.

By the way, about your patch:

def write(self, data):
    super().write("\033[?2026h" + data + "\033[?2026l")

1. Shouldn't be in Screen.write().
   • This is called multiple times when drawing the screen. So, the sequences will surround every chunk of data written and won't make any difference.
   • the output may or may not be flushed multiple times (even though Screen.flush() is called only once when drawing the screen, buffer size and line buffering may trigger a flush).
2. Should be in Screen.draw_screen() instead.
   • Calls to Screen.write() just before and after a call to super().draw_screen()

I'll try it out with Screen.draw_screen() and I have high hopes for better results. 😃 By the way, if this works out well, would you be interested in opening the PR that implements this?

Thanks so much.

[AnonymouX47]

I've tried it out and yes, it improves (or completely eliminates on some e.g Wezterm) the flickering situation on terminal emulators that support the feature.

I observed that Kitty still flickers when images get large (amount of bytes for the escape sequence, not necessarily size in pixels) enough (not sure what the exact threshold is) but it's still better than without the sequence. Anyways, as long as there are no large (by size in pixels) and (not "or") pixel-dense images on-screen, it should be just fine.

Looking at the original spec for the feature, it states:

Optional Conditions

Terminal emulators may choose to end an atomic update when:

• The number of bytes received since the update began exceeds some threshold
• ...

I suspect the reason for kitty still flickering is either due to the condition above or a timeout (also specified in the spec).

[AnonymouX47]

While trying this out, I realized that it can't work along with UrwidImageJanitor because it writes the image deletion sequences before Screen.draw_screen() is called. I adapted UrwidImageJanitor.render() into UrwidImageScreen and then I thought, "Why not just let UrwidImageScreen handle it all?" i.e clearing images and all.

I don't know why it never occurred to me until now but the Screen actually seems more like the right place for "clearing images of the screen". Also, the fact that it actually isn't ideal (even though it works :grin:) for a widget to explicitly write to output.

I'm considering moving this functionality (automatically clearing images) into UrwidImageScreen and scrapping UrwidImageJanitor entirely, after all, that's all it does. The only reason I might want to leave UrwidImageJanitor is to allow a user to wrap a single widget that is not the topmost widget, probably for the sake of performance but that would nullify synchronized output. What say you?

By the way, I'd also like to add support for this in BaseImage.draw(), it'll improve animations in particular... but that will be later.

[danschwarz]

Thanks for trying this out. I was so happy to see there was an easily overridable method in Screen.write() to implement this support, that I didn't bother looking for something higher level like draw_screen() 🙄 . That's definitely a better place to put it.

And you are probably correct that the remaining flicker with Kitty is due to an aggressive timeout setting or bytes-transferred setting. I might dig into that code looking for what they do, if I have some time.

I think that the right thing to do is kill UrwidImageJanitor and move the screen clearing code to UrwidImageScreen. Simpler is better. Keep UrwidImageJanitor only if performance is unacceptable.

[danschwarz]

Now that you've implemented the code for testing purposes, do you still need me to write a PR?

[AnonymouX47]

I might dig into that code looking for what they do, if I have some time.

Which code? 🤔 If it's Kitty's, this might be a starting point, though the feature was already implemented before that.

I think that the right thing to do is kill UrwidImageJanitor and move the screen clearing code to UrwidImageScreen. Simpler is better. Keep UrwidImageJanitor only if performance is unacceptable.

Ok, good. IMO, the performance is not noticeable (to a human) even for a UI that looks like:

Note that LineBoxes are actually composed of quite a number of widgets/canvases. So, I guess I'll kill it for now and if anything comes up in the future, I can easily bring it back from the grave.

Now that you've implemented the code for testing purposes, do you still need me to write a PR?

I just thought since you brought it up... but you're right. I shouldn't bother you. 😃

[AnonymouX47]

I'll leave this issue open until I implement it in the library's core.

Once again, thanks so much for all your suggestions and help. :pray: