Use iv.py to display images in terminal

[MatanZ]

This replaces kitty discovery and terminal image display with a module I wrote to support graphics terminal.

This module supports three graphics protocol (sixel, iterm2, kitty), and recognizes support by relevant escape sequences, rather than by checking terminal name.

I tested this commit in konsole (22.04), xterm, wezterm and kitty. It should work in any terminal that supports any of those protocols.

[dotcs]

Thanks a ton for those changes, @MatanZ! It will take me a few days to look through and understand the proposed changes, so please stay tuned.

[dotcs]

Thanks again for this PR! While reviewing it, I found a couple of issues we should have a look into. I've merged changes from develop into this branch that introduces a GitHub workflow to automatically lint and run a static code analysis on the branches. This should also happen automatically on the next commit in this PR. Meanwhile, can you please run those two commands manually and fix the issues they report? Code formatting can be done automatically with black (see docs) while pyright found a couple of typing issues that need to be resolved.

$ poetry run python -m black --check --diff .
$ poetry run python -m pyright

Please also specify in detail if you have used other code for this PR. I found this code to be very similar to the serialize_gr_command function in this PR. Eventually please double check that any submitted code in this PR is compliant with the current license (MIT).

In the meantime I try to better understand the code and the mechanics behind it. ;)

[MatanZ]

Please also specify in detail if you have used other code for this PR. I found this code to be very similar to the serialize_gr_command function in this PR. Eventually please double check that any submitted code in this PR is compliant with the current license (MIT).

This is from here. I never considered the license, but considering it is a small example in what the author hopes will become a standard, I expect it to be freelt reusable without limitation

[MatanZ]

I believe this addresses all your comments. I removed the example code from kitty with my own, so no question of copyright. Similarly for the KBhit class.

I also corrected the issues pointed to by pyright.

[dotcs]

Thanks for updating the PR. In the meantime I think I got a better understanding of those changes and of the ANSI escape codes that you have used. I'll add a couple of commits to this PR in order to improve typing support, and I'll add some docstrings. Maybe we can also add some tests for the critical parts of the code. Please don't force-push any longer in this PR, so that we both can commit without issues. 👍

[MatanZ]

Why are some of the typer help strings multilined with """, and another with ("", "")?

[dotcs]

Why are some of the typer help strings multilined with """, and another with ("", "")?

Thanks for the hint. That's not intentional. We should stick with a single way to multiline strings. The latter one is probably more convenient because there are no issues with whitespace at the beginning of lines, so I would like to pick this one. WDYT?

[dotcs]

Thanks for fixing the wrong behavior and your detailed explanation. 👍

With this implementation I have an issue when using a terminal that falls back to the convert command. poetry run xkcd show --comic-id 218 --width 100 triggers the cmd ['convert', '/tmp/tmpck1g5hmr/218.png', '-geometry', '100x-1', 'sixel:-'] under the hood which will not work, because the geometry 100x-1 is not defined. I guess instead 100x should be used.

Should we cover this case specifically or is this a general problem with the -1 value also in other branches?

[MatanZ]

This corrects sixel behaviour with convert.

There is still an issue with libsixel, as this library has no option for keeping aspect ratio, so if given both width and height (as we do now), it fits exactly, changing the aspect of the image.

[dotcs]

I've added a couple of tests to ensure that most critical parts are working as expected and we have some kind of documentation for the behavior. Would be great if you can look through the tests and let me know if you spot any issues, @MatanZ. Otherwise I'd consider this PR to be ready to merge. Again thanks a ton for your hard work! 🥳

[MatanZ]

I am from the if it compiles, ship it camp, so I know very little about testing infrastructure and such things. The functions do seem like they test what is expected, but I do not see anything that uses the functions in iv_test.py. I expected there to be the python equivalent of make test which runs the tests and reports success.

[dotcs]

I am from the if it compiles, ship it camp, so I know very little about testing infrastructure and such things.

I'm more from the camp that tries to sleep well by ensuring that the code in production runs. Not for all things, but at least for the more complex parts. I still have to add tests to the original code base though. 😂

Sorry for not being more specific on how to execute the tests. For now please run poetry run python -m pytest --cov="xkcd_cli/" --cov-report term --cov-report html to run all tests and create a html report of the code coverage. You might need to update packages with poetry install first, since I added some packages while working on this PR.