Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

PDF conversion patch + Collect a submission patch [corrected] #732

Merged
merged 21 commits into from

4 participants

@nicolasbouillon

New conversion process:

Use of the RubyGem RGhost instead of ImageMagick
Competitive conversion time
Output : a collection of jpg images, one image for each pdf page
(previous process gave only one image for the entire pdf file, leading to limiting the number of pages).
Possibility to grade a pdf file with a great number of pages.

Collect a submission :

Due to Rails 3.0 migration, the collecting of a submissions gave a routing issue.
Adding the required lines in routes.rb resolves the problem.

app/models/submission_file.rb
((30 lines not shown))
end
+ # Convert a pdf file into a an array of jpg files (one jpg file = one page of the pdf file)
+ RGhost::Convert.new(File.join(storage_path, self.filename)).to :jpeg, :filename => file_path, :multipage => true, :resolution => 150
@jerboaa Owner
jerboaa added a note

It would be good to do error checking here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jerboaa jerboaa commented on the diff
config/routes.rb
@@ -129,8 +129,12 @@
resources :results do
collection do
- post 'update_mark'
- post 'expand_criteria'
+ get 'update_mark'
+ get 'expand_criteria'
+ get 'collapse_criteria'
+ get 'expand_unmarked_criteria'
+ get 'edit'
+ get 'download'
@jerboaa Owner
jerboaa added a note

It would be good to have tests for routes we depend on. Could add tests for this? assert_recognizes should do this for you. There should be some existing tests around using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jerboaa
Owner

@nicolasbouillon Nice work! Thanks. Fix the two concerns and this should be good to go from my point of view.

@benjaminvialle

I tested the patch and the result is really impressive ! Good work.

I still noticed 8 errors doing functionnal tests. They are on edit function in results_controller.

@nicolasbouillon

@benjaminvialle I don't understand : I have just run the functional tests again, and I get no error at all...

@benjaminvialle

mmhh, did you push again your branch?

I'm going to checkout again your repository an re start tests.

@benjaminvialle

Ok, I was not using the good branch. Shame on me!

All tests pass ! That's perfect :)

We will look at the two points Severin asked and this will be ready to be merged!

@NelleV
Owner

I've pushed a patch.
Thanks,

app/models/submission_file.rb
((22 lines not shown))
begin
- # This is an ImageMagick command, see http://www.imagemagick.org/script/convert.php for documentation
- # For some reason, ImageMagick seemed to fail silently when not
- # redirecting the output to a log file. Let's redirect the output to
- # "something"
- `convert -limit memory #{MarkusConfigurator.markus_config_pdf_conv_memory_allowance} -limit map 0 -density 150 -resize 66% #{File.join(storage_path, self.filename)} -append #{file_path} >> #{File.join(RAILS_ROOT, "log", "export-pdf.log")}`
- # Sometimes, ImageMagick fails silently
- if File.exists?(file_path)
- m_logger.log("Successfully converted pdf file to jpg")
- else
- m_logger.log("Problem in PDF conversion")
- end
+ RGhost::Convert.new(File.join(storage_path,
@jerboaa Owner
jerboaa added a note

Does this always raise an exception? I'm asking because of this: https://github.com/shairontoledo/rghost/wiki/Converting-PDF-into-other-formats See the checking for errors section. Thoughts?

@NelleV Owner
NelleV added a note

I've checked both methods by uninstalling ghostscripts (so the conversion doesn't work). None of the methods seems to work (the flag isn't set, and no exception is raised.). We could check manually that the pdf files aren't converted but that seems like a terrible hack to me.

@jerboaa Owner
jerboaa added a note

Unless the RGhost doc is way out of date, there has to be a reason why they suggest to do error checking that way. I could see that it may report errors if the conversion failed due to other reasons such as out-of-memory or corrupted PDFs? I suggest to do the error checking as suggested by the RGhost documentation anyway. Feel free to keep the rescue block as well :) Otherwise this is good to go from my point of view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NelleV
Owner

I've modified the way we detect errors in PDF conversion, as RGhost doesn't raise errors, but uses a boolean flag.

@jerboaa
Owner

Looks good. Feel free to merge.

@benjaminvialle benjaminvialle merged commit dc2c0f1 into MarkUsProject:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 8, 2012
  1. Merge branch '637' of https://github.com/hoboman313/Markus into explo…

    CentraleNantes authored
    …ration
Commits on Feb 18, 2012
  1. @nicolasbouillon
  2. @nicolasbouillon
  3. @nicolasbouillon
  4. @nicolasbouillon
Commits on Feb 19, 2012
  1. @nicolasbouillon
  2. @nicolasbouillon
  3. @nicolasbouillon
Commits on Mar 6, 2012
  1. @nicolasbouillon
  2. @nicolasbouillon
Commits on Mar 17, 2012
  1. @nicolasbouillon

    spelling correction

    nicolasbouillon authored
  2. @nicolasbouillon
  3. @nicolasbouillon
  4. @nicolasbouillon
  5. @nicolasbouillon
Commits on Mar 22, 2012
  1. @nicolasbouillon
  2. @nicolasbouillon
  3. @nicolasbouillon
Commits on Mar 27, 2012
  1. @NelleV
Commits on Mar 30, 2012
  1. @NelleV
Commits on Mar 31, 2012
  1. @NelleV
This page is out of date. Refresh to see the latest.
View
8 Gemfile
@@ -71,3 +71,11 @@ end
group :mongrel do
gem "mongrel_cluster"
end
+
+# If you want to be able to view and annotate PDF files,
+# make sure that this group is included. GhostScript has to be
+# installed for rghost to work well. You also need to set
+# the PDF_SUPPORT bool to true in the config file(s).
+group :rghost do
+ gem "rghost"
+end
View
2  Gemfile.lock
@@ -95,6 +95,7 @@ GEM
rbx-require-relative (0.0.5)
rcov (0.9.10)
rdoc (3.9.2)
+ rghost (0.8.7.6)
routing-filter (0.2.4)
actionpack
ruby-debug (0.10.4)
@@ -136,6 +137,7 @@ DEPENDENCIES
rails
rcov
rdoc
+ rghost
routing-filter
ruby-debug
rubyzip
View
20 app/controllers/results_controller.rb
@@ -198,8 +198,8 @@ def download
elsif file.is_pdf? && !params[:show_in_browser].nil?
send_file File.join(MarkusConfigurator.markus_config_pdf_storage,
file.submission.grouping.group.repository_name, file.path,
- filename.split('.')[0] + '.jpg'), :type => "image",
- :disposition => 'inline', :filename => filename
+ filename.split('.')[0] + '_' + sprintf("%04d" % params[:file_index].to_s()) + '.jpg'),
+ :type => "image", :disposition => 'inline', :filename => filename
else
send_data file_contents, :filename => filename
end
@@ -234,6 +234,22 @@ def codeviewer
return
end
@code_type = @file.get_file_type
+
+ # if dealing with a pdf file, get the number of images to display
+ if @file.is_pdf?
+ i = 1
+ storage_path = File.join(MarkusConfigurator.markus_config_pdf_storage,
+ @file.submission.grouping.group.repository_name,
+ @file.path)
+ filePathToCheck = File.join(storage_path, @file.filename.split('.')[0] + '_' + sprintf("%04d" % i.to_s()) + '.jpg')
+ while File.exists?(filePathToCheck)
+ i += 1
+ filePathToCheck = File.join(storage_path, @file.filename.split('.')[0] + '_' + sprintf("%04d" % i.to_s()) + '.jpg')
+ end
+ i -= 1
+ @nb_pdf_files_to_download = i
+ end
+
render :template => 'results/common/codeviewer'
end
View
38 app/models/submission_file.rb
@@ -109,25 +109,27 @@ def convert_pdf_to_jpg
self.path)
file_path = File.join(storage_path, self.filename.split('.')[0] + '.jpg')
self.export_file(storage_path)
+
# Remove any old copies of this image if they exist
- FileUtils.remove_file(file_path, true) if File.exists?(file_path)
- m_logger.log("Starting pdf conversion from #{File.join(storage_path, self.filename)} to #{file_path}")
- # ImageMagick can only save images with heights not exceeding 65500 pixels.
- # Larger images result in a conversion failure.
- begin
- # This is an ImageMagick command, see http://www.imagemagick.org/script/convert.php for documentation
- # For some reason, ImageMagick seemed to fail silently when not
- # redirecting the output to a log file. Let's redirect the output to
- # "something"
- `convert -limit memory #{MarkusConfigurator.markus_config_pdf_conv_memory_allowance} -limit map 0 -density 150 -resize 66% #{File.join(storage_path, self.filename)} -append #{file_path} >> #{File.join(RAILS_ROOT, "log", "export-pdf.log")}`
- # Sometimes, ImageMagick fails silently
- if File.exists?(file_path)
- m_logger.log("Successfully converted pdf file to jpg")
- else
- m_logger.log("Problem in PDF conversion")
- end
- rescue Exception => e
- m_logger.log("Pdf file couldn't be converted")
+ i = 1
+ filePathToRemove = File.join(storage_path,
+ self.filename.split('.')[0] + '_' + sprintf("%04d" % i.to_s()) + '.jpg')
+ while File.exists?(filePathToRemove)
+ FileUtils.remove_file(filePathToRemove, true)
+ i += 1
+ filePathToRemove = File.join(storage_path,
+ self.filename.split('.')[0] + '_' + sprintf("%04d" % i.to_s()) + '.jpg')
+ end
+
+ # Convert a pdf file into a an array of jpg files (one jpg file = one page
+ # of the pdf file)
+ file = RGhost::Convert.new(File.join(storage_path,
+ self.filename)).to :jpeg,
+ :filename => file_path,
+ :multipage => true,
+ :resolution => 150
+ if file.error
+ m_logger.log("rghost: Image conversion error")
end
FileUtils.remove_file(File.join(storage_path, self.filename), true)
View
33 app/views/results/common/_pdf_codeviewer.html.erb
@@ -0,0 +1,33 @@
+<div id="image_container" class="image_container">
+ <div id="sel_box"></div>
+ <%# Create a box for each image annotation %>
+ <% @annots.each do |annot| %>
+ <div id="annotation_holder_<%=annot.annotation_text.id%>" class="annotation_holder"></div>
+ <%end%>
+ <div id="image_preview">
+ <%# First image displayed with a different alt message, in case of boolean PDF_SUPPORT set to false %>
+ <li>
+ <%= image_tag(url_for(:action => 'download', :controller => 'results', :select_file_id => submission_file_id, :show_in_browser => true, :file_index => 1), :alt => I18n.t("common.cant_display_image"), :width => "950") %>
+ </li>
+ <% (2..nb_files).each do|i| %>
+ <li>
+ <%= image_tag(url_for(:action => 'download', :controller => 'results', :select_file_id => submission_file_id, :show_in_browser => true, :file_index => i), :alt => I18n.t("common.image_downloading"), :width => "950") %>
+ </li>
+ <% end %>
+ </div>
+
+ <%# Keep track of all annotations associated with file %>
+ <input id="annotation_grid"type="hidden" value="<%=h(@file.get_annotation_grid.to_json)%>"></input>
+ <input id="enable_annotations?"type="hidden" value=<%=(@current_user.ta? || @current_user.admin?) && !@result.released_to_students%>></input>
+ <script type="text/javascript">
+ //<![CDATA[
+ source_code_ready_for_image();
+ <% if (defined? annots) %>
+ <% annots.each do |annot| %>
+ add_annotation_text(<%=annot.annotation_text.id%>,
+ '<%=simple_format(h(escape_javascript(annot.annotation_text.content.to_s)))%>');
+ <% end %>
+ <% end %>
+ //]]>
+ </script>
+</div>
View
17 app/views/results/common/codeviewer.rjs
@@ -1,20 +1,27 @@
#Render the source code for syntax highlighting...
begin
page['code_pane'].onmousemove = nil
- #Non-binary files
+ # Non-binary files
if !SubmissionFile.is_binary?(@file_contents)
page.replace_html 'codeviewer',
:partial => 'results/common/text_codeviewer',
:locals => { :uid => params[:uid], :file_contents => @file_contents,
:annots => @annots, :code_type => @code_type}
- #Supported image files and pdfs
- elsif @file.is_supported_image? || @file.is_pdf?
+ # Supported image files
+ elsif @file.is_supported_image?
page.replace_html 'codeviewer',
:partial => 'results/common/image_codeviewer',
:locals => { :uid => params[:uid], :file_contents => @file_contents,
:annots => @annots, :code_type => @code_type,
:submission_file_id => @submission_file_id}
- #Rest of the files
+ # Pdf files
+ elsif @file.is_pdf?
+ page.replace_html 'codeviewer',
+ :partial => 'results/common/pdf_codeviewer',
+ :locals => { :uid => params[:uid], :file_contents => @file_contents,
+ :annots => @annots, :code_type => @code_type,
+ :nb_files => @nb_pdf_files_to_download,
+ :submission_file_id => @submission_file_id}
else
page.replace_html 'codeviewer',
:partial => 'results/common/text_codeviewer',
@@ -23,7 +30,7 @@
:annots => @annots, :code_type => @code_type}
end
- #Also update the annotation_summary_list
+ # Also update the annotation_summary_list
if @current_user.ta? || @current_user.admin?
page.replace_html 'annotation_summary_list',
:partial => 'results/marker/annotation_summary',
View
6 app/views/submissions/update_converted_pdfs.rjs
@@ -7,12 +7,16 @@ if submission_collector.safely_stop_child_exited
submission_collector.safely_stop_child_exited = false
submission_collector.save
page.redirect_to :controller => 'results', :action => 'edit',
- :id => @grouping.current_submission_used.result.id
+ :assignment_id => @grouping.assignment_id,
+ :submission_id => @grouping.current_submission_used.id,
+ :id => @grouping.current_submission_used.result.id
end
else
submission_collector.safely_stop_child_exited = false
submission_collector.save
page.redirect_to :controller => 'results', :action => 'edit',
+ :assignment_id => @grouping.assignment_id,
+ :submission_id => @grouping.current_submission_used.id,
:id => @grouping.current_submission_used.result.id
end
end
View
30 config/environments/development.rb
@@ -108,6 +108,7 @@
###################################################################
# Directory where converted PDF files will be stored as JPEGs. Make sure MarkUs
# is allowed to write to this directory
+
PDF_STORAGE = "#{::Rails.root.to_s}/data/dev/pdfs"
###################################################################
@@ -118,32 +119,9 @@
###################################################################
# Set this to true or false if you want to be able to display and annotate
# PDF documents within the browser.
- PDF_SUPPORT = false
-
- ###################################################################
- # In order for markus to display pdfs, it converts them to jpg format via
- # ImageMagick first. The conversion process is very expensive and can quickly
- # eat up all available RAM and swap memory available to the server causing it
- # to crash. The solution to this is to set a limit on how much RAM ImageMagick
- # is allowed to use, forcing it to use the hard-disk for all the needs exceeding
- # the allowance.
- #
- # Using hard-disk memory for conversion is significantly slower than using RAM.
- # Increasing the memory allowance will help speed up conversion speeds.
- #
- # Caution: setting the allowance too high will result in ImageMagick using all
- # the server RAM and will crash it. Be sure that you can afford the memory you
- # allow.
- #
- # The maximum amount of megabytes that the ImageMagick pdf conversion process
- # may use. This setting is NOT a limit on MarkUs's total memory use. It only
- # limits the ImageMagick conversion process.
- #
- # 100 mbs should be enough to quickly convert most submissions around 10 pages
- # long.
- #
- # This setting doesn't matter unless PDF_SUPPORT is set to true
- PDF_CONV_MEMORY_ALLOWANCE = 100
+ # When collecting pdfs files, it converts them to jpg format via RGhost.
+ # RGhost is ghostscript dependent. Be sure ghostscript is installed.
+ PDF_SUPPORT = false
###################################################################
# Change this to 'REPOSITORY_EXTERNAL_SUBMITS_ONLY = true' if you
View
29 config/environments/production.rb
@@ -142,32 +142,9 @@
###################################################################
# Set this to true or false if you want to be able to display and annotate
# PDF documents within the browser.
- PDF_SUPPORT = true
-
- ###################################################################
- # In order for markus to display pdfs, it converts them to jpg format via
- # ImageMagick first. The conversion process is very expensive and can quickly
- # eat up all available RAM and swap memory available to the server causing it
- # to crash. The solution to this is to set a limit on how much RAM ImageMagick
- # is allowed to use, forcing it to use the hard-disk for all the needs exceeding
- # the allowance.
- #
- # Using hard-disk memory for conversion is significantly slower than using RAM.
- # Increasing the memory allowance will help speed up conversion speeds.
- #
- # Caution: setting the allowance too high will result in ImageMagick using all
- # the server RAM and will crash it. Be sure that you can afford the memory you
- # allow.
- #
- # The maximum amount of megabytes that the ImageMagick pdf conversion process
- # may use. This setting is NOT a limit on MarkUs's total memory use. It only
- # limits the ImageMagick conversion process.
- #
- # 100 mbs should be enough to quickly convert most submissions around 10 pages
- # long.
- #
- # This setting doesn't matter unless PDF_SUPPORT is set to true
- PDF_CONV_MEMORY_ALLOWANCE = 100
+ # When collecting pdfs files, it converts them to jpg format via RGhost.
+ # RGhost is ghostscript dependent. Be sure ghostscript is installed.
+ PDF_SUPPORT = false
###################################################################
# Change this to 'REPOSITORY_EXTERNAL_SUBMITS_ONLY = true' if you
View
30 config/environments/test.rb
@@ -117,33 +117,9 @@
###################################################################
# Set this to true or false if you want to be able to display and annotate
# PDF documents within the browser.
- PDF_SUPPORT = true
-
- ###################################################################
- # In order for markus to display pdfs, it converts them to jpg format via
- # ImageMagick first. The conversion process is very expensive and can quickly
- # eat up all available RAM and swap memory available to the server causing it
- # to crash. The solution to this is to set a limit on how much RAM ImageMagick
- # is allowed to use, forcing it to use the hard-disk for all the needs exceeding
- # the allowance.
- #
- # Using hard-disk memory for conversion is significantly slower than using RAM.
- # Increasing the memory allowance will help speed up conversion speeds.
- #
- # Caution: setting the allowance too high will result in ImageMagick using all
- # the server RAM and will crash it. Be sure that you can afford the memory you
- # allow.
- #
- # The maximum amount of megabytes that the ImageMagick pdf conversion process
- # may use. This setting is NOT a limit on MarkUs's total memory use. It only
- # limits the ImageMagick conversion process.
- #
- # 100 mbs should be enough to quickly convert most submissions around 10 pages
- # long.
- #
- # This setting doesn't matter unless PDF_SUPPORT is set to true
- PDF_CONV_MEMORY_ALLOWANCE = 100
-
+ # When collecting pdfs files, it converts them to jpg format via RGhost.
+ # RGhost is ghostscript dependent. Be sure ghostscript is installed.
+ PDF_SUPPORT = false
###################################################################
# Change this to 'REPOSITORY_EXTERNAL_SUBMITS_ONLY = true' if you
View
3  config/locales/en.yml
@@ -295,7 +295,8 @@ en:
submission_file: "Submission File:"
test_results: "Test Results:"
no_file_in_repository: "[No files in repository]"
- cant_display_image: "Image cannot be displayed. If you are trying to view a pdf file its probably too big to display or you have turned off pdf support. Click on the Download button to download the file instead"
+ cant_display_image: "Image cannot be displayed. If you are trying to view a pdf file, please check if you have turned on pdf support. Click on the Download button to download the file instead"
+ image_downloading: "Pdf page downloading. Please wait."
test_code: "Test Code"
criterion_incomplete_error: "Mark unfilled but marking state was set to complete"
View
3  config/locales/fr.yml
@@ -295,7 +295,8 @@ fr:
submission_file: "Fichier envoyé : "
test_results: "Résultat des tests : "
no_file_in_repository: "[Pas de fichiers dans le dépôt]"
- cant_display_image: "L'image ne peut pas être affichée. Si vous essayez d'afficher un PDF, ou le fichier est trop grand pour être affiché, ou l'option permettant d'afficher les PDF est désactivée. Vous pouvez télécharger manuellement le fichier."
+ cant_display_image: "L'image ne peut pas être affichée. Si vous essayez d'afficher un PDF, vérifiez que l'option permettant d'afficher les PDF est activée. Vous pouvez télécharger manuellement le fichier."
+ image_downloading: "Page pdf en cours de téléchargement. Veuillez patienter."
test_code: "Code pour les tests"
criterion_incomplete_error: "Note non remplie, mais état fixé comme terminé"
View
8 config/routes.rb
@@ -129,8 +129,12 @@
resources :results do
collection do
- post 'update_mark'
- post 'expand_criteria'
+ get 'update_mark'
+ get 'expand_criteria'
+ get 'collapse_criteria'
+ get 'expand_unmarked_criteria'
+ get 'edit'
+ get 'download'
@jerboaa Owner
jerboaa added a note

It would be good to have tests for routes we depend on. Could add tests for this? assert_recognizes should do this for you. There should be some existing tests around using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
member do
View
17 test/functional/results_controller_test.rb
@@ -16,6 +16,23 @@ def teardown
SAMPLE_ERR_MSG = "sample error message"
+ should "recognize routes" do
+ assert_recognizes({:controller => "results",
+ :action => "update_mark",
+ :assignment_id => '1',
+ :submission_id => '1'},
+ {:path => 'assignments/1/submissions/1/results/update_mark',
+ :method => :get})
+
+ assert_recognizes({:controller => "results",
+ :action => "expand_criteria",
+ :assignment_id => '1',
+ :submission_id => '1'},
+ {:path => 'assignments/1/submissions/1/results/expand_criteria',
+ :method => :get})
+
+ end
+
context "A user" do
# Since we are not authenticated and authorized, we should be redirected
Something went wrong with that request. Please try again.