Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OSC support, add OSC 8 (anchor) support to the public api, add a … #85

Closed
wants to merge 2 commits into from

Conversation

cbcmg
Copy link

@cbcmg cbcmg commented Jun 21, 2022

…simple implementation in VirtualTerminal, and use in 'input' example. Fixes #84.

Hope this is useful for you.

Running in iTerm...

image

Also see the definitive reference https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda

40ft added 2 commits June 22, 2022 09:29
…simple implementation in VirtualTerminal, and use in 'input' example. Fixes varabyte#84.
…rCodeConverter. This allows VirtualTerminal to process sequences produced outside of kotter, e.g. by jansi.
@cbcmg
Copy link
Author

cbcmg commented Jun 22, 2022

sorry, bit of a github noob. That second commit is not part of this PR (but merge it if you like). Should have created a branch first.

@bitspittle
Copy link
Contributor

Oh no I didn't notice this until just now. 😱 Sorry to sit on your PR for so long. I will take a look this week

@bitspittle
Copy link
Contributor

So I finally got a chance to look into this at last. I just wanted to thank you again, so much, for sharing the code you had.

I've finally gotten on top of things so I could finally start porting it over (the approach is mostly the same although I tweaked a few things here and there due to my familiarity with some of the nuances of the codebase :).

Feel free to see what I've got so far here if you're curious to see how it compares: main...feature+osclink

At this point I've hooked up everything except for the virtual terminal, and I want to add tests for it too (Kotter now has support for writing tests! It didn't went you first wrote this PR).

I'll confirm my version works on Mac later, but it does work on Windows. However, it doesn't work for either of the Linux consoles I tried (even though one is Gnome terminal and it should have been supported on that one for years).

I'm a little nervous about releasing a feature that you might think works just fine on your terminal but then it won't work for a bunch of users on Linux. Maybe it's OK as long as I just document the limitation. I need to sleep on it and another day to play around with stuff. (I'm deciding whether I want to slip this into 1.0.0 or not)

Once I have this working, I'll close this PR as rejected, since I went in a slightly different direction in some ways from your approach, but I will definitely give you credit for this feature in the release notes and commit message.

@bitspittle
Copy link
Contributor

And I just checked on a default Ubuntu install, and it works there. So it might just be my own local flavor of Linux that is busted. I'm pretty convinced at this point I want to get this into 1.0.0 in that case.

@bitspittle
Copy link
Contributor

BTW, just want to emphasize that the code diff I shared is a WIP. So it may change as you keep refreshing it.

I definitely appreciate how thorough you were with the feature.

I've actually decided in 1.0 to only allow a very constrained form of declaring links (e.g. link("https://example.com", "click this example")), but if you're aware for why I should allow the more open ended version (e.g. link("https://example.com") { text("click this example")) let me know! I understand it's technically possible to do so, but I'm not sure in practice the extra complexity to the API is worth the flexibility.

I opened #91 to track this decision.

@bitspittle
Copy link
Contributor

Closing this pull request since the feature is now in.

Looking back over my old code, I had a lot of warts, e.g. in the Ansi section. Hard to remember what I was thinking! Thanks for navigating through all that stuff.

@bitspittle bitspittle closed this Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OSC hyperlinks
3 participants