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
Support semi-colons in OSC 8 hyperlink URIs #5003
Conversation
…etHyperlink().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josiahhudson Thx for looking into this. I have a few remarks:
- Plz see my comment below, imho doing it with a regexp is a bit over-engineered.
- Would also be nice to have this covered by a test case.
- I just saw, that the
setHyperlink
handler returnsfalse
in some cases, which is suboptimal, as the parser has an optimization for the thetrue
case (since it is a default handler in xterm.js it can always returntrue
). Could you also change that?
lol, feedback makes sense. Happy to make the changes. I struggled to find a good place to put a test when I was looking at this, but I was short on time so I'll give it another go. I'll do it earlier if I can, but I may not have time to look at this again till Friday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @josiahhudson, I tweaked it to use @jerch's suggestion and added a couple of tests
Original code performed a simple
data.split(';')
which split on every ";" in the string. This new version treats the first ";" as the only split character, ensuring that params are properly separated from the URI.