Project

General

Profile

Actions

Defect #30434

closed

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

Added by Go MAEDA almost 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Attachments
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

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

Related to Redmine - Feature #24681: Syntax highlighter: replace CodeRay with RougeClosedJean-Philippe Lang

Actions
Actions #1

Updated by Yuichi HARADA almost 6 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.

Actions #2

Updated by Go MAEDA over 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.

Actions #3

Updated by Go MAEDA over 5 years 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.
Actions #4

Updated by Go MAEDA over 5 years ago

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

Updated by Marius BĂLTEANU over 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.

Actions #6

Updated by Go MAEDA over 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"?

Actions #7

Updated by Marius BĂLTEANU over 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

Actions #8

Updated by Go MAEDA over 5 years ago

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

Actions #9

Updated by Go MAEDA over 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.

Actions #10

Updated by Go MAEDA over 5 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF