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

Optimize and fix autolink function (#1442) #1444

Merged
merged 2 commits into from
Apr 12, 2017
Merged

Optimize and fix autolink function (#1442) #1444

merged 2 commits into from
Apr 12, 2017

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