Skip to content

Links (id and target) do not survive the render process #363

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.

Sign up for GitHub

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

Jump to bottom
Open
wischi-chr opened this issue Apr 10, 2021 · 5 comments · May be fixed by #1813
Open

Links (id and target) do not survive the render process #363

wischi-chr opened this issue Apr 10, 2021 · 5 comments · May be fixed by #1813
Labels
bug Something isn't working

Comments

@wischi-chr
Copy link
Contributor

wischi-chr commented Apr 10, 2021 •
edited by FrankRay78
Loading

Information

  • Version: most recent master: 6a5c507

Describe the bug
If a link is output into a small grid for example it gets layouted into multiple lines. The way the linkId is currently implemented (Hash+Random) will cause those segments to be treated as different links. This results in a situation where the hover effect occurs only on parts of the link.

To Reproduce

// create a narrow grid with width 10 containing a link
AnsiConsole.Console.Write(
    new Grid() { Width = 10 }
        .AddColumn()
        .AddRow("[link]https://google.com[/]")
);

// now hovering over the link (in a terminal that supports hyperlinks) only highlights part of the link.
// also because of the implicit link markup the link target is wrong as well.

Expected behavior
Even if the link is split into multiple rows during the render process it should be the same link (same id so the terminal recognizes it as the same link) and the target should still be valid for all segments.

Screenshots
image

Additional context
I think this are really two bugs.

  1. The link target is wrong because the implicit link is not set inside the style but pretty late in AnsiBuilder.GetAnsi but at that point the inner text might already be split and layouted all over the place.
  2. The link Id should probably be persisted inside Style to survive the layout process. At the moment everything is split into segments and later a linkId is assigned.

Ideas:
Regarding point 1) if a link markup only has text but not set an explicit link the assignment should happen before the style is created (probably in StyleParser.cs)
Regarding point 2) IMO the linkId should be part of the Style and just be incremented and set in the constructor if the provided link was not null or empty. The counter could simply be a static field and used like that: LinkId = Interlocked.Increment(ref _linkId);

To be honest I don't get why the current LinkId hasher was implemented as it is. First a hash is generated based on the link target and than a random number is added, so why hash the link in the first place? Just incrementing a counter would be simpler and deterministic and would result in a more testable output.

Please upvote 👍 this issue if you are interested in it.
@wischi-chr wischi-chr added the bug Something isn't working label Apr 10, 2021
@patriksvensson
Copy link
Contributor

@wischi-chr Great catch. This is an edge case for links without an explicit link attribute URL.

  1. Regarding your proposed solution. I would prefer if we didn't have any static state and similar in the library. This makes It error-prone and difficult to test.

  2. Regarding why we use hash + random, we wanted to reduce the number of collisions in a state-free way. I can't remember why we decided to do things like this, to be honest, but there was probably some reason for it regarding what the code looked like back then.

  3. Regarding why we don't precalculate this in the Style itself, it would highlight all links on the screen that uses the same style, making it difficult to reuse styles with a link.

To solve the other problem, I guess we could have another field within Segment called OriginalText or similar that we set when we split a segment in two that the link generator could use to insert the correct link in case of a missing explicit link attribute URL.

Suggestions are welcome!

@phil-scott-78
Copy link
Contributor

I can actually wager an educated guess on the hash algorithm - it matches how Rich does things.

If you get a chance, @wischi-chr, you should dig into that project a bit too. As the readme states the console portion of Spectre.Console been a major inspiration and starting point for many of the features. By porting the features is what allowed @patriksvensson to create a tremendous amount of functionality very quickly, but at the cost of some things having maybe slightly better implementations in C# if he had started everything from scratch.

I suspect "because Rich does" is also the answer to why there is a clone method on style.

Not saying what Rich did was perfect and obviously if there are better ways to implement in c# we should do so. But as with any OSS project the how's and why typically involve a combination of requirements at that time, and also the technical limitations at that time. But in this case if you are looking into that history for this project it's important to take Rich into account when you are researching why something was done that way at first

@wischi-chr wischi-chr mentioned this issue Apr 11, 2021
Closed
@patriksvensson
Copy link
Contributor

patriksvensson commented Apr 11, 2021 •
edited
Loading

The Clone method was introduced here and if I remember correctly, I used it for the SegmentLineIterator but ended up not needing it, but forgot to remove it.

@ShaunLawrie ShaunLawrie linked a pull request Apr 27, 2025 that will close this issue
Draft
6 tasks
@ShaunLawrie
Copy link

I know this is a really old issue but I've had a few people raise that their really long links get broken when they're trying to fit them in a panel in my powersehll wrapper for spectre.console. I have a pretty naive solution to this at #1813

Having the link broken when the terminal is narrow or a link is used inside a table is a pretty inconvenient issue which the naive solution addresses.

Making the ID consistent across a split URL requires adding state to the segment and passing it the whole way down to the AnsiLinkHasher. It's quite a bit of extra complexity for what is really just to make the whole link hover effect impact both halves of the split link so I'm not sure it's worth the effort at the moment.

@ShaunLawrie ShaunLawrie mentioned this issue Apr 27, 2025
Merged
@patriksvensson
Copy link
Contributor

@ShaunLawrie I think your solution is pragmatic and a good enough approach to solve this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
Spectre Console
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Preserve links when markup is being truncated/linebroken ShaunLawrie/spectre.console
4 participants
@patriksvensson @phil-scott-78 @wischi-chr @ShaunLawrie