Redmine

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

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?

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

Go MAEDA wrote:

Updated the patch. Simplified Attachment#is_text?.

Go Maeda, the patch contains some undesired changes.

! 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.

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.