Fix collision of osc-8 id #1507
Conversation
github-actions
bot
added
the
VT: Backend
Yaraslaut
force-pushed
the
fix/osc8_link_id
branch
from
9641436 to
689eae0
Compare
src/vtbackend/Screen.cpp
Outdated
| @@ -1470,19 +1470,13 @@ void Screen<Cell>::setCurrentWorkingDirectory(string const& url) | |||
|
|
|||
| template <typename Cell> | |||
| CRISPY_REQUIRES(CellConcept<Cell>) | |||
| void Screen<Cell>::hyperlink(string id, string uri) | |||
| void Screen<Cell>::hyperlink([[maybe_unused]] string id, string uri) | |||
There was a problem hiding this comment.
Thank you, @Yaraslaut, for this PR.
I think, if this parameter is not to be used anymore, we should get rid of it. We could then also give this function a more actionable name, to make it more clear on the caller side, what this function is intending to do.
There was a problem hiding this comment.
If you're not using the id parameter, doesn't that mean you'll fail to highlight both parts of the hyperlink when it gets split (e.g. in something like a wrapped tmux pane)?
There was a problem hiding this comment.
If you're not using the id parameter, doesn't that mean you'll fail to highlight both parts of the hyperlink when it gets split (e.g. in something like a wrapped tmux pane)?
Not sure about tmux, but every link has its own unique id and after wrap everything is alright:
There was a problem hiding this comment.
Thank you, @Yaraslaut, for this PR.
I think, if this parameter is not to be used anymore, we should get rid of it. We could then also give this function a more actionable name, to make it more clear on the caller side, what this function is intending to do.
I didn't change the signature to have osc-8 compliant function for hyperlinks, if you think that we do not need to keep it that way, i can propagate this change in the code further
There was a problem hiding this comment.
The point is that a multiplexer might have to redraw the wrapped link itself, potentially somewhere in the middle of the screen. In that case you'll end up with two links with the same id, which the multiplexer will output something like this:
printf '\e[10C\e]8;id=123;http://example.com/\e\\WRAPPE\e]8;;\e\\\n\e]8;id=123;http://example.com/\e\\D_LINK\e]8;;\e\\\n'
Hovering over either half of the link should underline both parts. That's the whole reason for the id parameter.
There was a problem hiding this comment.
Oh, i see, not sure which of the behaviours we want to break then, i would prefer instead of the id of the link to have some flag indicating that we need to connect the link with previous one, otherwise i have no idea how this can be overcome, any ideas @j4james, @christianparpart ?
There was a problem hiding this comment.
What I was trying to explain in #1499 (comment), and what the spec says, is that you should combine the id with the url in order to get a unique identifier for each hyperlink. If you've got two links on the page with the same id and url, they should both be underlined together. If you've got two links with the same id, but with different urls, then they won't be underlined together, and should have separate entries in your cache. The bug in #1499 should not be possible if you follow this approach.
There was a problem hiding this comment.
I updated PR @j4james, thanks a lot for taking a time and explained what I was missing in my previous fix. Hope that now everything is correct :)
There was a problem hiding this comment.
@Yaraslaut I don't know the code base, so I can't really review it properly, but the latest changes look good to me.
Yaraslaut
force-pushed
the
fix/osc8_link_id
branch
from
689eae0 to
e631221
Compare
Yaraslaut
force-pushed
the
fix/osc8_link_id
branch
from
e631221 to
7d907b3
Compare
Signed-off-by: Christian Parpart <christian@parpart.family>
christianparpart
force-pushed
the
fix/osc8_link_id
branch
from
7d907b3 to
c502242
Compare
Closes #1499