Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ImageModel missing :data option key #149

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

victorpolko
Copy link

This PR is intended to fix the issue #131 and may be a duplicate of #132 by @yholkamp (I noticed that PR after the fix), but it:

  1. adds a test case that fails without the fix
  2. doesn't mess with ImageModel#relationship_target, which is only used to retrieve the file extension

It also relates to issue #27 in a way that even after this fix there's no actual possibility to create a non-corrupted image from Base64 data:

# /lib/caracal/document.rb#L222
def render_media(zip)
  images = relationships.select { |r| r.relationship_type == :image }
  images.each do |rel|
    if rel.relationship_data.to_s.size > 0
      content = rel.relationship_data
    else
      content = open(rel.relationship_target).read
    end

    zip.put_next_entry("word/#{ rel.formatted_target }")
    zip.write(content)
  end
end

The read method that's used with the File object when no :data was supplied returns binary data, not Base64-encoded.
Similarly, if you do pass :data to the img method, it has to be binary, too, and you'll get nice and valid resulting image:

doc.img image_path,
  data: File.read(image_path), # !! not Base64.encode64(File.read(image_path))
  width: 100,
  height: 100

With all that said, I guess that caracal-example has to be updated:

# app/views/examples/show.docx.caracal#L22
data = Base64.encode64(File.read('public/plia-login.png'))
docx.img 'public/plia-login.png', data: data, width: 400, height: 124, align: :right
# => 
data = File.read('public/plia-login.png')
docx.img 'public/plia-login.png', data: data, width: 400, height: 124, align: :right

I don't mind taking care of it after this PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant