Add support images in kitty terminal emulator #104

fspillner · 2019-10-27T21:41:27Z

It resolves the open issue #65.

Demo:

swsnr · 2019-10-28T08:39:01Z

@fspillner Hey great to meet you here! How are you? Learning Rust too?

Thanks a lot for this pull request. I'd definitely like to merge it, and the code looks good on a first glance. It's quite a bit though, and it'll take me a while to really review it in detail, a month or more. If you'd like to wait for a review please be patient; if you don't hear anything by the end of November, ping me :)

On the other hand, I guess you'd like to have this feature release as soon as possible and merge the pull request now, before a review. In this case we'd just have to remove a few road-blocks, and then we'd merge before I review this pull request fully. In this case I'd expect that you address any of mine concerns at a later point, in a follow-up pull request, so there'd be some kind of obligation I'd like to put on you to do this :)

I'm fine with either way, so the choice is entirely yours, to do whatever you feel more comfortable with.

In any case, I'm giving this PR a short overall review now, so that we address the basics first :)

Thanks a lot :)

swsnr

Please

rebase on master to solve the conflict,
add Kitty to the features table in README, and
add the "kitty" feature to the ci.yml and release.yml workflows (similar to how terminiology is added there).

Please also improve the API documentation. In particular add more (I know I didn't do this myself, but I'd really appreciate if every function had an API doc 😇 ), and expand the existing comments. In particular, please do not just refer to an external link, but also add a brief summary of the contents of the external link and explain how they related to the documented function or type, to put the information of the link into the context of the documented code.

Also please edit documentation wrt to punctuation and style; make sure it's consistent with the existing documentation. Make sure to end all sentences with proper punctuation and use "Do something" style, not "Does something". And use Markdown syntax for links and inline code/code blocks :)

Finally I must admit that I couldn't really follow the code in the kitty module: I presume the KittyCommand trait and related structs abstract over the Kittys proprietary escape codes? It's a lot of code. Also some things look confusing: There's KittyCommandPairs, but KittyCommand::cmd_pairs doesn't use it?

I kinda feel that this abstraction obscures what's going on; at least to me it's not easy to understand the code and assign a mental model to each of the different types. I don't know, but can we perhaps implement this in a more straight-forward, and less abstract way? Take a look at the other terminal backends: They have way less code.

Cargo.toml

src/terminal/kitty.rs

It resolves the open issue #65.

…ts of documentation.

fspillner · 2019-10-29T23:04:44Z

@lunaryorn Hey! Thanks for the kind words here. I'm good and yes, I'm learning Rust. I'm still a beginner and so I'm looking forward to your feedback about my code. I'm making my progress forward by making contributions to some Rust projects, especially to the good one's like here. It has good project size to get fast in.

Thank you very much for giving me a detailed introduction what you do expect from me for this PR. I don't aim for a merge in a short-time - but I think I could satisfy the obligation that I should fix the issues after you've reviewed my PR in detail in next weeks. You should ping me then but please give me one week room for doing that.

Let's talk about the code itself.

Code abstraction
You made the point. As I told you, I'm learning Rust and the project gives me a good playground to explore and I've really lost in abstraction here - as the project doesn't expose the API of kitty's image rendering capacity outside so it doesn't make sense to abstract it over. I've reworked it and simply the code down as possible. I hope you like it now.

Documentation
I've made the changes to the API documentation and apply the style. I've reduced the amount of external links in return of a briefly explanation how kitty renders the images in general.

Git Workflows
I never worked with git workflows but I browsed the documentation to get the big picture and I've made the changes to ci.yml and release.yml and I hope that's exactly what you wanted.

Resolving conflict
I've rebased the upstream with my master and resolved the conflict.

And again: thanks a lot for the warm welcoming.

swsnr · 2019-11-05T18:47:04Z

@fspillner Thanks for the update. I haven't forgotten you :) but I just haven't found time to look at your changes yet 😔 I'm sorry, I'll try to take a look as soon as I can, but my I haven't got much time currently, so please be patient. If haven't heard anything from me in two weeks, please be so kind as to ping me here 😇

swsnr · 2019-11-22T06:47:02Z

@fspillner Hey, just a heads up, I haven't forgotten this, and hope to get to work on it this weekend or the next ❤️

swsnr

Hey, now I finally got to take a look at this. Great work, love it :) I'm very sorry for taking so long 😊

There's just a few minor changes left, if you could take look :) It's really just some extra polishing, if you're short on time we can also merge it as it is now, and polish later :)

swsnr · 2019-11-25T07:11:14Z

src/lib.rs

@@ -17,7 +17,7 @@
 //! Write markdown to TTYs.

 #[cfg(feature = "resources")]
-use url;
+extern crate url;


Why's this change? Shouldn't extern crate be redundant in Rust 2018 edition?

Is it perhaps because we now have url.rs here? If so, perhaps we can do things a bit different to avoid this naming conflict :)

Could you try to merge our url.rs with the existing resources.rs?

Yes - the internal module url.rs causes that naming conflict. I'll merge it into the existing resources.rs.

swsnr · 2019-11-25T07:12:35Z

.github/workflows/release.yml

@@ -14,7 +14,7 @@ jobs:
        include:
          - os: ubuntu-latest
            target: x86_64-unknown-linux-musl
-            flags: --no-default-features --features terminology
+            flags: --no-default-features --features terminology,kitty,remote_resources


Thanks :) Did you check the ci.yml workflow if there's a similar build where you could add kitty, so that it gets tested regularly and not just on a release? Sorry for asking this; I don't remember which builds I set up in ci.yml 😇

I've checked that, too. I just added the kitty feature to the test_linux job.

swsnr · 2019-11-25T07:20:00Z

src/lib.rs

+                if let Some(url) = ctx
+                    .resources
+                    .resolve_reference(&link)
+                    .filter(|url| access.permits(url))


That's the 3rd block which has this piece of code; it's duplicated in every image block here :(

I'll need to make a mental note to refactor this a little after this pull request is merged :)

No need for you to take any action here though :) Let's merge this as is :)

Good point.

src/terminal/kitty.rs

swsnr · 2019-11-25T07:22:13Z

src/terminal/kitty.rs

+        .arg("icat")
+        .arg("--print-window-size")
+        .stdout(Stdio::piped())
+        .spawn()?;


As far as I understand we're doing this for every image? Let's make a mental note for another refactoring here: Move this to the entry point of lib.rs or even main.rs and compute the window size only once, then pass it down in Context :)

Your assumption is correct - we need to find a way how to integrate the terminal dimension optionally, as if the terminal supports it.

src/terminal/kitty.rs

swsnr · 2019-11-25T07:30:37Z

@fspillner Hey I also invited you as collaborator :) No duties and no expectations attached, though :)

…ct with the url crate

See GH-65 and GH-104

swsnr · 2019-12-14T19:01:40Z

@fspillner Merging this broke something on CI. Looks like openssl gets pull in, even though it shouldn't.

I'll take a look but not before next year—Christmas and family don't allow before ^^ If you like I'd appreciate if you could also take a look meanwhile :) but again, no obligation. Enjoy Christmas and have a happy new year :)

Fixes clippy issues from GH-104

Fixes Clippy lints from GH-104

Fixes lint from GH-104

laoshaw · 2021-11-14T17:58:32Z

if I need pager for the output, it does not render the image anymore: mdcat -p with-image.md. mdcat with-image.md works though.

swsnr · 2021-11-14T18:37:43Z

That's expected and won't change unless you find a pager which understand inline images.

This was present since the original implementation in #104 but it looks as if it was just copied from code in kitty's documentation. I don't think we really need to flush all buffers here.

swsnr requested changes Oct 28, 2019

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/terminal/kitty.rs Outdated Show resolved Hide resolved

fspillner added 6 commits October 29, 2019 10:14

Add support images in kitty terminal emulator

1a2e636

It resolves the open issue #65.

Make the dependency on image optional

bd15fd9

Simplify the way of rendering the image bytes in kitty and improvemen…

de29ec4

…ts of documentation.

Added kitty to the features table in README.md

1a16e5a

Added kitty in ci and release workflows

2e0e941

Fixed the alignment of the features table in README.md

1f98277

Merge branch 'master' into master

43225f5

swsnr requested changes Nov 25, 2019

View reviewed changes

swsnr and others added 5 commits November 27, 2019 19:37

Merge branch 'master' into master

2ce0531

Updated Cargo.lock added

1a32e9f

KittyDimension added for the encapsulation of width and height

12a4249

Merge the url module into the resources module to avoid naming confli…

5a1f913

…ct with the url crate

Merge branch 'master' into master

96ba2fa

swsnr merged commit 13270d3 into swsnr:master Dec 14, 2019

swsnr mentioned this pull request Dec 14, 2019

Support images in Kitty #65

Closed

swsnr added a commit that referenced this pull request Dec 14, 2019

Update changelog

af1ac33

See GH-65 and GH-104

swsnr added a commit that referenced this pull request Dec 15, 2019

Remove redundant field names

41f8845

Fixes clippy issues from GH-104

swsnr mentioned this pull request Dec 15, 2019

Fix build #108

Merged

swsnr added a commit that referenced this pull request Dec 15, 2019

Use or_else to avoid constructing unused values

5f12bbc

Fixes Clippy lints from GH-104

swsnr added a commit that referenced this pull request Dec 15, 2019

Definitely write all bytes

da0df7b

Fixes lint from GH-104

swsnr added a commit that referenced this pull request Apr 21, 2023

Don't flush every chunk

9b5f2e0

This was present since the original implementation in #104 but it looks as if it was just copied from code in kitty's documentation. I don't think we really need to flush all buffers here.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support images in kitty terminal emulator #104

Add support images in kitty terminal emulator #104

fspillner commented Oct 27, 2019

swsnr commented Oct 28, 2019

swsnr left a comment

fspillner commented Oct 29, 2019 •

edited

Loading

swsnr commented Nov 5, 2019

swsnr commented Nov 22, 2019

swsnr left a comment

swsnr Nov 25, 2019

swsnr Nov 25, 2019

fspillner Nov 27, 2019

fspillner Nov 27, 2019

swsnr Nov 25, 2019

fspillner Nov 27, 2019

swsnr Nov 25, 2019

fspillner Nov 27, 2019

swsnr Nov 25, 2019

fspillner Nov 27, 2019

swsnr commented Nov 25, 2019

swsnr commented Dec 14, 2019 •

edited

Loading

laoshaw commented Nov 14, 2021

swsnr commented Nov 14, 2021

Add support images in kitty terminal emulator #104

Add support images in kitty terminal emulator #104

Conversation

fspillner commented Oct 27, 2019

swsnr commented Oct 28, 2019

swsnr left a comment

Choose a reason for hiding this comment

fspillner commented Oct 29, 2019 • edited Loading

swsnr commented Nov 5, 2019

swsnr commented Nov 22, 2019

swsnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swsnr commented Nov 25, 2019

swsnr commented Dec 14, 2019 • edited Loading

laoshaw commented Nov 14, 2021

swsnr commented Nov 14, 2021

fspillner commented Oct 29, 2019 •

edited

Loading

swsnr commented Dec 14, 2019 •

edited

Loading