Vote #79440
完了Attachment preview does not work for some source files such as JavaScript and Go
0%
説明
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]]]
journals
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|
--------------------------------------------------------------------------------
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).
<pre><code class="diff">
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?
</code></pre>
--------------------------------------------------------------------------------
The @is_text?@ method should be called last for performance reason.
<pre><code class="diff">
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
</code></pre>
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Updated the patch.
* Changed not to use MimeMagic gem
* Added a test.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Updated the patch. Simplified @Attachment#is_text?@.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Updated the patch. Simplified @Attachment#is_text?@.
Go Maeda, the patch contains some undesired changes.
--------------------------------------------------------------------------------
<pre>
! Redmine::Utils.binary?(File.read(diskfile, 4096)) rescue false
</pre>
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.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Since we have moved from CodeRay to Rouge, we cannot use the proposed @is_code@ method.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Setting the target version to 4.0.4.
--------------------------------------------------------------------------------
Committed the fix.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
related_issues
relates,Closed,24681,Syntax highlighter: replace CodeRay with Rouge
blocks,Closed,31285,Syntax highlighting does not work for attachments with .pl extension