-
Notifications
You must be signed in to change notification settings - Fork 579
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
Add Sixel Support #2157
base: master
Are you sure you want to change the base?
Add Sixel Support #2157
Conversation
Sorry about the delay, I've dropped some older unsupported python versions from CI so it should run if you push a superficial change and I'll take a look in the meantime |
Related to Guake#1806 sem-ver: feature
@@ -211,6 +211,15 @@ def configure_terminal(self): | |||
if hasattr(self, "set_alternate_screen_scroll"): | |||
self.set_alternate_screen_scroll(True) | |||
|
|||
enable_sixel = client.get_boolean("use-sixel") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a preferences option? Seems like the kind of thing that anyone who has a terminal that can support it would want. If we do keep this as an option in preferences, we also need to add some switches on setting toggle, because as-written currently if a user clicks the box to enable they still won't see sixels until they restart guake.
I'm leaning towards always on and throw a low log message if the toggle fails just because I feel like everyone who knows these exist would be fine having them. Preferences windows become scary when there's too many things in them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it's probably a good idea to have this optional at least as long as sixel support is still considered experimental in vte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is experimental right now, it's also quite unavailable, I think the only way currently for people to have a version of general VTE that supports sixels installed is to compile it yourself, which is tantamount to opting in, and when it lands in an official VTE release pushed to general users I would think it is no longer experimental.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I'll remove the option to enable them and just turn them on when available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably still keep the setting in the gschema, if advanced users want to still opt-out somehow / for some reason. But I'll default it to enabled, would that be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, sure. And a memo for another day to change some things when sixels come to VTE proper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been busy, but I got the time to take a look at this, don't have a sixel enabled version of VTE installed to check that the flag works so I still need to do that but I do still have thoughts on parts of this.
This adds support for displaying Sixels, in case VTE is compiled with support for it enabled.
This is related to #1806.
Upstream issue for Sixel support: https://gitlab.gnome.org/GNOME/vte/-/issues/253
Side notes:
While developing I came across three issues:
make build
with pipenv >=2022.10.9 #21562.13
because otherwise it would complain about options that no longer exist.8.0.4
: Incompatible with click 8.1.0 (ImportError: cannot import name '_unicodefun' from 'click') psf/black#2964