Defect #30434

Line height is too large when previewing files with syntax highlighting if the line terminators are CRLF

Added by Go MAEDA 5 months ago. Updated 5 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Attachments
Target version:4.0.2
Resolution:Fixed Affected version:4.0.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.

line-height-too-large@2x.png (13 KB) Go MAEDA, 2019-01-15 12:01

30434-remove-cr-chars.patch Magnifier (1.43 KB) Yuichi HARADA, 2019-01-28 01:12

30434-remove-cr-chars-v2.patch Magnifier (1.19 KB) Go MAEDA, 2019-02-02 06:11

30434-remove-cr-chars-v3.patch Magnifier (1.48 KB) Go MAEDA, 2019-02-03 07:22


Related issues

Related to Redmine - Feature #24681: Syntax highlighter: replace CodeRay with Rouge Closed

Associated revisions

Revision 17847
Added by Go MAEDA 5 months ago

Line height is too large when previewing files with syntax highlighting if the line terminators are CRLF (#30434).

Patch by Yuichi HARADA.

Revision 17848
Added by Go MAEDA 5 months ago

Merged r17847 from trunk to 4.0-stable (#30434).

History

#1 Updated by Yuichi HARADA 5 months 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.

#2 Updated by Go MAEDA 5 months ago

  • Target version set to 4.0.2

Confirmed that the patch fixes this issue. Setting the target version to 4.0.2.

#3 Updated by Go MAEDA 5 months ago

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.

#4 Updated by Go MAEDA 5 months ago

  • Related to Feature #24681: Syntax highlighter: replace CodeRay with Rouge added

#5 Updated by Marius BALTEANU 5 months 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.

#6 Updated by Go MAEDA 5 months 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"?

#7 Updated by Marius BALTEANU 5 months 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

#8 Updated by Go MAEDA 5 months ago

Added a comment as discussed in #30434#note-6 and #30434#note-7.

#9 Updated by Go MAEDA 5 months ago

  • Status changed from New to Resolved
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed. Thank you for fixing this issue.

#10 Updated by Go MAEDA 5 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF