Project

General

Profile

Actions

Defect #29259

closed

Attachment preview does not work for some source files such as JavaScript and Go

Added by Stephan Wenzel over 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

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]]


Files


Related issues

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

Actions
Blocked by Redmine - Defect #31285: Syntax highlighting does not work for attachments with .pl extensionClosedGo MAEDA

Actions
Actions #1

Updated by Stephan Wenzel over 6 years 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
Actions #2

Updated by Go MAEDA over 6 years 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?
Actions #3

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

Updated by Go MAEDA over 6 years ago

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

Updated by Go MAEDA over 6 years ago

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

Updated by Go MAEDA about 6 years ago

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

Updated the patch.

  • Changed not to use MimeMagic gem
  • Added a test.
Actions #7

Updated by Go MAEDA about 6 years ago

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

Updated by Go MAEDA about 6 years ago

Updated the patch. Simplified Attachment#is_text?.

Actions #10

Updated by Marius BĂLTEANU about 6 years ago

Go MAEDA wrote:

Updated the patch. Simplified Attachment#is_text?.

Go Maeda, the patch contains some undesired changes.

Actions #11

Updated by Pavel Rosický about 6 years 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.

Actions #12

Updated by Stephan Wenzel about 6 years 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.

Actions #13

Updated by Go MAEDA about 6 years ago

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

Updated by Go MAEDA about 6 years 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.

Actions #15

Updated by Go MAEDA over 5 years ago

I completely rewrote the patch to support the new syntax highlighter Rouge introduced in Redmine 4.0. The patch has to be applied after the patch attached to #31285.

After applying the patch, files supported by the syntax highlighter are treated as text.

Actions #16

Updated by Go MAEDA over 5 years ago

  • Blocked by Defect #31285: Syntax highlighting does not work for attachments with .pl extension added
Actions #17

Updated by Go MAEDA over 5 years ago

  • Tracker changed from Patch to Defect
  • Subject changed from Attachments Controller squanders CodeRay's capabilities to Attachment preview does not support source code in some languages
  • Target version changed from Candidate for next major release to 4.0.4

Setting the target version to 4.0.4.

Actions #18

Updated by Go MAEDA over 5 years ago

  • Subject changed from Attachment preview does not support source code in some languages to Attachment preview does not work for some source files such as JavaScript and Go
  • Status changed from New to Resolved
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the fix.

Actions #19

Updated by Go MAEDA over 5 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF