Add OSC8 hyperlink support#1360
Conversation
|
For us to consider merging this it needs to have test coverage. |
|
That's a fair cop. I didn't immediately find the test suite but now I have I will take a crack at it. |
|
Good catch. I'm rethinking the way the params are parsed and they should all be technically preserved, not just id. |
|
PTAL, there's a test now and I reworked the param parsing. |
|
Please let me know if there's any further issue. I'd like to take a stab at adding OSC7 support as well since it seems straightforward compared to this, but would like to see this merged first. |
|
I did a quick test and review and it seems to work as advertised, but I'll do some more real-world testing over the next few weeks |
|
I'm doing some testing with ripgrep, which has support for OSC 8 hyperlinks. In particular there's a special "vscode" mode that causes some problems. echo test > test
rg test -g test --hyperlink-format=vscode --color=alwaysIn my terminal, it looks right: 
But when I 
This doesn't happen outside of mosh. You can also test with this: |
|
Thanks for the review. I missed clearing the hyperlink correctly in |
|
Please let me know if you found any other problems, would love to see this merged. |
zedinosaur
force-pushed
the
osc8
branch
2 times, most recently
from
63c1bb8 to
ef3b296
Compare
zedinosaur
force-pushed
the
osc8
branch
3 times, most recently
from
2b16655 to
1e2ff5b
Compare
Per the specification described in https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda, add hyperlink support to mosh. Closes mobile-shell#1245
Just store the params unparsed since we don't explicitly need the id.
|
Please let me know if there's anything else I can do to get this merged. |
src/terminal/terminalframebuffer.cc
Outdated
|
|
||
| std::shared_ptr<const Hyperlink> Hyperlink::make_empty() | ||
| { | ||
| static auto* const empty_hyperlink = new std::shared_ptr<const Hyperlink>( new Hyperlink ); |
There was a problem hiding this comment.
There is no point in make_empty returning a std::shared_ptr.
static const auto* empty_hyperlink = new Hyperlink;
return empty_hyperlink;
suffices
There was a problem hiding this comment.
If I do that every empty hyperlink will get a fresh shared_ptr, including allocating a new control block. I can do that but seems wasteful of memory?
There was a problem hiding this comment.
Taking a step back, what problem is this shared_ptr trying to solve? Is the issue that it's going to be incremented in every cell by default?
There was a problem hiding this comment.
My idea for the shared_ptr was just to make the representation of an empty hyperlink cheaper in a Cell, since most Cells won't have hyperlinks set. Creating a new shared_ptr for Cell that's empty seems similarly wasteful. I'll happily rip out all of the shared_ptr stuff out if you think this optimization isn't worth it.
src/terminal/terminalfunctions.cc
Outdated
| fb->ds.set_hyperlink( Hyperlink::make_empty() ); | ||
| } else { | ||
| fb->ds.set_hyperlink( | ||
| std::make_shared<const Hyperlink>( OSC_string.substr( 2, second_semicolon - 2 ), std::move( url ) ) ); |
There was a problem hiding this comment.
I think this is the only place in the code where a non-empty hyperlink is created? I'm a bit weirded out by the indexing, but I think it's safe? OSC_substring is guaranteed to be at least size() > 2 on line 611 and second_semicolon is guaranteed to be 2..size() if it's not npos, which it's not on line 617, so it's probably fine?
There was a problem hiding this comment.
That's right. I can capture the params into a variable if that would make things clearer.
3 participants
Per the specification described in https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda, add hyperlink support to mosh.
Closes #1245