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

Make links clickable #85

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FliegendeWurst
Copy link
Contributor

Makes navigation easier. No need to copy and paste links to the address bar.

src/highlight.rs Outdated Show resolved Hide resolved
@FliegendeWurst
Copy link
Contributor Author

All fixed :)

@matze
Copy link
Owner

matze commented Jan 6, 2025

I like the idea but there can be implications on runtime, especially with very large files. On the one hand there is another allocation, on the other hand

the overall worst case time complexity for this routine is O(m * n^2)

But I will merge for now and wait for complaints later :-)

Thank you for your contribution 👍

src/highlight.rs Outdated Show resolved Hide resolved
@FliegendeWurst
Copy link
Contributor Author

FliegendeWurst commented Jan 6, 2025

I like the idea but there can be implications on runtime, especially with very large files.

The regex runs per-line, so the impact will be limited. If you are worried about the allocation, I suppose it could be skipped if there are no matches.

This regex specifically has a static prefix http so it will run very quickly.

@FliegendeWurst FliegendeWurst force-pushed the clickable-links branch 3 times, most recently from 39845c0 to 0df2554 Compare January 7, 2025 09:00
src/highlight.rs Outdated Show resolved Hide resolved
src/highlight.rs Outdated Show resolved Hide resolved
src/highlight.rs Outdated Show resolved Hide resolved
src/highlight.rs Outdated Show resolved Hide resolved
@matze
Copy link
Owner

matze commented Jan 7, 2025

Seeing the potential for wrong output, how about we take a step back and write a custom HTML generator based on the one that's used? This avoids extra allocations and hacks like the double post-processing.

@FliegendeWurst
Copy link
Contributor Author

Thanks for the suggestion. I had a look and the Markdown highlighter actually colors URL already.
This is the rule used: https://github.com/sublimehq/Packages/blob/fa6b8629c95041bf262d4c1dab95c456a0530122/Markdown/Markdown.sublime-syntax#L672-L696
That can be referenced in an additional syntax set. I'll update the PR soon.

Comment on lines +35 to +39
let link_highlighting = SyntaxDefinition::load_from_str(
include_str!("../assets/LinkHighlight.sublime-syntax"), false, None).expect("loading link style");
let mut builder = syntax_set.into_builder();
builder.add(link_highlighting);
let syntax_set = builder.build();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kills debug performance. We could use the trick from https://stackoverflow.com/questions/60751806/how-to-compile-some-dependencies-with-release but I don't know which dependency is the culprit.

Copy link
Contributor Author

@FliegendeWurst FliegendeWurst Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it depends on yaml-rust which was promptly denied by CI :(

trishume/syntect#537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants