Defect #30434
closed
Line height is too large when previewing files with syntax highlighting if the line terminators are CRLF
Added by Go MAEDA over 5 years ago.
Updated about 5 years ago.
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
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.
- Target version set to 4.0.2
Confirmed that the patch fixes this issue. Setting the target version to 4.0.2.
Slightly updated the patch.
- Fix that the first argument of
highlight_by_filename is destructively changed by gsub!.
- Changed the test method name from
test_syntax_highlight_should_remove_cr_chars to test_syntax_highlight_should_normalize_line_endings because gsub(/\r\n?/, "\n") not only simply removes CR characters but also replaces single CR with LF.
- Related to Feature #24681: Syntax highlighter: replace CodeRay with Rouge added
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.
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"?
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
- Status changed from New to Resolved
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed. Thank you for fixing this issue.
- Status changed from Resolved to Closed
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by