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

Format rotated cell text #32

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

Conversation

straydogstudio
Copy link

I must admit, it is ludicrous how long it has taken me to get to this. But here we are.

This formats text appropriately for text rotated between 0 and 90 degrees. See below:

prawn-table

What remains:

  • Angles between 0 and -90. Should other angles be considered?
  • Right to left text. How should it behave?
  • Skewed cells (for table headers.)

@elad
Copy link
Member

elad commented Nov 7, 2014

In case this is what you're asking: For right-to-left, it should look horizontally flipped, so that for example blocks 01-06 start from the top right and block 07 starts from the bottom right. If it's not, please clarify. :)

@straydogstudio
Copy link
Author

@elad That's exactly what I was expecting. Flipping the behavior.

@johnnyshields
Copy link
Contributor

👍 +1 Impressive work!!

@@ -1,2 +1,3 @@
Gemfile.lock
manual.pdf
rotate.pdf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like that line wasn't meant to be committed.

@hbrandl
Copy link
Contributor

hbrandl commented Nov 11, 2014

The screen shots look awesome! Great work! Thank you very much for the hard work!

With new features like yours I'm trying to keep the code complexity as low as possible to ensure future bug fixes are easily possible. We had lots of issues with tables at the end of last year, and my hope is to stabilize the code base. To ensure that this is possible I need others and myself to be able to read the code as easily as possible.
Would it thus be possible to refactor the code a little bit? Especially Prawn::Table::Cell::Formatted::Box?
It would be awesome if you could manage to get an A from codeclimate on the added code e.g. https://codeclimate.com/github/straydogstudio/prawn-table/Prawn::Table::Cell::Formatted::Box
I'd settle for a B if more refactoring doesn't benefit readability.

Also would be it be possible for you to add an entry to the manual?

Thank you once more for the awesome work that you've already done!

@practicingruby
Copy link
Member

@hbrandl: Refactored code would be nice, but formal high-level documentation of the computations we're implementing is better. Keep in mind that an "A" in code climate does not imply understandability beyond the elimination of local incidental complexity, so I expect it will only help a little bit with preventing bugs from surfacing, or re-surfacing.

@practicingruby
Copy link
Member

In other words, clean code will certainly help make fixes easier, but documentation of how things are supposed to work has both preventative effect and an educational effect that clean code can't gain us. Both are desirable, of course.

@hbrandl
Copy link
Contributor

hbrandl commented Nov 11, 2014

@sandal I agree. Both things are important. And refactoring can also go too far. So let me relax that statement above. A is the goal. Anything less should have a reason.

@practicingruby
Copy link
Member

I really don't think this is a good idea to depend so heavily on Code Climate scores. They only measure extremely local internal quality issues!

I care much more about these kinds of things, which code climate cannot help us with:

  • How likely is it that this code is going to break existing behavior?
  • How likely is it that we're going to write new code that will depend on this feature?
  • What (mathematical) computations are we using here and why?
  • What edge cases are there to consider, and what will this code do when it encounters those edge cases (correctly render, raise an exception, incorrectly render, undefined behavior)?
  • What side effects, if any, does this code have on other systems?
  • Do our tests reflect whatever we've learned from the above questions?

Please understand that if you're emphasizing external stability, internal code quality is only an indirect measure, and it's not the first or best place to focus our energies. It can act as a good supplement to the above, but only if we give them the attention they deserve first.

@hbrandl
Copy link
Contributor

hbrandl commented Nov 12, 2014

@sandal I've given your posting quite some thought. And I agree with you. From now on I'll ignore codeclimate metrics for prawn-table.

@practicingruby
Copy link
Member

@hbrandl: I use code climate metrics for Prawn core occasionally, but usually for my own cleanup work, rather than asking contributors to pay attention to them. I should have probably brought this up over email (and may still do that), sorry for not doing that.

So... in the interest of getting back on topic, let's take a look at the code!

@straydogstudio: I'm really excited about this feature, it's something we've been looking to add to tables for a while. But as @hbrandl mentioned, we've extracted prawn-table because it's unstable and very high complexity, so we need to be careful when merging new patches.

One thing that would help a lot is a high-level overview of what's going on in the Prawn::Table::Cell::Formatted::Box class. What I'd like to know is what documentation (if any) you used to figure out the rotation computations, and also what methods we've overridden from Text::Formatted::Box and why.

As a general note I'm a little nervous that it's a subclass of Prawn::Text::Formatted::Box, because we may accidentally break it with upstream changes (composition might be better), but we can ignore that for the moment. Right now, I just want to make sure we understand how the code works.

@straydogstudio
Copy link
Author

Somehow I never got any messages that you guys responded here. I'm not very responsive! My profuse apologies. I just thought this got lost in the shuffle.

I have lots of notes on the way that it was done. And I will look everything over again. I have also spent a lot of time thinking on how it could be simplified. And spent more time thinking on right to left text. So I will document it some more.

A lot of the rotation calculations are rather tedious trigonometry work, mainly accounting for the height of the line itself and the corners of the cell. Really you are positioning a rectangle inside a rectangle.

By the way, one thing I found was that the height inside the cell essentially leaves no padding on the bottom. I did several checks and found the height was always a bit tall. Rotated text would always sit on the bottom edge. Not necessarily exactly, though. I will try to reproduce this. I did some searching but never did find where this got produced. Ostensibly the padding had been accounted for.

@practicingruby
Copy link
Member

@straydogstudio: Thanks. The basic idea is that if we can have some decent documentation on how the code is supposed to work, we can verify that (a) the testing is adequate and (b) that the code can be revised as needed to make it more maintainable. We'll be able to help with both of those things as long as we know how things are supposed to work.

If you can provide a code example and/or screenshot to illustrate the padding issue you saw, that would be helpful!

@hmt
Copy link

hmt commented Mar 7, 2015

This looks very nice but I get width problems:

data = [%w(some text that I just made up for demonstration purposes only)]                                                          
table data, :cell_style =>{:height => 80, :valign => :bottom, :rotate => 90}

will look like this:
screenshot from 2015-03-07 22 46 10

It looks like cell width is still calculated by regular text width and not by rotated width. So I assumed I could just set :width => xx:

data = [%w(some text that I just made up for demonstration purposes only)]                                                          
table data, :cell_style =>{:height => 80, :valign => :bottom, :rotate => 90, :width => 30}

which then looks like this:
screenshot from 2015-03-07 22 49 29

@JoshTGreenwood
Copy link

I'm seeing the same behavior as @hmt

@mauromorales
Copy link

I just popped into this issue as well, @hmt, @JoshTGreenwood did any of you managed to get past the width problem?

@AnwarShah
Copy link

I came here following this bug report prawnpdf/prawn#409. I am using prawn-rails gem in Rails 4. Can I just pull the repo with PR and use it in Rails?

@AnwarShah
Copy link

AnwarShah commented Oct 28, 2016

@marcusramberg @hmt @JoshTGreenwood I've found that setting :valign creates the problem. If I remove :valign option all is fine. I also asked a question on StackOverflow before but have to answer myself http://stackoverflow.com/questions/40139848/bottom-to-top-text-in-column-of-prawn-table/

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

9 participants