Base64 encode fonts and images for PDF rendering #1363

hayesr · 2017-05-17T04:47:15Z

Due to the repo split and image digests, the paths for images do not correspond to their location on disk, this prevents Prince from finding them and breaks any rendering with images.

Depends on ManageIQ/manageiq#15131 to write encoded files for Prince.

Problems to fix

Files to adjust

pdf_summary.css
pdf_report.css

This change

Creates new image tag and path helpers to encode images when necessary for PDFs
Breaks up current PDF stylesheets and uses Asset Pipeline to encode assets
Moves the spec for PdfGenerator back to main repo

Addresses
https://bugzilla.redhat.com/show_bug.cgi?id=1447940

Currently

After

hayesr · 2017-05-17T19:23:13Z

@miq-bot add_labels bug, pending core

@dclarizio This is ready for a look.

martinpovolny · 2017-05-18T12:03:54Z

app/helpers/image_encode_helper.rb

+  def encodable_image_tag(source, options = {})
+    image_tag(encodable_image_source(source), options)
+  end
+  alias eimage_tag encodable_image_tag


why do we need this alias (and the one below -- eimage_source)?

Just a future convenience. Any image that might ever make it into a PDF should use this helper.

not a fan of "future convenience" reasons. Would prefer we just name the method the way we want the API to look, and that should be the defined API.

martinpovolny · 2017-05-18T12:08:12Z

app/helpers/image_encode_helper.rb

+
+    if asset.content_type == 'image/svg+xml'
+      encoding = 'charset=utf-8'
+      data = CGI.escape(asset.source).gsub('+', '%20')


ERB::Util.url_encode probably? I know it does the same but in other cases we prefered the Active*/Rails variants over CGI

himdel · 2017-05-18T15:04:04Z

@hayesr Looks like none of these pdf-related css files are ever precompiled, which means they get compiled on the appliance, the first time somebody tries to use the css. Is that so? Shouldn't you add them to some asset precompile array?

himdel · 2017-05-18T15:04:47Z

app/helpers/quadicon_helper.rb

@@ -477,7 +467,7 @@ def render_host_quadicon(item, options)
      output << flobj_img_simple('100/shield.png', "g72") unless item.get_policies.empty?
    else
      output << flobj_img_simple
-      output << flobj_img_small(img_for_host_vendor(item), "e72")
+      output << flobj_img_simple(img_for_host_vendor(item), "e72", 64)


Why this change? Seems like you're just undoing a simplification, without ever calling it with another value.

flobj_img_simple seemed redundant, we can probably get rid of using the size altogether now.

This is a change I made early in the process. It makes the PR too noisy now, I'll just revert these.

Thanks, I have no real objection to removing it, especially if we can get rid of that size completely :) Just confused me here, thanks :)

skateman · 2017-05-18T15:08:56Z

@himdel @hayesr isn't that possible that these files aren't going through the asset pipeline at all? I'm not familiar how our PDF renderer works, but we might need to look at this.

hayesr · 2017-05-18T17:01:49Z

@skateman We render PDFs using Prince, and by sending an HTML blob and a single stylesheet file. Currently the files don't go through the pipeline, they're just static css files. When everything was in one directory and in a predictable place that worked fine. Now we need to worry about fonts and images that might be in multiple gem directories. Encoding them means the paths don't matter.

hayesr · 2017-05-18T20:30:17Z

@himdel You're correct in that these would be compiled on first-use. Precompiling is definitely an option, I'm not sure how much time or resources it will save. Not all users need PDF functionality, and it adds like ~250k but that's not a huge deal. I'm neutral on the idea.

himdel · 2017-05-19T12:11:28Z

@hayesr I'm pretty neutral about precompiling too... From those tree-related BZs with missing .png, I remember that asset rebuild taking a few minutes .. which would be a strong pro-precompiling argument. But if this doesn't trigger the whole rebuild and takes only a few seconds, no problem there :).

himdel · 2017-05-22T16:25:18Z

app/helpers/image_encode_helper.rb

+  def base64_encoded_uri(source)
+    asset = Rails.application.assets[source]
+
+    if asset.content_type == 'image/svg+xml'


Do we actually need a special SVG branch here?

Guess what, base64 encoded images are smaller :) - for our SVGs, the size sum is 1215 kB URL-encoded and 1189 kB base64 encoded.

So, simpler code and smaller images...

Yeah, Prince prefers the straight SVG sometimes.

Not encoded:

Encoded

Plus, it's not always better.

Prince prefers straight SVG sometimes..

Are you sure that's not a bug in the base64 branch of code? Unless prince's uri parser is signifcantly broken, this should not happen, so it feels like this may be exposing a bug there.

Are you sure there's not a working non-home-cooked way of converting files to data uris?

Guess what, base64 encoded images are smaller :) - for our SVGs, the size sum is 1215 kB URL-encoded and 1189 kB base64 encoded.

Sounds like we need to run all of our images through an optimizer.

This is more about base64 reliably taking 8/6ths of space, while URL-encoding a SVG tends to preserve most bytes, but for about a fifth, it makes them 3 times larger.

But really, my concern here is mostly about having a hand-rolled solution to encode URLs in 2 different ways. If we used something ..established, I don't care what the resulting format is.

Are you sure there's not a working non-home-cooked way of converting files to data uris?

I googled for a while but did not find one.

Even URI::Data supports just parsing them. Oh well.

himdel · 2017-05-22T16:26:48Z

app/helpers/image_encode_helper.rb

+      data = Base64.encode64(asset.source)
+    end
+
+    "data:#{asset.content_type};#{encoding},#{data}"


Surely there must be a gem/something written specifically for the purpose of converting binary data to data URIs.

Doing this by hand sounds wrong.

Yeah, Base64 encodes data into data URIs 😜

Seriously though, Sprockets does some of this, but it seems difficult to use outside of a stylesheet. You can compare this method with what I have. And why would we want another dependency?

hayesr · 2017-05-30T13:05:53Z

Restart CI

hayesr · 2017-06-05T15:38:41Z

Restart CI

Fryguy · 2017-06-07T15:28:34Z

app/controllers/application_controller.rb

@@ -157,6 +158,9 @@ def download_summary_pdf
    @embedded = true
    @showlinks = false

+    # encode images and embed in HTML that is sent to Prince
+    @base64_encode_images = true


Upstream code does not have Prince by default, though I think the only other choice is the NullPdf plugin. Even so, if/when we ever create a new PDF plugin, will this base64 encoding of images still be needed there? If not, should this be a property of the PDF plugin system?

Additionally, I don't really like this instance variable hanging out here. I think it might be better if the ImageEncodeHelper has an attr_accessor for this value (it already has the predicate getter), and then either the default is set in that getter on first access, or the value is set here through the setter self.base64_encode_images = true

This might allow for better testing as well, so you can just call a method instead of doing some nasty instance_variable_set in the test.

Fryguy · 2017-06-07T15:34:31Z

spec/helpers/quadicon_helper_spec.rb

@@ -98,6 +98,8 @@
 # FIXME: complex describe blocks mirror the existing complex control flow

 describe QuadiconHelper do
+  helper(ImageEncodeHelper)


Super-minor, but helper here is a DSL-like method, which usually we avoid the parens on

Fryguy · 2017-06-07T15:35:02Z

spec/lib/pdf_generator_spec.rb

-    stub_generator_instance
-    expect(PdfGenerator.pdf_from_string("html", "css")).to eq "pdf-data"
-  end
-end


Why is this removed?

Moves the spec for PdfGenerator back to main repo

Oh duh... I need to lrn2read

Moved it back to the main repo to be with the corresponding file.

Fryguy · 2017-06-07T15:42:56Z

Overall, LGTM. Most of my comments are nitpicks, which could be a follow up PR.

Not sure about the svg thing, but it feels like that could be an issue that can be opened and worked on separately from this PR. This PR keeps it in a working state, and research on why it doesn't work as base64 can be a follow up.

martinpovolny · 2017-06-07T16:23:26Z

I'd say that with the exception of #1363 (review) -- there's a strong consensus that we don't need the alias -- we can merge this one.

Can we, @himdel or have I overlooked something?

himdel · 2017-06-07T16:25:13Z

@martinpovolny I'd like to see those issues fixed, which is why I haven't pushed the button.

But agreed that there's no serious blocker here and definitely nothing that couldn't be fixed later.

* Encode FontAwesome and Patternfly icon fonts * Add new image tag and path helpers to encode assets when necessary * Move pdf_generator_spec to main repo

miq-bot · 2017-06-07T19:53:43Z

Checked commits hayesr/manageiq-ui-classic@bd879b6~...4d2d963 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
21 files checked, 0 offenses detected
Everything looks fine. 🍪

Base64 encode fonts and images for PDF rendering (cherry picked from commit 87ddd8f) https://bugzilla.redhat.com/show_bug.cgi?id=1460815

simaishi · 2017-06-16T13:04:53Z

Fine backport details:

$ git log -1
commit 0b19b05bf4aee4f132aeed2b6b355f372dfd80aa
Author: Dan Clarizio <[email protected]>
Date:   Fri Jun 9 12:43:47 2017 -0700

    Merge pull request #1363 from hayesr/compiled_pdf_css
    
    Base64 encode fonts and images for PDF rendering
    (cherry picked from commit 87ddd8fe860bcafac4aac6f638754636ecc88db1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460815

simaishi · 2017-07-14T17:07:33Z

@dclarizio Backporting this to Euwe branch results in conflicts in multiple files, please create a separate Euwe PR or suggest other PR(s) to backport.

epwinchell · 2017-07-17T17:48:53Z

@simaishi @dclarizio PDF styling seems to be fine in Euwe. I think it was fixed with this PR: ManageIQ/manageiq#15504

epwinchell · 2017-07-17T17:51:58Z

@miq-bot rm_label euwe/yes

epwinchell · 2017-07-17T17:53:20Z

@miq-bot rm_label euwe/conflict

hayesr mentioned this pull request May 17, 2017

[WIP] Base64 encode images for PDFs #1090

Closed

5 tasks

miq-bot added the wip label May 17, 2017

hayesr force-pushed the compiled_pdf_css branch from f7aa272 to d2cdd7a Compare May 17, 2017 18:28

hayesr mentioned this pull request May 17, 2017

Compile and write stylesheets for PDF rendering ManageIQ/manageiq#15131

Merged

hayesr changed the title ~~[WIP] Base64 encode fonts and images for PDF rendering~~ Base64 encode fonts and images for PDF rendering May 17, 2017

miq-bot removed the wip label May 17, 2017

miq-bot added bug pending core labels May 17, 2017

hayesr force-pushed the compiled_pdf_css branch from d2cdd7a to 22b5d37 Compare May 17, 2017 19:25

dclarizio requested a review from epwinchell May 17, 2017 19:49

dclarizio self-assigned this May 17, 2017

dclarizio added formatting/styling euwe/yes labels May 17, 2017

dclarizio requested a review from himdel May 17, 2017 20:49

martinpovolny reviewed May 18, 2017

View reviewed changes

himdel reviewed May 18, 2017

View reviewed changes

hayesr force-pushed the compiled_pdf_css branch from 22b5d37 to c79a5f1 Compare May 18, 2017 18:15

himdel reviewed May 22, 2017

View reviewed changes

hayesr force-pushed the compiled_pdf_css branch from c79a5f1 to 61d9874 Compare May 22, 2017 19:04

simaishi mentioned this pull request May 23, 2017

Fix broken stylesheet path for PDFs ManageIQ/manageiq#14793

Merged

hayesr closed this Jun 5, 2017

hayesr reopened this Jun 5, 2017

hayesr force-pushed the compiled_pdf_css branch from d9c3504 to e2f8b41 Compare June 5, 2017 20:08

Fryguy reviewed Jun 7, 2017

View reviewed changes

epwinchell approved these changes Jun 7, 2017

View reviewed changes

hayesr added 4 commits June 7, 2017 13:48

Base64 encode fonts and images for PDF rendering

bd879b6

* Encode FontAwesome and Patternfly icon fonts * Add new image tag and path helpers to encode assets when necessary * Move pdf_generator_spec to main repo

Use ERB::Util over CGI

e2ae91c

Sprockets uses strict_encode

5b220e9

Addressing feedback

4d2d963

hayesr force-pushed the compiled_pdf_css branch from e2f8b41 to 4d2d963 Compare June 7, 2017 19:49

dclarizio merged commit 87ddd8f into ManageIQ:master Jun 9, 2017

dclarizio added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 9, 2017

simaishi pushed a commit that referenced this pull request Jun 16, 2017

Merge pull request #1363 from hayesr/compiled_pdf_css

0b19b05

Base64 encode fonts and images for PDF rendering (cherry picked from commit 87ddd8f) https://bugzilla.redhat.com/show_bug.cgi?id=1460815

simaishi added fine/backported and removed fine/yes labels Jun 16, 2017

simaishi added the euwe/conflict label Jul 14, 2017

miq-bot removed the euwe/yes label Jul 17, 2017

miq-bot removed the euwe/conflict label Jul 17, 2017

Base64 encode fonts and images for PDF rendering #1363

Base64 encode fonts and images for PDF rendering #1363

Conversation

hayesr commented May 17, 2017 • edited Loading

hayesr commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented May 18, 2017

hayesr commented May 18, 2017

hayesr commented May 18, 2017

himdel commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayesr commented May 30, 2017

hayesr commented Jun 5, 2017

Choose a reason for hiding this comment

Fryguy Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 7, 2017

martinpovolny commented Jun 7, 2017

himdel commented Jun 7, 2017

miq-bot commented Jun 7, 2017

simaishi commented Jun 16, 2017

simaishi commented Jul 14, 2017

epwinchell commented Jul 17, 2017

epwinchell commented Jul 17, 2017

epwinchell commented Jul 17, 2017

hayesr commented May 17, 2017 •

edited

Loading

Fryguy Jun 7, 2017 •

edited

Loading