-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Markdown rendering overhaul #186
Conversation
Codecov Report
@@ Coverage Diff @@
## master #186 +/- ##
========================================
Coverage ? 5.27%
========================================
Files ? 33
Lines ? 8272
Branches ? 0
========================================
Hits ? 436
Misses ? 7793
Partials ? 43
Continue to review full report at Codecov.
|
@@ -580,6 +580,11 @@ func runWeb(ctx *cli.Context) error { | |||
}, reqSignIn, reqRepoWriter) | |||
}, repo.MustEnableWiki, context.RepoRef()) | |||
|
|||
m.Group("/wiki", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the Markdown rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@metalmatze Related closely. To support all types of embedding images into markdown files (both in wiki and repo itself) I changed wiki content loading scheme. Without this change markdown files with HTML <img>
tags inside would fail to load pictures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, no problem. Just seemed unrelated to me at first glance.
modules/markdown/markdown.go
Outdated
|
||
WikiLinkPattern = regexp.MustCompile(`(\[\[.*\]\]\w*)`) | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you commit all this? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@metalmatze I can't quite catch the meaning. It is necessary for the system to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he's talking about the examples for the matches below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move those patterns into a unit-test instead ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go fmt
and go lint
after these
modules/markdown/markdown.go
Outdated
@@ -271,6 +346,212 @@ func RenderSha1CurrentPattern(rawBytes []byte, urlPrefix string) []byte { | |||
})) | |||
} | |||
|
|||
func RenderFullSha1Pattern(rawBytes []byte, urlPrefix string) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these exported?
routers/repo/wiki.go
Outdated
@@ -43,33 +47,186 @@ type PageMeta struct { | |||
Updated time.Time | |||
} | |||
|
|||
func renderWikiPage(ctx *context.Context, isViewPage bool) (*git.Repository, string) { | |||
func UrlEncoded(str string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two should not be exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, this seems a very good addition. 👍
I just made some comments. Remove commented code, run go fmt
, maybe add some test cases.
modules/markdown/markdown.go
Outdated
case bytes.HasPrefix(text, []byte("[x] ")): | ||
text = append([]byte(`<input type="checkbox" disabled="" checked="" />`), text[3:]...) | ||
case bytes.HasPrefix(text, []byte(prefix+"[ ] ")): | ||
text = append([]byte(`<div class="ui fitted read-only checkbox"><input type="checkbox" /><label /></div>`), text[3+len(prefix):]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing disabled
attribute on checkbox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering yes, to remove awful grey shadow over the checkbox I used read-only
instead of disabled
. In fact, it only influences appearance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer the old disabled
here.
Also, the checkbox should also be disabled while previewing changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering will change.
Also, the checkbox should also be disabled while previewing changes.
Uhm... you mean markdown editor frontend? Or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean markdown editor frontend?
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering it is theme for another PR, isn't it? I'm not exactly sure how to do it at the moment, in fact, gogs/gitea frontend is quite an unknown land for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bug, so should be fixed in this PR. I will take a look on what caused it when I have a little time. 👍
For future contributions, I think it's a good idea to split big features in more PRs. It makes it easier and faster to review.
Let me know if you need help with this. We can push your branch to the main repo, so more people would have write access to it.
modules/markdown/markdown.go
Outdated
} | ||
} | ||
/* | ||
for k, v := range props { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all commented code
modules/markdown/markdown.go
Outdated
@@ -300,9 +584,9 @@ func RenderRaw(body []byte, urlPrefix string) []byte { | |||
extensions |= blackfriday.EXTENSION_NO_INTRA_EMPHASIS | |||
extensions |= blackfriday.EXTENSION_TABLES | |||
extensions |= blackfriday.EXTENSION_FENCED_CODE | |||
extensions |= blackfriday.EXTENSION_AUTOLINK | |||
//extensions |= blackfriday.EXTENSION_AUTOLINK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this necessary anymore? If not maybe we can delete them
modules/markdown/markdown.go
Outdated
extensions |= blackfriday.EXTENSION_STRIKETHROUGH | ||
extensions |= blackfriday.EXTENSION_SPACE_HEADERS | ||
// extensions |= blackfriday.EXTENSION_SPACE_HEADERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
modules/markdown/markdown.go
Outdated
|
||
func RenderWiki(rawBytes []byte, urlPrefix string, metas map[string]string) string { | ||
defer func() { isWikiMarkdown = false }() | ||
isWikiMarkdown = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Global variables are bad. Maybe this can be a function argument?
@@ -594,6 +594,7 @@ function initWikiForm() { | |||
function (data) { | |||
preview.innerHTML = '<div class="markdown">' + data + '</div>'; | |||
emojify.run($('.editor-preview')[0]); | |||
$('.editor-preview').autolink(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to move this from backend to frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering I've experimented hard with it for a long time. Often rendering on backend resulted in broken children links or image links. So instead it is now done on frontend, with a better, conservative yet effective implementation, which operates the DOM, not the wall of text. No links or image urls are now broken, as now it respects tags and their children. It is quite fast, and it offloads servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO that should be done on the backend, otherwise it will never get indexed for public wikis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@STALKER2010 it's also frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only preview, so put it in front-end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunny (1st message) yes, as server-side autolinking often pulled apart tags, attributes. On client side we can manipulate DOM the way we want to, with no harm done to structure.
@tboerger about search engine indexing.
Default <http://google.com>
markdown syntax is always done on server side. Autolinking in plain text is not defined by any markdown standard, so it's just nice stuff to have. If we do plain text autolinking on server, we can corrupt the whole document. Wrong thing to do, so I disabled it. If we do it on client, we may not linkify all possible links, but we do it in much safer way for the document. So, why not. Even if user has disabled JS in his browser, standard links will still work, plain text will not. Better than rendering corrupted document either way. Plus, most popular search engines (like Google, Bing, Yandex) execute JavaScript, so no harm done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunny (2nd question) no, as you see below, it is for both editor preview and final rendering. Reasoning is shown above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most popular search engines (like Google, Bing, Yandex) execute JavaScript, so no harm done.
They also infer a penalty for changing the context in JS 🙁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft the context? You mean, the web page content? I've looked it up, googled it and found no reference of any penalty. What I did find is that Google does not always run JS. Yet, I bet Google crawler is smart enough to do autolinking itself. And even if it isn't, the standard Markdown links should work flawlessly, we only lose unnecessary standard extensions. If anyone is willing to implement autolinking server-side after merging this PR, I would be glad to hear it.
@@ -1361,6 +1362,7 @@ $(document).ready(function () { | |||
node.append('<a class="anchor" href="#' + name + '"><span class="octicon octicon-link"></span></a>'); | |||
}); | |||
}); | |||
$('.markdown').autolink(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
routers/repo/wiki.go
Outdated
} | ||
return nil, git.ErrNotExist{"", target} | ||
} | ||
func _commitTreeEntry(repo *git.Repository, commit *git.Commit, target string, wikiOnly bool) (*git.TreeEntry, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't prefix functions with _
modules/markdown/markdown.go
Outdated
rawBytes = RenderIssueIndexPattern(rawBytes, urlPrefix, metas) | ||
rawBytes = RenderSha1CurrentPattern(rawBytes, urlPrefix) | ||
//rawBytes = RenderSha1CurrentPattern(rawBytes, urlPrefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't we need this anymore?
What's the milestone for this ? |
Fixing all weak areas of PR, writing some additional tests, improving test coverage. I hope it will be done by weekends. |
Ready to review once again. Coverage of markdown.go increased significantly, new tests cover more than half of the code (about 65%, compared to about 25% before the PR), greatly improved wiki page loading (rewritten system, optimizing and improving architecture and code itself). Made some changes to Markdown API, so that all systems work with changes. Fixed bugs found, now markdown files look even better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still minor things that needs addressing.
modules/markdown/markdown.go
Outdated
case bytes.HasPrefix(text, []byte("[x] ")): | ||
text = append([]byte(`<input type="checkbox" disabled="" checked="" />`), text[3:]...) | ||
case bytes.HasPrefix(text, []byte(prefix+"[ ] ")): | ||
text = append([]byte(`<div class="ui fitted read-only checkbox"><input type="checkbox" /><label /></div>`), text[3+len(prefix):]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer the old disabled
here.
Also, the checkbox should also be disabled while previewing changes.
modules/markdown/markdown.go
Outdated
} | ||
|
||
// renderFullIssuePattern renders issues-like URLs | ||
func renderFullIssuePattern(rawBytes []byte, repoLink, urlPrefix string) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found problems with issue pattern rendering.
Link https://github.com/go-gitea/gitea/pull/186
renders to:
- Title:
#186
- Preview URL:
http://localhost:3000/issues/186
- After saved URL:
http://localhost:3000/andreynering/mylocalrepo/issues/186
Link https://github.com/go-gitea/gitea/pull/186/files
renders to:
- Title:
#186/files
- Preview URL:
http://localhost:3000/issues/186
- After saved URL:
http://localhost:3000/andreynering/mylocalrepo/issues/186
The original URL should be preserved. It can refer to GitHub or another Gitea instance, as example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering will look into the case, not sure at the moment what to do with that.
modules/markdown/markdown.go
Outdated
Sha1CurrentPattern = regexp.MustCompile(`(?:^|\s|\()[0-9a-f]{40}\b`) | ||
|
||
// WikiLinkPattern matches difficult [[link syntax]] | ||
WikiLinkPattern = regexp.MustCompile(`(\[\[.*\]\]\w*)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed these kind of link always point to wiki. Is this intentional?
Example: [[foobar]]
on README.md
will refer to /user/repo/wiki/foobar
and not a file called foobar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering yes, it is, it can be changed though. I've just never seen a single repo with markdown file in it using [[syntax]]
. Shall I implement it for repo too?
modules/markdown/markdown_test.go
Outdated
. "github.com/smartystreets/goconvey/convey" | ||
) | ||
|
||
func TestMarkdown(t *testing.T) { | ||
setting.AppUrl = "http://gogs/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oldAppUrl := setting.AppUrl
defer func() {
setting.AppUrl = oldAppUrl
}()
setting.AppUrl = "http://gogs/"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
This need rebase and review continue! |
Still in conflicting state |
@STALKER2010 Any updates on this? |
if nobody want to takeover this one, I will move it to v1.2 |
Please rebase. |
@lunny In progress. Test system rework was quite a change. |
Could someone explain why the build failed? I can only see lint suggestions, not in my code. |
Looks like Windows has done some strange stuff to file permissions. Fixing... |
Please rebase, it seems there are many unused changes. |
Cleaned up and squashed commits into single one. Signed-off-by: Andrew Boyarshin <[email protected]>
@lunny Done. Had to recreate branch and export all changes to patch file due to merge conflicts in the past and Windows file permissions change recently. Also squashed all commits into one. |
routers/api/v1/misc/markdown.go
Outdated
@@ -26,9 +26,9 @@ func Markdown(ctx *context.APIContext, form api.MarkdownOption) { | |||
|
|||
switch form.Mode { | |||
case "gfm": | |||
ctx.Write(markdown.Render([]byte(form.Text), form.Context, nil)) | |||
ctx.Write(markdown.Render([]byte(form.Text), ctx.Repo.RepoLink, nil)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure about this one. I can't recall doing that. Any thoughts? Is it good or bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe repo.HTMLUrl()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunny Hmm... Three different variants. Perhaps, invite more reviewers? Opinions matter.
Great. I will review this. |
It seems this PR could not fix #652 ? |
…erage Signed-off-by: Andrew Boyarshin <[email protected]>
I've finally figured out the correct API code, it was different from both old code and my change. Fixed, added tests to the API (looks like noone ever did API tests in Gogs/Gitea before me). Also increased code coverage, added a couple of tests. Finally, it is completely ready (or at least I hope so). Uhm... Drone is unable to find new |
Anyone willing to review? |
I've tried but it's huge.
Do you have a test repository on try.gitea.io which we can clone
locally to see how your version changes things ?
|
@strk you mean good repos to test on? I test it on guard, libgdx, jquery, D3 wikis. There are also some screenshots in the original Gogs version of this PR. |
There are also tests in this PR, which cover everything pretty good (if not to count API tests, which only work locally; strange thing, haven't figured out why it didn't work remotely). |
OK, 1 out of 2 |
Anyone willing to review? |
LGTM I'll let the owners decide, but I'd merge right this after the v1.1 release, so people that use master may file any bugs that may happen until we have this in a stable release (v1.2). |
@andreynering that's a good idea. We need a PR to stay on master for a long time to find more bugs. I'm thinking our release cycle. Maybe we should publish a release about 2 months? 1 month for merge new feature, one month for bug fixed. All the PRs on the second month should be merged at next release cycle. |
Sorry, i want to ask about my problem with markdown texts. |
@Lourens-Rich it should be unrelated to this PR. Yet, I confirm the problem and the solution is to remove display: block from table, that is _markdown.less#L243. It solves the problem, although it makes table 100% wide in parent container (nothing can be to the left or right side of the table anyway). |
text = append([]byte(`<div class="ui checked fitted disabled checkbox"><input type="checkbox" checked="" disabled="disabled" /><label /></div>`), text[3+len(prefix):]...) | ||
} | ||
if prefix != "" { | ||
text = bytes.Replace(text, []byte("</p>"), []byte{}, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-boyarshin You should remove "/p" tag only if you also remove starting tag (so only if u do append). This code cause bugs in rendering (example: a list rendering bug with code in list - "/li" is replaced by "/p" in postprocess because the last starting tag before "pre, code" is only starting "p" tag).
The majority of markdown files should now look great.
[[...]]
image & link handling. It is quite a feature. It supports a variety of syntaxes.