kovidgoyal / kitty

Cross-platform, fast, feature-rich, GPU based terminal
https://sw.kovidgoyal.net/kitty/
GNU General Public License v3.0
24.15k stars 972 forks source link

Image protocol extension for heavy image preview scripts #326

Closed mark-dawn closed 6 years ago

mark-dawn commented 6 years ago

I have implemented some code to allow ranger to leverage kitty's image previewing abilities. I was ready to ship it when I found a bug, where I would fail sometimes to catch back kitty's response to an upload, leaking the control sequence into stdout and corrupting curses output. To catch the responses I use the same method as in kitty icat, and since this bug occurs unreliably only when quickly switching between many previews I am guessing that the problem is external to both ranger and kitty and probably due to late wakeups from the select call. (This seems to affect icat too, using it on a big list of images seem to screw up something, as my sxhkd or the script I use to manage windows stops working, froze kitty, or on a single test froze the whole system, but I can't be 100% sure that those issues are related)

To fix this, my idea would be a small extension on kitty's image protocol. Basically a new key command that when set would instruct kitty to just not send responses for the current command. I would propose q (for quiet) with a default of 0.

In term of implementation, a quick scan and grepping around the kitty source pointed me to the grma_handle_command function in graphics.c. This is called in screen.c and his return value is used as the response, so I would propose to wrap the return statement in a if else block, and return NULL is the new flag is set, preserving the functionality of the body of the function. I guess alongside this the parser for the command string would have to be updated to produce a struct with this new field.

If those changes are alright with you I can try implementing this myself and submit a pull request.

kovidgoyal commented 6 years ago

I dont mind adding a quiet command to the protocol, however, it would be nice to be able to track down the cause of the issue. I dont see how a late wakeup in select could cause it, unless you are reading the responses in parallel? The only way I can see it happening is a partial read of a command sequence. In pseudo-code:

while True:
   data = read_some_data()
   if expected_response in data:
      break

With this code, what can happen is that suppose kitty sends two responses A and B, of which A is the expected response. Then read_some_data() could read all of A and part of B before returning. This means that the rest of B would get printed to stdout, leading to the symptoms you describe. The fix would be to print out any leftovers in data after reading A (although I dont know how that works with rangers event loop).

kovidgoyal commented 6 years ago

And I should note that in the example above B need to be an image related command, it could be anything, from a keypress to a DECQRSS response.

mark-dawn commented 6 years ago

I see what you mean, I am just doing a read() and discarding the data. I don't see how it would happen more often when calling the drawing function (that prints the APC to stdout and then wait for the answer on stdin in a nutshell) with high frequency. I guess I can check for the trailing \ at the end of kitty response to stop and see what happens.

EDIT: well, I'll be damned, it works now. Thanks for the input, I was overthinking this!

kovidgoyal commented 6 years ago

Cool, glad I could help. Let me know when your code is released, I am always interested to see kitty's graphics protocol being used in the wild.