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

Add :dry_run to text_box and formatted_text_box #913

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamjohnford
Copy link
Contributor

I made a patch based on @practicingruby's suggestions on issue #763. The main changes are that text_box and formatted_text_box return the box object and allow access to attributes such as font_size and height. Passing :dry_run => true provides a way to get font sizes from :shrink_to_fit and box heights before rendering text to the pdf.

Changed text_box and formatted_text_box to return the box object and allow access font_size.
@packetmonkey
Copy link
Contributor

Thanks for the PR! My concern is that this would change the return value for an API that was marked as stable, which would necessitate a major version bump.

I think I get what you are trying to do and agree this result could be more useful. What does everyone think of having this functionality in a new lower level API call that you could make and get the box return value, and the existing API call calls that lower level value, then returns the render result so we maintain backwards API compatibility?

I'm also concerned with you changed an API return and it didn't look like you had to change an existing test so we probably have at least one missing test that should be written as well.

@iamjohnford
Copy link
Contributor Author

Yeah, I noticed your concern when I was working on the patch. I probably should have asked about that sooner.

One thing I noticed while testing this was that height_of doesn't take into account :inline_format. It should, right? So instead of:

def height_of(string, options = {})
  height_of_formatted([{ :text => string }], options)
end

It should be something like:

def height_of(string, options = {})
  options = options.dup

  if p = options[:inline_format]
    p = [] unless p.is_a?(Array)
    options.delete(:inline_format)
    array = self.text_formatter.format(string, *p)
  else
    array = [{ :text => string }]
  end

  height_of_formatted(array, options)
end

The heights are pretty different between the two as you can see with this example:

Prawn::Document.generate("test-height-of.pdf") do
  string = "Prawn love <sup>superscript</sup> " * 10

  options = {
    inline_format: true,
    size: 20,
    width: 180
  }

  height = height_of(string, options)
  puts "height_of: #{height}"
  bounding_box [0, bounds.height], width: options[:width], height: height do
    stroke_bounds
    text string, options
  end

  array = text_formatter.format(string, options)
  height = height_of_formatted(array, options)
  puts "height_of_formatted: #{height}"
  bounding_box [options[:width] + 10, bounds.height], width: options[:width], height: height do
    stroke_bounds
    text string, options
  end
end

Does that seem correct? I'm happy to create a pull request with a patch for that.

@practicingruby
Copy link
Member

@packetmonkey: What if we only return the box when :dry_run is set? This is backwards compatible because it'd keep the return value the same for any code following the 2.0 API, but would provide the box object if you use the newly introduced :dry_run param

In Prawn 3.0, we could consider unifying the interface.

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

Successfully merging this pull request may close these issues.

None yet

3 participants