Faster link highlighting with more types supported #188

tim-gromeyer · 2023-08-20T18:51:25Z

Faster link highlighting
More link types supported
Don't highlight HTML tags with a . in it (not true anymore, since it wes requested to support files...)

tim-gromeyer · 2023-08-20T20:05:25Z

markdownhighlighter.cpp

@@ -2051,7 +2026,7 @@ void MarkdownHighlighter::highlightInlineRules(const QString &text) {
 */
 int MarkdownHighlighter::highlightInlineSpans(const QString &text,
                                              int currentPos, const QChar c) {
-    //clear code span ranges for this block
+    // clear code span ranges for this block


Nothing changed below this spot, except the formatting. The next change is in the next file

tim-gromeyer · 2023-08-20T23:15:30Z

I noticed something: It highlights HTML tags as links, I need to fix this

pbek · 2023-08-21T06:29:45Z

Thank you for your concern.
You ripped a lot of code out. It's hard to tell what implications that would have.

@Waqar144, opinions?

tim-gromeyer · 2023-08-21T06:36:53Z

You ripped a lot of code out. It's hard to tell what implications that would have.

Yeah, I ripped the old code out which used regular expressions. It was just to unreliable so I replaced it

tim-gromeyer · 2023-08-21T09:05:13Z

It is fixed now!

tim-gromeyer · 2023-08-21T19:52:18Z

You ripped a lot of code out. It's hard to tell what implications that would have.

You can test it by using the following file (https://raw.githubusercontent.com/tim-gromeyer/html2md/main/tests/links.md) to check the highlighting. It should match the behavior as it did before.

You can copy the content of the linked file and try out the old behavior here (https://tim-gromeyer.github.io/MarkdownEdit/markdownedit.html). Then, compare it to a locally built version with this PR integrated.

Waqar144 · 2023-08-22T11:55:51Z

And sorry... my IDE changed a bit of formatting in some lines... but no feature changes

Yes, but it makes reviewing harder than it needs to be as one has to go through changes that are meaningless. Thus I would suggest you to remove the formatting changes, or at least put them in a separate commit, then rebase the actual feature changes on top. The way it currently is, it is hard to see what is going on.

It is also bad for git blame history.

Waqar144 · 2023-08-22T12:06:52Z

Code wise things look okay I think. The new link highlighting code probably won't be able to handle complex link cases but those don't matter usually and I haven't tested to be sure.

pbek · 2023-08-22T12:39:10Z

I'd also rather like to have whitespace changes in a different PR...

tim-gromeyer · 2023-08-22T12:43:13Z

Allright, I'll try to revert the formatting changes

pbek · 2023-08-22T12:51:49Z

And this PR breaks lots of links...

<link%20destination.md>
<linkdestination.md>
[Link test](Link%20test.txt)
[Link Destination - Heading 2](Link%20Destination.md#Heading%202)
[Link test - A heading](Link%20test.md#A%20heading)

---

[Test 1](Test%201.md#Heading%201)
[# Test 1](Link%20Destination.md#Heading%201)
[# Test 2](Link%20Destination.md#Heading%202)
[# Test 3](Link%20Destination.md#Heading)
[# Test 3](Link%20Destination.md#Heading)

[# Test 4](.#A%20Heading)
[Link test](Link%20test.md#A%20Heading)
[# Test 3](Link%20Destination.md)

<Link%20Destination.md>
<Link%20Destination.md>

asdas www.github.com asdasd
www.github.com
https://www.github.com


- [Rename test](folder/Rename%20test3c.md)

- [Rename test](Rename%20test3b.md)
- [Note with one bracket](Note%20with%20one%20bracket%5D.md)
- [Note with one bracket](Note%20with one bracket].md)
- [Note with one bracket](Note%20with&#32;one bracket].md)

tim-gromeyer · 2023-08-22T13:14:40Z

For the links enclosed in angle brackets (<>) it is on purpose. Before it also highlighted HTML tags with a dot (.) in it. For example <script src="script.js">. Just put file:// before it and it will work

And for the greyed links... I didn't saw it. You couldn't really see it with dark mode

What highlighter state _format[HighlighterState] does it use?

Edit: It also fixes things like this:

## Images with References

[![Forest][forest-image]][forest-link]

[forest-image]: https://www.example.com/images/forest.jpg
[forest-link]: https://www.example.com/nature/forest

tim-gromeyer · 2023-08-22T15:24:42Z

@pbek, can you test it again, please? It should be fixed now

pbek · 2023-08-23T07:41:27Z

Those still don't work:

## PR 188

- <note://QOwnNotes_Todo>
- <link%20destination.md>
- <linkdestination.md>
- <attachments/1672212884.pdf>
- note://Note_2018_06_26T22_11_10
- <../p11.md>

pbek · 2023-08-23T07:45:34Z

Link highlighting doesn't work some times.

Can you please give some examples when it doesn't work?
And can you please explain why your changes are more robust?

I'm still very unhappy to lose all the documentation on what cases of links the regular expressions cover...

tim-gromeyer · 2023-08-23T08:30:46Z

Those still don't work:

## PR 188

- <note://QOwnNotes_Todo>
- <link%20destination.md>
- <linkdestination.md>
- <attachments/1672212884.pdf>
- note://Note_2018_06_26T22_11_10
- <../p11.md>

The files don't work on purpose. Use file:// and it will work. The files caused some problems, they sometimes highlighted HTML tags if they contained a dot

And for note://, I can add support for it by simply adding note:// to this list

But 1 Question: Should note:// always be highlighted or just when it's in ankled brackets (<>)

tim-gromeyer · 2023-08-23T09:26:04Z

Can you please give some examples when it doesn't work?

Images with references... don't work with this PR either. It's just way too complex

And can you please explain why your changes are more robust?

I encountered an issue but found a solution... (Original reason)
This change should result in improved performance. (I once used regex in a C++ library, and after removing it, the tests were 60% faster, even though another library was used twice to perform conversions before and after...)
Adds support for more types of links (see the isLink function)
Fixes some instances of HTML tags being highlighted

I'm still very unhappy about losing all the documentation regarding the cases of links that the regular expressions cover...

I can add more documentation if you would like; it's a pretty simple task

Waqar144 · 2023-08-23T11:30:31Z

Those still don't work:

note://QOwnNotes_Todo

<link%20destination.md>

<linkdestination.md>

<attachments/1672212884.pdf>

note://Note_2018_06_26T22_11_10

<../p11.md>

imo, Those shouldn't be handled by this library as they aren't really links I think but only something that QON considers as links. As such, QON should highlight these in it's highlighter subclass. The base class should be more generic

pbek · 2023-08-23T12:34:49Z

imo, Those shouldn't be handled by this library as they aren't really links I think but only something that QON considers as links. As such, QON should highlight these in it's highlighter subclass. The base class should be more generic

This would be fine with me, but then someone would need to do the migration in https://github.com/pbek/QOwnNotes/blob/main/src/helpers/qownnotesmarkdownhighlighter.cpp first before I'd consider merging that PR...

tim-gromeyer · 2023-08-23T15:41:06Z

I already added support for it here

tim-gromeyer · 2023-08-23T20:41:34Z

markdownhighlighter.cpp

+        QLatin1String("spotify:"), QLatin1String("steam:"),
+        QLatin1String("bitcoin:"), QLatin1String("magnet:"),
+        QLatin1String("ed2k://"),  QLatin1String("news:"),
+        QLatin1String("ssh://"),   QLatin1String("note://")};


note:// is supported here

pbek · 2023-08-24T08:12:16Z

I already added support for it here

yes, but just the note:// ones...

tim-gromeyer · 2023-08-24T09:17:55Z

I already added support for it here

yes, but just the note:// ones...

What else do you want? The files? They caused problems with some HTML tags being highlighted. They are not highlighted on purpose because of this. You can use file:// or make a list of all file extensions and check if it ends with a known one to avoid false positives with HTML tags.

Waqar144 · 2023-08-24T10:02:37Z

I already added support for it here

yes, but just the note:// ones...

What else do you want? The files? They caused problems with some HTML tags being highlighted. They are not highlighted on purpose because of this. You can use file:// or make a list of all file extensions and check if it ends with a known one to avoid false positives with HTML tags.

I think he means that we still need to be able to support all the kind of links that previous code did. Some links that may not be "generic" enough and are only needed to by QOwnNotes need to be supported there, so someone, must add support for them in QON before this PR can be considered for merging.

tim-gromeyer · 2023-08-24T11:32:21Z

I think he means that we still need to be able to support all the kind of links that previous code did

It had a tendency to highlight HTML tags, but of course I'll add support for that

tim-gromeyer · 2023-08-24T11:48:44Z

Works now

pbek · 2023-08-24T12:58:53Z

Works now

Thank you, but currently the closing > are not highlighted.

pbek · 2023-08-24T13:45:54Z

LGTM, thank you!

tim-gromeyer · 2023-08-24T13:55:17Z

You are welcome!

pbek · 2023-12-04T17:45:19Z

@tim-gromeyer, unfortunately 53c4c598 did break something else too:

    wget http://google.com

http://google.com is now highlighted as link, but it shouldn't.

pbek · 2023-12-04T17:48:32Z

I opened an issue for it #199.

tim-gromeyer commented Aug 20, 2023

View reviewed changes

tim-gromeyer marked this pull request as draft August 20, 2023 23:14

tim-gromeyer force-pushed the more-robust-link-highlighting branch from 868e3eb to 8d1c397 Compare August 21, 2023 09:04

tim-gromeyer marked this pull request as ready for review August 21, 2023 09:04

tim-gromeyer force-pushed the more-robust-link-highlighting branch from 39823a4 to 9030278 Compare August 22, 2023 12:51

tim-gromeyer force-pushed the more-robust-link-highlighting branch from 9030278 to 231a188 Compare August 22, 2023 13:03

tim-gromeyer force-pushed the more-robust-link-highlighting branch from e93567c to c18ab90 Compare August 22, 2023 15:24

tim-gromeyer force-pushed the more-robust-link-highlighting branch from c18ab90 to b597cbf Compare August 22, 2023 18:39

More robust link highlighting

53c4c59

tim-gromeyer force-pushed the more-robust-link-highlighting branch from b597cbf to 53c4c59 Compare August 23, 2023 07:38

tim-gromeyer added 2 commits August 23, 2023 11:18

Add support for highlighting note://

1ff4d62

Fix build

1efae61

Merge branch 'pbek:develop' into more-robust-link-highlighting

7eb86c0

tim-gromeyer commented Aug 23, 2023

View reviewed changes

Highlight files in ankled brakets

51fdaed

tim-gromeyer changed the title ~~More robust link highlighting~~ Faster link highlighting with more types supported Aug 24, 2023

Highlight closing >

23d6f6d

pbek merged commit 361af8b into pbek:develop Aug 24, 2023

tim-gromeyer deleted the more-robust-link-highlighting branch August 24, 2023 13:55

pbek added a commit to pbek/QOwnNotes that referenced this pull request Aug 25, 2023

pbek/qmarkdowntextedit#188 deps: update qmarkdowntextedit library

5fb9df5

pbek mentioned this pull request Dec 4, 2023

Falsely highlighting of links in 4-Spaces fence code blocks #199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster link highlighting with more types supported #188

Faster link highlighting with more types supported #188

tim-gromeyer commented Aug 20, 2023 •

edited

Loading

tim-gromeyer Aug 20, 2023

tim-gromeyer commented Aug 20, 2023

pbek commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

Waqar144 commented Aug 22, 2023 •

edited

Loading

Waqar144 commented Aug 22, 2023

pbek commented Aug 22, 2023

tim-gromeyer commented Aug 22, 2023

pbek commented Aug 22, 2023

tim-gromeyer commented Aug 22, 2023 •

edited

Loading

tim-gromeyer commented Aug 22, 2023

pbek commented Aug 23, 2023

pbek commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023 •

edited

Loading

Waqar144 commented Aug 23, 2023

pbek commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023

tim-gromeyer Aug 23, 2023

pbek commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

Waqar144 commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

pbek commented Aug 24, 2023

pbek commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

pbek commented Dec 4, 2023

pbek commented Dec 4, 2023

Faster link highlighting with more types supported #188

Faster link highlighting with more types supported #188

Conversation

tim-gromeyer commented Aug 20, 2023 • edited Loading

tim-gromeyer Aug 20, 2023

Choose a reason for hiding this comment

tim-gromeyer commented Aug 20, 2023

pbek commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

tim-gromeyer commented Aug 21, 2023

Waqar144 commented Aug 22, 2023 • edited Loading

Waqar144 commented Aug 22, 2023

pbek commented Aug 22, 2023

tim-gromeyer commented Aug 22, 2023

pbek commented Aug 22, 2023

tim-gromeyer commented Aug 22, 2023 • edited Loading

tim-gromeyer commented Aug 22, 2023

pbek commented Aug 23, 2023

pbek commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023 • edited Loading

Waqar144 commented Aug 23, 2023

pbek commented Aug 23, 2023

tim-gromeyer commented Aug 23, 2023

tim-gromeyer Aug 23, 2023

Choose a reason for hiding this comment

pbek commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

Waqar144 commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

pbek commented Aug 24, 2023

pbek commented Aug 24, 2023

tim-gromeyer commented Aug 24, 2023

pbek commented Dec 4, 2023

pbek commented Dec 4, 2023

tim-gromeyer commented Aug 20, 2023 •

edited

Loading

Waqar144 commented Aug 22, 2023 •

edited

Loading

tim-gromeyer commented Aug 22, 2023 •

edited

Loading

tim-gromeyer commented Aug 23, 2023 •

edited

Loading