Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Paragraph indentation and RTL text #513

Closed
simplej opened this Issue · 12 comments

3 participants

@simplej

Looks like ":indent_paragraphs => 60" doesn't work for RTL text. The text isn't indented. It works for LTR.

PS: Fantastic library, the sole reason I learned Ruby. :)

@practicingruby practicingruby added bug-confirmed and removed stale labels
@practicingruby

Sorry for the long delay in response.

It looks like indentation is being added, but on the left side, which is almost certainly the wrong behavior for RTL text. Reopening this for a while to see if anyone wants to investigate further.

@practicingruby

@simplej or someone else with this problem, can you confirm the fix on #795 works as you'd want it to?

@practicingruby

Merged #795 with a test, hope it resolves this issue.

@elad
Collaborator

I have tested both :indent_paragraphs and :indent blocks and I don't think the issue has been resolved. Here's code and output for English and Hebrew:

English:

Prawn::Document.generate("english-indent.pdf") do
  text "English without indentation"
  text "English with indentation", :indent_paragraphs => 60
end

screen shot 2015-01-19 at 4 35 22 am

Hebrew:

require "prawn"
require "bidi"

Prawn::Document.generate("hebrew-indent.pdf") do
  font_families.update("Arial" => {
    :normal => "Arial.ttf",
  });
  font("Arial")

  self.text_direction = :rtl

  bidi = Bidi.new
  s = bidi.render_visual "עברית ללא הזחה"
  text s
  s = bidi.render_visual "עברית עם הזחה"
  text s, :indent_paragraphs => 60
end

screen shot 2015-01-19 at 4 35 35 am

(the same applies to :indent blocks; I didn't include them for brevity.)

@elad elad reopened this
@practicingruby

The issue here is that for whatever reason (we'd need to investigate), setting the global text_direction does not pass :direction => :rtl to the text() method, so the fixed code never runs, and the :ltr indent is used by default.

Here's a code sample which I think works correctly:

require "bidi"

require_relative "lib/prawn"

Prawn::Document.generate("hebrew-indent.pdf") do
  font("#{Prawn::BASEDIR}/data/fonts/DejaVuSans.ttf")

  bidi = Bidi.new
  s = bidi.render_visual "עברית ללא הזחה"
  text s, :direction => :rtl
  s = bidi.render_visual "עברית עם הזחה"
  text s, :indent_paragraphs => 60, :direction => :rtl
end

(note I changed the font and require to make it easier for others to test with against master, but your original font will have the same result)

So, a fix for this would involve writing a test similar to the one on #795, but using the global text direction method rather than specifying direction on each text call. Then, figure out how that default gets handled internally, and update accordingly. It may be as simple as assuming that if :direction isn't specified that we should use doc.text_direction, rather than hardcoding to :ltr.

We should probably fix this before the next release. @elad if you have time to even start on this by writing the test and open a pull request, that'd be helpful. Otherwise I'll take a look when I find time.

@elad
Collaborator

The issue here is that for whatever reason (we'd need to investigate), setting the global text_direction does not pass :direction => :rtl to the text() method, so the fixed code never runs, and the :ltr indent is used by default.

I'm not too familiar with the code, but looking at the text method I don't see where options would even inherit anything from the global document settings. I do see options[:document] = self in inspect_options_for_text, but not much else. So, again, unless I'm missing something, options.fetch(:direction, :ltr) will always return :ltr unless :direction was set specifically in the options passed to text.

Using this code in draw_indented_formatted_line fixes text for me:

direction = options[:direction] ? options[:direction] : options[:document].text_direction
gap = direction == :ltr ? [@indent_paragraphs, 0] : [0, @indent_paragraphs]

I can submit it in a pull request, but it doesn't work for indent blocks:

require "bidi"
require_relative "prawn/lib/prawn"

Prawn::Document.generate("hebrew-indent.pdf") do
  font("#{Prawn::BASEDIR}/data/fonts/DejaVuSans.ttf")

  self.text_direction = :rtl

  bidi = Bidi.new

  s = bidi.render_visual "עברית ללא הזחה"
  text s

  s = bidi.render_visual "עברית עם הזחה 1"
  text s, :indent_paragraphs => 60

  self.indent 60 do
    s = bidi.render_visual "עברית עם הזחה 2"
    text s
  end
end

The string marked with 1 gets indented properly with the above fix, but the string marked with 2 doesn't. I suspect it's because self isn't the document in its context. Looks like it's because draw_indented_formatted_line doesn't get called when text is called from an indent block.

Once we settle on a fix I can adjust the tests as well. If I understand correctly we want to test for:

  • Direction set globally on the document (above fix)
  • Direction passed to text (works)
  • Direction set globally on the document used from indent block (unresolved)
@practicingruby

I'm not too familiar with the code, but looking at the text method I don't see where options would even inherit anything from the global document settings.

The relationship is inverted, :direction gets passed down into the initializer of a Formatted::Text::Box, which will default to doc.text_direction if the :direction key is not provided.

So instead of doing options.fetch(:direction, :ltr), we could probably do options.fetch(:direction, text_direction) and that would work. self is the document object, so you don't need to dig into the options hash.

With this rewritten method, your original example seems to work correctly for me:

    def draw_indented_formatted_line(string, options)
      gap = options.fetch(:direction, text_direction) == :ltr ?
              [@indent_paragraphs, 0] : [0, @indent_paragraphs]

      indent(*gap) do
        fill_formatted_text_box(string, options.dup.merge(:single_line => true))
      end
    end

The indent() method is not text-specific, and so you'd need to explicitly use something like this to put the indentation on the right side:

Prawn::Document.generate("hebrew-indent.pdf") do
  font("#{Prawn::BASEDIR}/data/fonts/DejaVuSans.ttf")

  self.text_direction = :rtl

  bidi = Bidi.new

  indent(0, 60) do
    s = bidi.render_visual "עברית עם הזחה"
    text s
  end
end

So I don't think there's a bug there. Let me know if I misunderstood you, though. Even using :indent_paragraphs within an indent block seems to look OK.

If I got this right, the change should be the one line revision to options.fetch(:direction, ...) combined with a test for the global text_direction attribute. If you can do a pull request for that I should be able to merge quickly.

@practicingruby

@elad: Note I had misunderstood a part of your comment about indent(), so I updated the above to comment to make it clearer.

@elad
Collaborator

Thanks for making the fix cleaner! I verified and it works. You're also right about proper usage of indent - I was using it wrong, there's no bug there.

I'd like to adjust the tests. Do we want two tests for each (RTL/LTR) case? That is:

  • rtl via document
  • rtl via text
  • ltr via document
  • ltr via text

If so, I'll add the relevant tests for the LTR case as well.

@practicingruby

@elad I guess it wouldn't hurt to test each case, so go for it. Also, thanks for verifying this in general, I had merged with fingers crossed there because I didn't get a response back on this ticket from the original reporter.

@elad
Collaborator

Done, I'd appreciate it if you could review the changes. What's the policy for Prawn? Should I push a branch or use my own fork for the pull request?

@practicingruby

@elad, Make a branch off the main repo, this way any contributor could help revise it where needed. Happy to review when you're ready.

@elad elad closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.