Skip to content

Conversation

@mjwwit
Copy link
Contributor

@mjwwit mjwwit commented Apr 4, 2017

This PR fixes the markdown rendering issues with code-blocks containing URLs (#1442). The culprit was the autolink function, which conflicted with highlight.js. I've excluded code-block URLs from being auto-linked to fix this issue.

Besides being broken the autolink function was also crazy inefficient. I've improved the efficiency by making use of the browser-native createTreeWalker function. If this function is not available (IE8 and below) I make use of a recursive function (which is still more efficient than the previous implementation).

@lunny lunny added this to the 1.2.0 milestone Apr 5, 2017
@lunny lunny added the type/bug label Apr 5, 2017
@strk
Copy link
Member

strk commented Apr 5, 2017

Can you try to have this bug covered by an automated testcase ?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 5, 2017
@mjwwit
Copy link
Contributor Author

mjwwit commented Apr 6, 2017

I'd love to, but there are no automated tests for the front-end AFAIK. Would you like me to build a unit testing environment just to test this case?

@strk
Copy link
Member

strk commented Apr 6, 2017 via email

@andreynering
Copy link
Contributor

Since we don't have a test setup for the font-end yet, this should not be a block for this PR. That should be discussed in a proposal first, and than made in another PR.

@strk
Copy link
Member

strk commented Apr 6, 2017 via email

@mjwwit
Copy link
Contributor Author

mjwwit commented Apr 6, 2017

Reviewing the fix is quite easy. For your convenience I made a benchmark for the performance difference:
https://codepen.io/mjwwit/pen/YZbJEN?editors=1010

For those who can't be bothered to try it out, my optimized implementation will be about 30-50% faster on average.

@strk
Copy link
Member

strk commented Apr 7, 2017 via email

@strk
Copy link
Member

strk commented Apr 7, 2017 via email

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 7, 2017
@mjwwit
Copy link
Contributor Author

mjwwit commented Apr 7, 2017

Yeah, it can be quirky at times. I added a warm-up before the tests, but the results are still not super consistent. I guess it's because there are a ton of factors involved. On average though it will perform significantly better.

@lunny
Copy link
Member

lunny commented Apr 11, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 11, 2017
@lunny lunny merged commit 21290d4 into go-gitea:master Apr 12, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants