Rework file highlight rendering and fix yaml copy-paste

[silverwind]

Fixes: #19331

Followup to #19798. Remove both the .line and .cl classes from Chroma's HTML which made the old conditional work again. This fixed Copy of YAML files for me while also reducing the amount of rendered HTML nodes.

This retains the .w class which is for whitespace and which may be useful for themes to style.

[wxiaoguang]

I proposed a PR silverwind#2 .

Major changes:

• the tags are processed accurately
• read the buffer directly instead of splitting it, performance improvement (maybe?).
• EOLs are handled correctly
• complete (and more simple) tests

After this PR, the highlighted result can be processed easily without hacky methods.

The screenshot for the new tests (according to the discussion).

And the 
 can be easily replaced to anything (eg: .w span) if needed.

[wxiaoguang]

I have a feeling that the fix should also be included in 1.17 release, otherwise the lost empty line problem is too annoying for users.

(Or if the release shouldn't be blocked, then release 1.17 first and fix the problem later)

[silverwind]

Merged that pr. Need to rebase and test this.

[wxiaoguang]

More information during discussion:

• For GitHub (which is not ideal or even incorrect):
  • https://github.com/wxiaoguang/playground/blob/main/eol-no.py : no new line, 1 lines (1 sloc) 13 Bytes
  • https://github.com/wxiaoguang/playground/blob/main/eol-newline.py : one new line, 1 lines (1 sloc) 14 Bytes
  • https://github.com/wxiaoguang/playground/blob/main/eol-newline2.py : two new lines, 2 lines (1 sloc) 15 Bytes , but still only one line is displayed on UI
  • The copying of all of these GitHub files on UI will result in only one line without new line.

The changes beside #19967 (comment):

• The redundant <span class="line"><span class="cl"> are removed.
• Line number rule (see the comment in renderFile):
  • empty: 0 line
  • "a": one line
  • "a\n": two lines
  • "a\nb": two lines
• If users want to hide the last empty line or show a block emoji (🚫) for no-newline when rendering, the new code is easier to do so in the future.

[silverwind]

Tested this successfully, copy paste of empty lines in YAML works as expected.

Regarding final newline, it seems to behave similar to #18076 now, which I'm personally happy with. The option from that PR could still be added separately later.

Need one more review.

[silverwind]

Squashed and added a better title.

[silverwind]

@zeripath would you mind a review? This PR started out as a bugfix for copy/paste, but then was rewritten by @wxiaoguang. I think it's a nice simplification of the output HTML of rendered files, it also removes wrapper elements introduced by chroma, the same as we did in #19958, resulting in less HTML sent the the browser, and in turn faster rendering.

[zeripath]

Hmm... I have a feeling that we're going about this the wrong way.

What's the purpose of all of this?

Remove both the .line and .cl classes from Chroma's HTML which made the old conditional work again

OK. Where and why do they appear?

• The chroma HTML formatter adds them for each line during its Format
• https://github.com/alecthomas/chroma/blob/96fe58e6b13d6bf3e2da0bb88b37f70be2dd9bef/formatters/html/html.go#L279

Let's examine that code:

	fmt.Fprintf(w, f.preWrapper.Start(true, f.styleAttr(css, chroma.PreWrapper)))

	highlightIndex = 0
	for index, tokens := range lines {
		// 1-based line number.
		line := f.baseLineNumber + index
		highlight, next := f.shouldHighlight(highlightIndex, line)
		if next {
			highlightIndex++
		}

		if !(f.preventSurroundingPre || f.inlineCode) {
			// Start of Line
			fmt.Fprint(w, `<span`)

			if highlight {
				// Line + LineHighlight
				if f.Classes {
					fmt.Fprintf(w, ` class="%s %s"`, f.class(chroma.Line), f.class(chroma.LineHighlight))
				} else {
					fmt.Fprintf(w, ` style="%s %s"`, css[chroma.Line], css[chroma.LineHighlight])
				}
				fmt.Fprint(w, `>`)
			} else {
				fmt.Fprintf(w, "%s>", f.styleAttr(css, chroma.Line))
			}

			// Line number
			if f.lineNumbers && !wrapInTable {
				fmt.Fprintf(w, "<span%s%s>%s</span>", f.styleAttr(css, chroma.LineNumbers), f.lineIDAttribute(line), f.lineTitleWithLinkIfNeeded(lineDigits, line))
			}

			fmt.Fprintf(w, `<span%s>`, f.styleAttr(css, chroma.CodeLine))
		}

		for _, token := range tokens {
			html := html.EscapeString(token.String())
			attr := f.styleAttr(css, token.Type)
			if attr != "" {
				html = fmt.Sprintf("<span%s>%s</span>", attr, html)
			}
			fmt.Fprint(w, html)
		}

		if !(f.preventSurroundingPre || f.inlineCode) {
			fmt.Fprint(w, `</span>`) // End of CodeLine

			fmt.Fprint(w, `</span>`) // End of Line
		}
	}
	fmt.Fprintf(w, f.preWrapper.End(true))

• Interestingly this differs from the code we currently have in Gitea because of: alecthomas/chroma@d18e8a4

1. Upgrade Chroma to at least version 2.2.0

Chroma has added an option to prevent these spans from being created in the first place.

Can we fix this without upgrading Chroma?

However assuming that we can't upgrade Chroma... Perhaps we can do things differently... Lets just look at this loop a bit more clearly.

	for index, tokens := range lines {
		// 1-based line number.
		...

		// Start of Line
		fmt.Fprint(w, `<span`)
		if highlight {
			...
		} else {
			fmt.Fprintf(w, "%s>", f.styleAttr(css, chroma.Line))
		}

		// Line number
		...

		fmt.Fprintf(w, `<span%s>`, f.styleAttr(css, chroma.CodeLine))

		for _, token := range tokens {
			html := html.EscapeString(token.String())
			attr := f.styleAttr(css, token.Type)
			if attr != "" {
				html = fmt.Sprintf("<span%s>%s</span>", attr, html)
			}
			fmt.Fprint(w, html)
		}

		fmt.Fprint(w, `</span>`) // End of CodeLine

		fmt.Fprint(w, `</span>`) // End of Line
	}

2. Lex and Format lines individually then remove the common prefix and suffix

There is no need to send the whole excerpt to the lexer and formatter in one go.

So long as you keep hold of the lexer state you can lexer and feed each line to the formatter individually.

Looking at the formatter code it should be clear that every line will start with <span class="line"><span class"cl"> and end with </span></span>

Therefore if we feed lines in to the lexer and formatter one at a time we will always be able to just return the inner subset of the string.

3. Split lines using the suffix-prefix

Even if we don't want to do that - there is still no need to parse the html output from chroma. You can simply remove the initial <span class="line"><span class"cl"> and terminal </span></span> and just strings.Split(formatted, `</span></span><span class="line"><span class"cl">`) to get the highlighted lines.

[zeripath]

I guess I also don't understand what the 
 is supposed to be doing.

[silverwind]

Chroma has added an option to prevent these spans from being created in the first place.

Right, it's then obvious that we should upgrade first and set that Chroma option to remove the span wrappers. Ideally we may then get away without modifying HTML, but I have my doubts about it because of this empty line copying issue.

Maybe we should start a fresh PR. I'm not sure I have the bandwidth to do so in the next days, so if @wxiaoguang wants to take a stab, please go ahead. It will probably also require a update in the diff code after the Chroma upgrade.

[wxiaoguang]

Hmm... I have a feeling that we're going about this the wrong way.
Interestingly this differs from the code we currently have in Gitea because of: alecthomas/chroma@d18e8a4

At that time, it was the right way for old Chroma. (I missed that Chroma just got the InlineCode and other options).

Looking at the formatter code it should be clear that every line will start with <span class"cl"> and end with

Replacing these tags without a parser are fragile, it's not right. The simple parser could make it be able to co-work with new Chroma versions even if the layout changes (now there are InlineCode and other options, so I also agree that the parser might not be needed). About strings.Split(formatted, `</span></span><span class="line"><span class"cl">`), you can read the old code like 948190e , if you think that was right, I have nothing to argue, while I insist that was not.

I guess I also don't understand what the 
 is supposed to be doing.

That's in comment, to make the test clear: make it clear to do tests: "
" is the real newline for rendering, '\n' is ignorable/trim-able and could be ignored. You can take the old tests if you like.

Maybe we should start a fresh PR. I'm not sure I have the bandwidth to do so in the next days, so if wxiaoguang wants to take a stab, please go ahead. It will probably also require a update in the diff code after the Chroma upgrade.

Sorry but I have no time (for the upgrading) either, feel free to close this one and open a new one for the upgrading. I agree that since there are InlineCode and other options in Chroma, this PR's parser might not be needed.

(ps: when upgrading Chroma, if there is any issue in the diff code, I can help to fix ASAP, or just leave the diff code issues to me and @ me).

[wxiaoguang]

@zeripath

Now, in 49f62a4 , there is no tag parser, just use Split+Trim as your wish.

[zeripath]

I'm bowing out from reviewing this PR. I've said my piece regarding this.

I do not believe that the tag parser is necessary. I've explained that instead we should just use our knowledge of the output that comes from the formatter as I proposed in my PR.

I am deeply disturbed by the proposal to use the magic code 
 too. I do not understand why it is necessary.

I will not review further.

[silverwind]

I am deeply disturbed by the proposal to use the magic code too. I do not understand why it is necessary.

I was also against this. It seems just weird to abuse this token for our purposes when browsers can render literal newlines just fine.

[wxiaoguang]

OK, since many people do not like the HTML entity, then 025e3d9 removed it.

But I do not know how to rewrite the tests to satisfy everyone (especially for when a test case failed, how to show the diff to developers clearly). Please edit the tests directly, I am fine with any approach.

---

All disagreements should have been resolved now.

[6543]

.

[silverwind]

Seems to complicated to backport, even with 3310dd1 applied I get merge conflicts, let's skip this.

[wxiaoguang]

There should be no hard dependency between these 2 PRs.

Maybe, for backport, could a simple and quick fix be fine? Maybe zeripath's approach can be backported without conflict, it doesn't change too much.

[wxiaoguang]

I tried to make a backport with zeripath's approach.

• Fix the new-line copy-paste for rendered code #20612

[wxiaoguang]

• If users want to hide the last empty line or show a block emoji (🚫) for no-newline when rendering, the new code is easier to do so in the future.

-> Show a no-EOL mark for the last line when viewing a file #27391