Patch #29259

Attachments Controller squanders CodeRay's capabilities

Added by Stephan Wenzel 5 months ago. Updated 3 months ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Attachments
Target version:Candidate for next major release

Description

Redmine squanders the capabilities of CodeRay for previewing code files in Redmine's preview pane. A very prominent squandering is the negligence of javascript files, for which Redmine only shows that no preview is available.

AttachmentsController#show method only tests: @attachment.is_text? - The function Attachment::is_text? only relies on Redmine::MimeType.is_type?('text', filename). The mime type of javascript, however, is "application/javascript". Here Redmine misses what CodeRay can do.

I propose the following patches:

Add function 'is_code?' to Attachment.rb

def is_code?
  ::CodeRay::FileType[filename].present?
end

Patch AttachmentsController.rb (Redmine 3.4.6)

- elsif @attachment.is_text? && @attachment.filesize <= Setting.file_max_size_displayed.to_i.kilobyte
+ elsif (@attachment.is_text? || @atachments.is_code?) && @attachment.filesize <= Setting.file_max_size_displayed.to_i.kilobyte

These two patches add previews for cfc, cfm, clj, cpp, cu, cxx, c++, C, dpr, gemspec, go, groovy, gvy, haml, hpp, h++, html.erb, js, lua, mab, pas, phtml, prawn, py3, pyw, raydebug, rjs, rpdf, ru, rxml, sass, taskpaper, template, tmproj, xaml

I have provided a plugin that applies these patches as a proof of concept:

[[https://www.redmine.org/plugins/redmine_more_code]]

allow-preview-for-any-text-files.diff Magnifier (2.54 KB) Go MAEDA, 2018-08-05 09:31

allow-preview-for-any-text-files-v2.diff Magnifier (5.38 KB) Go MAEDA, 2018-08-10 10:03


Related issues

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

History

#1 Updated by Stephan Wenzel 5 months ago

Difference between is_text? based on Redmine::MimeType and proposed is_code? based on CodeRay::FileType

is_text? is_code?
c c
cc cc
cfc
cfm
clj
cpp cpp
cs
csh
css css
csv
cu
cxx
c++
C
diff diff
dpr
erb erb
gemspec
go
groovy
gvy
h h
haml
hh hh
hpp
h++
htm htm
html html
html.erb
ini
install
java java
js
json
jsp
lua
mab
mxml
pas
patch patch
phtml
php php
php3 php3
php4 php4
php5 php5
pl
pm
po
properties
prawn
py py
py3
pyw
rake rake
raydebug
rb rb
rbw rbw
readme
rhtml rhtml
rjs
rpdf
ru
ruby
rxml
sass
sh
sql sql
taskpaper
template
tmproj
tpl
txt
upgrade
xaml
xhtml xhtml
xml xml
xsd
yaml yaml
yml yml

#2 Updated by Go MAEDA 5 months ago

Thank you for reporting this issue.

I think the following code can detect more kinds of text files. Redmine already uses mimemagic gem (source:tags/3.4.6/lib/redmine/thumbnail.rb#L33).

Index: app/models/attachment.rb
===================================================================
--- app/models/attachment.rb    (revision 17449)
+++ app/models/attachment.rb    (working copy)
@@ -235,7 +235,8 @@
   end

   def is_text?
-    Redmine::MimeType.is_type?('text', filename)
+    Redmine::MimeType.is_type?('text', filename) ||
+      File.open(diskfile) {|f| MimeMagic.by_magic(f).try(:text?)} rescue false
   end

   def is_image?

#3 Updated by Go MAEDA 5 months ago

The is_text? method should be called last for performance reason.

Index: app/models/attachment.rb
===================================================================
--- app/models/attachment.rb    (revision 17452)
+++ app/models/attachment.rb    (working copy)
@@ -235,7 +235,8 @@
   end

   def is_text?
-    Redmine::MimeType.is_type?('text', filename)
+    Redmine::MimeType.is_type?('text', filename) ||
+      File.open(diskfile) {|f| MimeMagic.by_magic(f).try(:text?)} rescue false
   end

   def is_image?
@@ -259,7 +260,7 @@
   end

   def previewable?
-    is_text? || is_image? || is_video? || is_audio?
+    is_image? || is_video? || is_audio? || is_text?
   end

   # Returns true if the file is readable

#4 Updated by Go MAEDA 5 months ago

  • Category set to Attachments
  • Target version set to Candidate for next major release

#5 Updated by Go MAEDA 5 months ago

  • Target version changed from Candidate for next major release to 4.1.0

#6 Updated by Go MAEDA 5 months ago

  • File allow-preview-for-any-text-files.diff added

Updated the patch.

  • Changed not to use MimeMagic gem
  • Added a test.

#7 Updated by Go MAEDA 5 months ago

  • File deleted (allow-preview-for-any-text-files.diff)

#9 Updated by Go MAEDA 4 months ago

Updated the patch. Simplified Attachment#is_text?.

#10 Updated by Marius BALTEANU 4 months ago

Go MAEDA wrote:

Updated the patch. Simplified Attachment#is_text?.

Go Maeda, the patch contains some undesired changes.

#11 Updated by Pavel Rosický 4 months ago

! Redmine::Utils.binary?(File.read(diskfile, 4096)) rescue false

see
http://blog.honeybadger.io/benchmarking-exceptions-in-ruby-yep-theyre-slow/
https://robots.thoughtbot.com/don-t-inline-rescue-in-ruby

IMO text-type detection should be evaluated by mime only, reading the content this way is just guessing.

#12 Updated by Stephan Wenzel 4 months ago

Why not stick with a function „is_code?“ as I already proposed?

A negative test of a file on it‘s binary property may be a too powerful „catch all“. A „catch all“ based on it‘s non-binary character may raise questions how f.i. an „.html“-file should be treated in the preview pane - it (the html-file) has a non binary character (thus, is „text“) but a non-textual representation may be appropriate. The same is true for non-binary .pdf or .svg files. Just because a file‘s content is not binary, does not necessarily mean that a text-representation is most appropriate.

The example „is_code?“ function based on CodeRay, which does the preview coloring of code, would leave other plugins room for further previewers for files in text form, for which a non-textual representation is appropriate.

I propose to always leave the mime test to the respective previewer, else there will always be a mismatch.

Eventually, it might be useful to implement a hook in the format block of attachments_controller#show method, that allows an easy integration for highly specialised previewers.

#13 Updated by Go MAEDA 3 months ago

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

#14 Updated by Go MAEDA 3 months ago

  • Target version changed from 4.1.0 to Candidate for next major release

Since we have moved from CodeRay to Rouge, we cannot use the proposed is_code method.

Also available in: Atom PDF