Kitty: Test for protocol support & wait for "OK" on print#79
Conversation
26be8a9 to
f4eec2c
Compare
|
I had 2 mistakes that lead me to force-push more than i wanted:
Though the infinite-loops still continue to exist if there is some unexpected environment, should we maybe add some kind of fallback (timeout and/or too much response data)? (Note that this is the same behavior as before the PR currently) |
| write!( | ||
| stdout, | ||
| "\x1b_Gf=32,a=T,t=d,s={},v={},c={},r={},m=1;{}\x1b\\", | ||
| "\x1b_Gf=32,a=T,t=d,s={},v={},c={},r={},i=10,m=1;{}\x1b\\", |
There was a problem hiding this comment.
Additionally, while i was at it and noticed it, i made it so that printing a image waits until the terminal responds, as otherwise it could lead to a race condition, which in viu could manifest as never actually displaying the image at all.
To do this response, i had to add i=10 (image identifier), as otherwise kitty seemingly does not respond at all.
I don't understand when that happens.
As a result of using the same id of 10, only the last picture is shown. If you run viu a few times and scroll up, they will have been replaced.
There was a problem hiding this comment.
Additionally, while i was at it and noticed it, i made it so that printing a image waits until the terminal responds, as otherwise it could lead to a race condition, which in viu could manifest as never actually displaying the image at all.
I don't understand when that happens.
The state i was in when starting this PR was that the Kitty terminal was always detected for Remote, even though it should have supported Local too.
After i had gotten the Kitty terminal to be detected for Local due to the file "NotFound" error (see commit fix(kitty): dont error on non-existed temporary file after print), i had noticed that sometimes for (at the time) unexplainable reasons it just sometimes would print nothing and other times will when running viu.
Then i tried to just see if catching a response from the terminal would be available, but (without the i= set yet) the terminal did not respond, but due to this i noticed that it was now drawing 100% (though with a bit of delay since starting the application). So instead to verify this was a race-condition, i added a simple std::thread::sleep(10s) and it also "just worked" consistently.
After some fiddling around, i had found that when i added i= to the command line, the terminal did start responding with OK which was always after the image had been displayed, so i added that.
I did not know that multiple images should be displayable at once via viuer though, what would you propose to fix this and maybe still be user controllable? Should we maybe expose setting the ID to the user, with a default provided in Config(with naming along the lines of kitty_id)? (this could implicitly and in a indirect way allow #28 for kitty)
Note: i was (and still am for this PR) running a locally modified viu which uses the local viuer and disable all possible other protocols other than kitty.
Due to kitty terminals deleting the file after fully reading it in known temporary directories.
…o stdout(commands to terminal) Also add a helper replay utility.
… "OK" before returning as otherwise it could be a racecondition in for example in "viu", which can exit before kitty has displayed the image.
…h "OK" before returning
And "Clone" & "Copy" where it makes sense
e431c08 to
2227cac
Compare
|
@atanunq I think this PR is still ready, or do you still have problems with it like mentioned in #79 (comment)? Should i move some code (like the |
and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr atanunq#79.
and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr atanunq#79.
and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr atanunq#79.
|
I have decided to split this PR into multiple parts (like mentioned in my last comment), so i will convert this to be a DRAFT again. |
and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr atanunq#79.
I think i found a workaround that does not require a identifier ( With setting The same is true for Konsole. I found this sequence while trying to look if iterm2 has a "check protocol support" thingy too, via # Report device status. Responds with esc [ 0 n. All terminals support this. We
# do this because if the terminal will not respond to iTerm2's custom escape
# sequence, we can still read from stdin without blocking indefinitely.
echo -n '�[5n'I guess alternatively we could also use the PS: it turns out there still is no correct way to detect iterm2 protocol support (and only |
and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr atanunq#79.
* feat: add ability to test stdin(response from terminal) in addition to stdout(commands to terminal) Also add a helper replay utility. * refactor(kitty::has_local_support): take generic stdin & stdout parameters for testing use * test(kitty): add test to know what keys are consumed and how the function will behave. Mostly to showcase "ReadKey" trait usage. This implementation tests against known bugs, see pr #79. * test(sixel): add test to know what keys are consumed * style(printer): use proper doc-comments * refactor(read_key): move module into "printer/" * style(read_key): remove "pub(crate)" and just use "pub" * Reorder arguments * Reorder print_file arguments * Reorder helper arguments * Reorder kitty helper arguments --------- Co-authored-by: atanunq <atanas.yankov98@gmail.com>
This PR implements actually protocol testing for the Kitty Graphics Protocol, instead of testing for known
TERMenvironment variables, which could be misleading and lead to #70.Additionally, while i was at it and noticed it, i made it so that printing a image waits until the terminal responds, as otherwise it could lead to a race condition, which in
viucould manifest as never actually displaying the image at all.To do this response, i had to add
i=10(image identifier), as otherwise kitty seemingly does not respond at all.And finally, also fix a issue of all functions that interact with a temporary file and attribute
t=treturning a error, when actually everything was working correctly. This was due to Kitty terminals on temporary files deleting after finishing reading them:This fixes:
Additionally, from my testing, this now supports KDE-
Konsolewith kitty protocol, though it only seems to support theRemotedisplay method (the other returns a error withNOT SUPPORTED).TL;DR:
TERM