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

OSC hyperlink support #3904

Closed
wants to merge 9 commits into from
Closed

OSC hyperlink support #3904

wants to merge 9 commits into from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jul 17, 2022

Weekend project 🤓

Fixes #1134

Test command: echo -e '\x1b]8;;https://github.com\aGitHub\x1b]8;;\a'

TODO

  • Resolve TODOs
  • How do wrapped lines get handled when reflow occurs?
  • Share underline status between links with the same id and uri
  • General clean up
  • Tests
  • Webgl renderer
  • Hover/activate handlers in API

@Tyriar Tyriar added this to the 4.20.0 milestone Jul 17, 2022
@Tyriar Tyriar self-assigned this Jul 17, 2022
@jerch
Copy link
Member

jerch commented Jul 18, 2022

@Tyriar Why dont you use the extended attributes for marking the cells? This would automatically deal with line wrapping, reflow and such as it is an attribute directly on a cell like SGR. It also would avoid bad runtime from the event dispatching of onPrintChar for every single character.

For reference: #2904 contains fully working hyperlink code using extended attributes. If added into core the needed changes are very few. Only missing feature back then was the multi range support of the linkifier, which is needed for proper spec conform behavior (and which you refused to add to the new linkfier).

@Tyriar
Copy link
Member Author

Tyriar commented Jul 18, 2022

@jerch I completely forgot about your PR here 😅

Why dont you use the extended attributes for marking the cells? This would automatically deal with line wrapping, reflow and such as it is an attribute directly on a cell like SGR.

My thought process was trying to make it a very standalone feature as opposed to being integrated into everything, but I ended up hitting a bit of a wall how to do it properly in the webgl renderer because I didn't do the attribute approach. It turns out your achieves better in this respect since it's an actual addon.

Correct me if I'm wrong but we never ended up getting underline color/style working in the renderers, just the buffer model? Seems like we could do this in webgl by specifying the number of bits for each extended attribute and then making glyphs accessible via 3 keys (fg, bg, extended packed into a single number) here:

https://github.com/Tyriar/xterm.js/blob/a8405b86defb32743c3834a19741cbc117bbda5e/addons/xterm-addon-webgl/src/atlas/WebglCharAtlas.ts#L175-L181

For reference: #2904 contains fully working hyperlink code using extended attributes. If added into core the needed changes are very few.

When I resume this work I think it'd probably be better to base it off yours and bring over any useful bits from this PR.

Only missing feature back then was the multi range support of the linkifier, which is needed for proper spec conform behavior (and which you refused to add to the new linkfier).

I don't recall this discussion but I'd like to support the shared underline for all link ranges with the same id before pushing this in.

@jerch
Copy link
Member

jerch commented Jul 18, 2022

Correct me if I'm wrong but we never ended up getting underline color/style working in the renderers, just the buffer model?

Ah yepp, the renderers never got extended by the different underline styles/colors. I only had a very early stub back then for DOM renderer. Main issue was to get around the ugly dashed/dotted render output of the DOM renderer (basically unusable with plain DOM styling - see #1145 (comment)).

So yeah - I did not address the underline issue at all in the hyperlink draft, I just used normal underline similar to the weblink addon. With proper underline styles in all renderers a dotted/dashed default repr turning into a full underline on hover might work for a better UX.

When I resume this work I think it'd probably be better to base it off yours and bring over any useful bits from this PR.

Sure, well it would get the reflow/wrapping done automatically with less perf penalty, but still lacks the renderer extensions for other underline styles. There are a few more other bits missing, like resetting hyperlink state from RIS/DECSTR/term.reset and such. And if it goes into core, then the custom linkProvider/schemeHandler prolly can be written much shorter or simply kept in sync with other link settings.

I don't recall this discussion but I'd like to support the shared underline for all link ranges with the same id before pushing this in.

Yeah, thats all it needs to stay spec-conform with OSC 8.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 18, 2022

So yeah - I did not address the underline issue at all in the hyperlink draft, I just used normal underline similar to the weblink addon. With proper underline styles in all renderers a dotted/dashed default repr turning into a full underline on hover might work for a better UX.

Seems like I should tackle underline style/color rendering before continuing on this. I don't mind so much if the underlines look a little ugly in the dom renderer if that's just how the browser renders the underlines. As long as webgl looks great as it's the primary renderer

@jerch
Copy link
Member

jerch commented Jul 18, 2022

Seems like I should tackle underline style/color rendering before continuing on this.

While technically not really needed to get OSC 8 support, its prolly a good idea, as those underline styles/colors are pretty widespread in cmdline applications already.

Edit: Once the styled underline is in place, the hyperlink code could simply add underline-dashed | dotted when adding chars in hyperlink state as default text decoration, and turn it into a full underline on hover. Something like that get some distinction to normal text output ...

@Tyriar
Copy link
Member Author

Tyriar commented Jul 25, 2022

With #3921 we'll have full support for all underline styles/colors. I tried to merge master into #2904 but it's pretty old and I struggled with that.

So based off #3921 I'll bring in the best parts of both PRs, probably opting for the addon + extended cell data approach. Also the buffer doesn't actually need to know about the link if we change the underline styles, so this could be in extended cell data but not need to be included in the new packed ext which is used as the glyph cache key.

@Tyriar Tyriar added the reference A closed issue/pr that is expected to be useful later as a reference label Jul 25, 2022
@jerch
Copy link
Member

jerch commented Jul 25, 2022

@Tyriar You already fixed the styles in the renderers - wow that was really fast. About #2904 - yeah, thats a very old old PR, might be easier to just pull some ideas from the code there, instead of messing around with the whole PR?

@Tyriar
Copy link
Member Author

Tyriar commented Jul 25, 2022

Was a particularly productive weekend lol

@Tyriar Tyriar modified the milestones: 4.20.0, 5.0.0 Jul 28, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Jul 31, 2022

Closing for now, will use as a reference for the next try

@Tyriar Tyriar closed this Jul 31, 2022
@Tyriar Tyriar removed this from the 5.0.0 milestone Jul 31, 2022
@Tyriar Tyriar mentioned this pull request Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reference A closed issue/pr that is expected to be useful later as a reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support hyperlink ansi escapes
2 participants