Defect #30434
closed
Line height is too large when previewing files with syntax highlighting if the line terminators are CRLF
0%
Description
The height of some lines is too large if the line terminators of the file are CRLF. Only Redmine 4.0.0 is affected. No problem in prior versions.
In the following screenshot, The height of line 1 and 2 is too large.
Files
Related issues
Updated by Yuichi HARADA about 5 years ago
The rouge gem is splitting a character string with LF ("\n"). Therefore, output HTML containing CR ("\r"). As a result, we saw it with two rows of height. I replaced CR with LF as follows.
diff --git a/lib/redmine/syntax_highlighting.rb b/lib/redmine/syntax_highlighting.rb
index e2d47f277..36612bc8c 100644
--- a/lib/redmine/syntax_highlighting.rb
+++ b/lib/redmine/syntax_highlighting.rb
@@ -76,6 +76,7 @@ module Redmine
# Highlights +text+ as the content of +filename+
# Should not return line numbers nor outer pre tag
def highlight_by_filename(text, filename)
+ text.gsub!(/\r\n?/, "\n")
lexer =::Rouge::Lexer.guess_by_filename(filename)
formatter = ::Rouge::Formatters::HTML.new
::Rouge.highlight(text, lexer, CustomHTMLLinewise.new(formatter))
I attached a patch.
Updated by Go MAEDA about 5 years ago
- Target version set to 4.0.2
Confirmed that the patch fixes this issue. Setting the target version to 4.0.2.
Updated by Go MAEDA about 5 years ago
Slightly updated the patch.
- Fix that the first argument of
highlight_by_filenameis destructively changed bygsub!. - Changed the test method name from
test_syntax_highlight_should_remove_cr_charstotest_syntax_highlight_should_normalize_line_endingsbecausegsub(/\r\n?/, "\n")not only simply removes CR characters but also replaces single CR with LF.
Updated by Go MAEDA about 5 years ago
- Related to Feature #24681: Syntax highlighter: replace CodeRay with Rouge added
Updated by Marius BĂLTEANU about 5 years ago
We should mark this fix as "workaround/quick fix" until rouge team fixes the problem on their side.
I found a PR which fixes this problem for other languages, but unfortunately, is not accepted yet. I've tested the solution proposed by that PR and it works as expected, I'll try to make a new PR.
Updated by Go MAEDA about 5 years ago
Marius BALTEANU wrote:
We should mark this fix as "workaround/quick fix" until rouge team fixes the problem on their side.
Do you think it will be OK if we add a comment like "TODO: Delete the following line when Rouge is improved to handle CRLF properly"?
Updated by Marius BĂLTEANU about 5 years ago
Go MAEDA wrote:
Marius BALTEANU wrote:
We should mark this fix as "workaround/quick fix" until rouge team fixes the problem on their side.
Do you think it will be OK if we add a comment like "TODO: Delete the following line when Rouge is improved to handle CRLF properly"?
Yes, and you can point to Redmine issue number and the PR just opened by me to fix the issue: https://github.com/jneen/rouge/pull/1078
Updated by Go MAEDA about 5 years ago
Added a comment as discussed in #30434#note-6 and #30434#note-7.
Updated by Go MAEDA about 5 years ago
- Status changed from New to Resolved
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed. Thank you for fixing this issue.