Add a knob to disable sixels at runtime #954

Manually merged
dnkl merged 1 commits from jvoisin/foot:disable_sixels into master 8 months ago

This should close #950

This should close #950
jvoisin changed title from Add a knob to disable sixels at runtime to WIP: Add a knob to disable sixels at runtime 9 months ago
jvoisin force-pushed disable_sixels from 0e4e77eefc to c59e1de395 9 months ago
craigbarnes requested changes 9 months ago
csi.c Outdated
static const char reply[] = "\033[?62;4;22c";
term_to_slave(term, reply, sizeof(reply) - 1);
if (term->sixel.enabled == false) {
static const char reply[] = "\033[?62;4c";
Collaborator

This reply is still indicating sixel support (4). See line 768.

This reply is still indicating sixel support (`4`). See line 768.
Poster

Oops, I inverted 22 and 4, my bad!

Oops, I inverted `22` and `4`, my bad!
jvoisin marked this conversation as resolved
terminal.h Outdated
} render;
struct {
bool enabled:true;
Collaborator

The pre-processor will expand true as 1 here and this declaration will then be parsed as a bitfield.

Using a bitfield may actually make sense, but it should probably done as an explicit :1 declaration and be moved next to the other bool members below.

The pre-processor will expand `true` as `1` here and this declaration will then be parsed as a bitfield. Using a bitfield may actually make sense, but it should probably done as an explicit `:1` declaration and be moved next to the other `bool` members below.
jvoisin marked this conversation as resolved
craigbarnes reviewed 9 months ago
craigbarnes reviewed 9 months ago
dcs.c Outdated
sixel_init(term, p1, p2, p3);
term->vt.dcs.put_handler = &sixel_put;
term->vt.dcs.unhook_handler = &sixel_unhook;
if (term->sixel.enabled == true) {
Collaborator

There's no need to fetch params if sixels are disabled.

I'd suggest changing this to e.g.:

if (!term->sixel.enabled)
    break

...and moving it above the 3 calls to vt_param_get().

There's no need to fetch params if sixels are disabled. I'd suggest changing this to e.g.: ```c if (!term->sixel.enabled) break ``` ...and moving it above the 3 calls to `vt_param_get()`.
jvoisin marked this conversation as resolved
craigbarnes reviewed 9 months ago
*sixel*
Boolean. When enabled, foot will process sixel images
Default: _yes_
Collaborator

This option is currently defaulting to false, as far as I can tell. It needs to be included in the initializer at the top of config_load() for it to default to true.

This option is currently defaulting to `false`, as far as I can tell. It needs to be included in the initializer at the top of `config_load()` for it to default to `true`.
jvoisin marked this conversation as resolved
Collaborator

I tested this a bit (with the issues above fixed) and noticed that it "breaks" chafa to some extent. Since chafa assumes foot always supports sixels, running e.g. chafa image.jpg with sixels disabled produces no visible output, instead of falling back to Unicode symbols.

I guess that can be considered chafa's fault for only inspecting $TERM instead of the primary DA, but it's just an example of what downstream issues might result from this change. I suppose it's largely a non-issue in most cases though, considering most users probably won't disable it.

I tested this a bit (with the issues above fixed) and noticed that it "breaks" `chafa` to some extent. Since chafa [assumes](https://github.com/hpjansson/chafa/blob/d7cc111313d65c092158b46186512d3a238a5a1f/chafa/chafa-term-db.c#L354-L355) foot always supports sixels, running e.g. `chafa image.jpg` with sixels disabled produces no visible output, instead of falling back to Unicode symbols. I guess that can be considered chafa's fault for only inspecting `$TERM` instead of the primary DA, but it's just an example of what downstream issues might result from this change. I suppose it's largely a non-issue in most cases though, considering most users probably won't disable it.
Owner

I guess that can be considered chafa's fault for only inspecting $TERM instead of the primary DA, but it's just an example of what downstream issues might result from this change. I suppose it's largely a non-issue in most cases though, considering most users probably won't disable it.

This is one of the reasons I suggested this be a "tweak" option. As such, it's less visible to users, since we don't include tweaks in the example config.

It also leaves us with more room to change the implementation since tweak options aren't guaranteed to be stable.

> I guess that can be considered chafa's fault for only inspecting $TERM instead of the primary DA, but it's just an example of what downstream issues might result from this change. I suppose it's largely a non-issue in most cases though, considering most users probably won't disable it. This is one of the reasons I suggested this be a "tweak" option. As such, it's less visible to users, since we don't include tweaks in the example config. It also leaves us with more room to change the implementation since tweak options aren't guaranteed to be stable.
jvoisin force-pushed disable_sixels from c59e1de395 to 4acd1f6124 9 months ago
jvoisin changed title from WIP: Add a knob to disable sixels at runtime to Add a knob to disable sixels at runtime 9 months ago
jvoisin requested review from craigbarnes 9 months ago
jvoisin force-pushed disable_sixels from 4acd1f6124 to bb5b2f28a3 9 months ago
jvoisin force-pushed disable_sixels from bb5b2f28a3 to c9dcd09b16 9 months ago
jvoisin force-pushed disable_sixels from c9dcd09b16 to 8fee0846dd 9 months ago
jvoisin force-pushed disable_sixels from 8fee0846dd to c858be3a61 9 months ago
craigbarnes requested changes 9 months ago
csi.c Outdated
*/
static const char reply[] = "\033[?62;4;22c";
term_to_slave(term, reply, sizeof(reply) - 1);
if (term->sixel.enabled == false) {
Collaborator

Nitpick: this feels like a bit of a javascriptism. The existing style for this is generally:

if (!term->sixel.enabled) {
Nitpick: this feels like a bit of a javascriptism. The existing style for this is generally: ```c if (!term->sixel.enabled) { ```
jvoisin marked this conversation as resolved
csi.c Outdated
break;
case 'S': {
if (term->sixel.enabled == false)
Collaborator

As above.

As above.
jvoisin marked this conversation as resolved
dcs.c Outdated
case 0:
switch (final) {
case 'q': {
if (term->sixel.enabled == false)
Collaborator

As above.

As above.
jvoisin marked this conversation as resolved
jvoisin force-pushed disable_sixels from c858be3a61 to 525df60420 9 months ago
jvoisin requested review from craigbarnes 9 months ago
Collaborator

@jvoisin this is looking like a clean patch now. However, I noticed some issues while testing with chafa (see here for details).

The problem is only semi-related to this PR, but I think it might be a good idea to fix the underlying logic before landing this.

@jvoisin this is looking like a clean patch now. However, I noticed some issues while testing with `chafa` (see [here](https://libera.irclog.whitequark.org/foot/2022-03-01#1646153555-1646155347;) for details). The problem is only semi-related to this PR, but I think it might be a good idea to fix the underlying logic before landing this.
dnkl added a new dependency 9 months ago
Owner

@jvoisin the DCS buffering improvements have been merged; can you please rebase this PR against the master branch?

@jvoisin the DCS buffering improvements have been merged; can you please rebase this PR against the master branch?
jvoisin force-pushed disable_sixels from 525df60420 to 1fdca759c3 9 months ago
dnkl requested changes 9 months ago
dnkl left a comment
Owner

Overall, this looks good. A couple of minor issues below.

Overall, this looks good. A couple of minor issues below.
csi.c Outdated
case 'S': {
if (!term->sixel.enabled)
break;
Owner

Perhaps we should add a call to UNHANDLED() here?

Perhaps we should add a call to `UNHANDLED()` here?
jvoisin marked this conversation as resolved
dcs.c Outdated
switch (final) {
case 'q': {
if (!term->sixel.enabled)
break;
Owner

... and here

... and here
Poster

UNHANDLED() is csi-specific.

`UNHANDLED()` is csi-specific.
dnkl marked this conversation as resolved
*sixel*
Boolean. When enabled, foot will process sixel images
Default: _yes_
Owner

Can we shorten this to a single line (Boolean. When enabled... Default: _yes_)?

Can we shorten this to a single line (`Boolean. When enabled... Default: _yes_`)?
jvoisin marked this conversation as resolved
terminal.c Outdated
.palette_size = SIXEL_MAX_COLORS,
.max_width = SIXEL_MAX_WIDTH,
.max_height = SIXEL_MAX_HEIGHT,
.enabled = conf->tweak.sixel,
Owner

Since this is a setting that cannot be changed at runtime, we could remove the enabled member from the term struct, and just check term->conf.tweak.sixel in the CSI/DCS handlers.

Since this is a setting that cannot be changed at runtime, we could remove the `enabled` member from the term struct, and just check `term->conf.tweak.sixel` in the CSI/DCS handlers.
jvoisin marked this conversation as resolved
jvoisin force-pushed disable_sixels from 1fdca759c3 to a2632d87c3 9 months ago
jvoisin requested review from dnkl 9 months ago
dnkl reviewed 9 months ago
terminal.h Outdated
bool scrolling:1; /* Private mode 80 */
bool use_private_palette:1; /* Private mode 1070 */
bool cursor_right_of_graphics:1; /* Private mode 8452 */
bool enabled:1; /* Enable sixel support */
Owner

Drop this?

Drop this?
jvoisin marked this conversation as resolved
jvoisin force-pushed disable_sixels from a2632d87c3 to 053beb884c 9 months ago
Collaborator

@jvoisin I've tested again with chafa and confirmed the old DCS buffering lag is gone. The patch itself also LGTM.

Would you mind amending the commit message to be a bit more descriptive of the changes? Something like e.g. "config: add tweak option to allow disabling sixels".

@jvoisin I've tested again with chafa and confirmed the old DCS buffering lag is gone. The patch itself also LGTM. Would you mind amending the commit message to be a bit more descriptive of the changes? Something like e.g. "config: add tweak option to allow disabling sixels".
jvoisin force-pushed disable_sixels from 053beb884c to 341e71281f 8 months ago
jvoisin force-pushed disable_sixels from 341e71281f to 5d2dfde186 8 months ago
jvoisin force-pushed disable_sixels from 5d2dfde186 to 097de668a3 8 months ago
craigbarnes approved these changes 8 months ago
craigbarnes left a comment
Collaborator

@jvoisin Thanks, LGTM!

@jvoisin Thanks, LGTM!
dnkl requested changes 8 months ago
dnkl left a comment
Owner

Looks good to me as well, but I do think this deserves a changelog entry :)

Looks good to me as well, but I do think this deserves a changelog entry :)
jvoisin requested review from dnkl 8 months ago
dnkl approved these changes 8 months ago
dnkl left a comment
Owner

LGTM! Please squash and rebase, and we're good to go!

LGTM! Please squash and rebase, and we're good to go!
jvoisin force-pushed disable_sixels from a3a6e82239 to 319ceee67f 8 months ago
dnkl merged commit 58a1ffe724 into master manually 8 months ago
Owner

Thanks!

Thanks!

Reviewers

craigbarnes approved these changes 8 months ago
dnkl approved these changes 8 months ago
ci/woodpecker/pr/woodpecker Pipeline was successful
The pull request has been manually merged as 58a1ffe724.
Sign in to join this conversation.
Loading…
There is no content yet.