Skip to content

Syntax highlighting should be full-file only #33358

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

Closed
silverwind opened this issue Jan 22, 2025 · 9 comments · Fixed by #33766
Closed

Syntax highlighting should be full-file only #33358

silverwind opened this issue Jan 22, 2025 · 9 comments · Fixed by #33766
Labels
topic/content-rendering Changes how certain filetypes are displayed topic/ui Change the appearance of the Gitea UI type/bug

Comments

@silverwind
Copy link
Member

silverwind commented Jan 22, 2025

Description

Diff lines are passed individually line-by-line to chroma, which its a massive source of highlighting bugs because the highlighter needs to see the whole file to highlight correctly. We should instead:

  • Highlight both the old and new files individually, store the resulting HTML in Maps of (lineText, lineHTML).
  • For each content line in the diff, look into the maps and if there is an exact match, replace the line with the highlighted HTML line.

This assumes chroma does not output HTML tags that span multiple lines, but I think this may already be the case.

@lunny
Copy link
Member

lunny commented Jan 22, 2025

Is it possible to highlight a specific part of a file if we know the file type? Since markdown allows highlighting within embedded code sections, this could be an effective way to focus on relevant content. Instead of rendering the entire file, we could adopt a similar approach to diff views, where only a portion of the file is displayed by default.

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2025

Is it possible to highlight a specific part of a file if we know the file type

No, chroma assumes it is being passed a full file (basically all syntax highlighters work that way). Any random section of a file is not guaranteed to highlight correctly, only the whole file content is.

@lunny
Copy link
Member

lunny commented Jan 22, 2025

Is it possible to highlight a specific part of a file if we know the file type

No, chroma assumes it is being passed a full file (basically all syntax highlighter work that way). Any random section of a file is not guaranteed to highlight correctly, only the whole file content is.

So do you mean all code sections embedded in the issue markdown are not guaranteed to highlight correctly?

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2025

You mean those inline code blocks that render when linking to code in discussions? They are also affected. They need to pass the whole content for highlighting purpose and then only render a subsection of the highlighted HTML. This kind of rendering is best-effort thought because if the snippet is incomplete code, there will be highlighting bugs.

Basically our interface with chroma should be to only ever pass full files to it, and then reduce the returned highlighted lines to what we need to display.

@wxiaoguang
Copy link
Contributor

A related PR but stale: fix highlight problem of git diff #24336

@silverwind silverwind changed the title Diff syntax highlighting should not be line-by-line Syntax highlighting should be full-file only Jan 28, 2025
@warasilapm
Copy link

warasilapm commented Feb 27, 2025

@silverwind I noticed that multi-line block comments in my C code were not highlighting correctly.

Image

Any chance this is a symptom of this issue? I missed the linked issue #23176. Probably just another case of this.

@silverwind
Copy link
Member Author

Yes that is exactly what this issue is about. 90% of highlighting bugs are this issue, the remaining 10% are bugs in the chroma dependency 😉.

@warasilapm
Copy link

warasilapm commented Feb 27, 2025

Does this issue need a champion? I'm not particularly handy with go but I could find some time or see if I can nerdsnipe a friend who is. Thoughts on using #24336 as a starting point?

@silverwind
Copy link
Member Author

silverwind commented Feb 28, 2025

I don't think anyone is working on this currently, so feel free to. #24336 seems to have the right idea but from a quick glance, I'm concerned that it did not remove any line-by-line diff code. Imho, all such code needs to be replaced.

hiifong pushed a commit to hiifong/gitea that referenced this issue Mar 10, 2025
Fix go-gitea#33358, fix go-gitea#21970

This adds a step in the `GitDiffForRender` that does syntax highlighting for the
entire file and then only references lines from that syntax highlighted
code. This allows things like multi-line comments to be syntax
highlighted correctly.

---------

Co-authored-by: wxiaoguang <[email protected]>

(cherry picked from commit 3f1f808)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/content-rendering Changes how certain filetypes are displayed topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants