fix: reset background color after candidates are flushed out (closes #494)

[c4eater]

The initial problem is: Under a certain combination of startup options, Skim does not reset the background color in the end of the execution. To reproduce:

echo "AAAA\nBBBB" | SKIM_DEFAULT_OPTIONS="" sk --no-info --no-clear-start --ansi --height=5
# Complete to "AAAA" and observe the background selection color leaking into the tty.

Printout of candidates on my system:

And the result is (note the selection's bg color corrupting the output):

Further investigation showed that this particular set of options (which is quite common btw) results in Skim not adding an ANSI control sequence, namely sgr0, that resets the background in the end.

The fix simply adds the missing color reset in the end of Screen::present().

After the fix, the background color in the same scenario is consistent:

Note that I am not adding any test as I found it incredibly hard to programmatically test this kind of behavior, which would require:

• Launching a full-fledged skim instance in a real tty (Skim::run_with) - as opposed to unit-testing a Skim struct.
• Some kind of probing of the background color between the EOL of the last completion candidate and the next line, which is beyond tuikit's capabilities.
• Relying on the calling tty capabilities which may vary (eg unclear what happens if the test is run from a monochrome kernel tty).

[LoricAndre]

Hi, thanks for opening this ! About the tests, check the e2e crate, which allows us to test skim by running a tmux instance. You might be able to test this behavior

[c4eater]

Oh ok, didn't see there is a testing infra. No problem I'll try writing a test...

[c4eater]

Upd: Ok tried writing a test similar to issue_361_literal_space_invert et al, only to discover that the tester, TmuxController, apparently sees the output (eg the lines argument in until()) as plain strings, with no coloring information. Guess it's expected, but it prevents me from testing my particular scenario.

Maybe a manual test would suffice? My starting comment shows a simplistic test whose output would be different with and without the proposed commit.

Thanks

[c4eater]

Any feedback pls? This bug is rather irritating and it corrupts colored output fairly often. IMO a manually tested one-liner is better than no fix at all.

[LoricAndre]

This is being completely rewritten. Sorry for the delay, but as I do not see any E2E tests in the PR and the CI blocks on its title, I don't want to merge this blindly.

[c4eater]

What do you mean by being rewritten?

I can just repeat myself: Background color beyond EOL cannot be programmatically tested. Even if I manage to add colored text support to TmuxController as a part of my change, there is no way to test the bg color beyond EOL using that support. The 1st character on the next line comes in normal color as the starting comment shows.

I've provided a manual test scenario and this change is not making any existing test fail. Imo that's all I can do here. (I have properly renamed the PR though) Also @leo-arch could probably confirm that this fix addresses the issue he reported.

[leo-arch]

Hi @c4eater. Quick test: no, the issue is still there. Run ls | ./sk --height=20, press Esc, and the screen is still not properly cleared.

[c4eater]

@leo-arch sorry to hear that=( Its odd but I cannot reproduce your problem. As soon as I switch to the fix/494 branch and run the patched skim, the past-EOL background is good for just any colored list of candidates (ls output or anything)

Can you please double check the following:

• You are unsetting SKIM_DEFAULT_OPTIONS before any testing. The fact that you haven't specified the "--ansi" option in your command and are still able to observe colors (not raw ANSI escape sequences) in skim output likely means that your local SKIM_DEFAULT_OPTIONS are leaking into the test environment. That said, for the sake of definiteness can you pls rerun the same test with ls | SKIM_DEFAULT_OPTIONS="" sk --ansi --height=20 <...> and check if the fix works on it. If the fix doesn't work I'd like to know a precise SKIM_DEFAULT_OPTIONS string that triggers the problem in fix/494.

• You are compiling the proper branch: fix/494.

Also can you pls share your system specs because this problem might be platform dependent. Here is a replay of 2 versions of skim, with and without fix, on my system, maybe that would give a hint on what is different in your case.

Thx in advance...

[leo-arch]

Thanks for the detailed build procedure @c4eater. I can confirm the issue is still there provided you run sk without --no-clear-start. The exact command I'm using is: ls -A --color=always ~ | SKIM_DEFAULT_OPTIONS="" ./sk --height=20 --ansi.

[c4eater]

Can't reproduce that either =( On my side, bg color is properly displayed in black in fix/494 even if I delete --no-clear-start from the list of options.

Afraid I can't proceed without a repro VM / environment. Appreciate if you provide one, like eg a github codespace or a dockerfile.

Also I have just learned that skim is in the middle of a migration from tuikit to ratatui, @LoricAndre is that what you meant by full rewriting? This bugfix might be unneeded then, as my amendment is done to skim-tuikit.

[LoricAndre]

Yes exactly, any such glitches should naturally get resolved with the migration. It's taking longer than expected, sorry for the delay, but I can assure you it's coming

[leo-arch]

@c4eater, the issue I reported is not about the background color, but rather about the screen not being properly cleared at exit when --height is set.

As to my environment, I'm running fix/494 on an up to date Archlinux system, Wayland, sway, and foot. If you need anything else just let me know.