Vote #79765
完了Line height is too large when previewing files with syntax highlighting if the line terminators are CRLF
0%
説明
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.
!{width: 305px; border: 1px solid #ccc;}.line-height-too-large@2x.png!
journals
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.
<pre><code class="diff">
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))
</code></pre>
I attached a patch.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
We should mark this fix as "workaround/quick fix" until rouge team fixes the problem on their side.
I found a "PR":https://github.com/jneen/rouge/pull/723 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
--------------------------------------------------------------------------------
Added a comment as discussed in #30434#note-6 and #30434#note-7.
--------------------------------------------------------------------------------
Committed. Thank you for fixing this issue.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
related_issues
relates,Closed,24681,Syntax highlighter: replace CodeRay with Rouge