Custom page size

[Kevindum]

Hello, Since Northern America is the only place where the default format paper is US Letter (21.6 × 27.9 cm), and many people (myself included) need to use A4 format (21 × 29.7 cm), I added the possibility to choose the document media box when calling newRgbPageMgr(). The default is still PDPage.PAGE_SIZE_LETTER. Having the top and bottom margins depending on the newly customizable format, I changed that also, while adding the possibility to set them from the LogicalPage object. Keep in mind that the default values (i.e. a 37 top margin and a 0 bottom margin for US Letter format) are the same as before, so everything is backward compatible. Anyway, thank you for this very convenient wrapper, feel free to comment and correct anything you deem necessary.

[GlenKPeterson]

Thank you! Great idea. I'd love to have this featue! Thank you for making a minimal diff and generally matching the coding style. I don't see any obvious bugs, but I only skimmed your changes (didn't try them out). I hope to be able to review it more carefully over the weekend before merging it, but here are my initial thoughts:

I'd like to see a unit test for yPageTop(), yPageBottom(), yPageWidth() to show that the old defaults still work. It might be easiest for me to write that, then merge your changes and see that the tests still pass? Not a big deal either way.

I'd like to see com.planbase.pdf.layoutmanager.XyDim instead of PDRectangle because XyDim is immutable. I'm also going to think about how difficult it would be to make mediaBox final. Maybe adding additional constructors or factory methods to let you pass a mediaBox at that time instead of mutating things later. Or a builder if there are already a lot of those (I didn't look).

Have you checked that yPageTop() works correctly in both portrait and landscape?

I see you used get...() and set...() where most of the methods here are a "getter" if they take zero args and a "setter" if they take arguments, but they don't follow the bean convention of get/set in the naming. Also, there may not be a "set" method. :-)

I will probably make most of these changes if you don't, but I plan to add the feature and having you code it up makes that easier/quicker if nothing else.

[Kevindum]

Great ! I'm glad you like it. I'll review some of the code before the week-end given your inputs, only leaving you the unit tests so you can be 100% sure that I didn't broke anything :). Considering using XyDim instead of PDRectangle, I don't really think it would be meaningful since a PDRectangle is required when creating the PDPage in PdfLayoutMgr.logicalPageEnd(...), so it's not just a matter of xy dimensions. Unless I misunderstood you. EDIT : I can still use it in the LogicalPage though, replacing the PdfLayoutMgr.getMediaBox() by some pageDim() method which would extract the data from the PDRectangle. As for the yPageTop() working, it returned 755 for a Letter format height of 792, so putting the default margin to 37 while returning the mediaBox height minus the margin is iso. However, I wondered why the default bottom margin was 0 in portrait and 230 in landscape ? Consequently I didn't touch the landscape one and only parametrized the portrait one. The rest is OK, I'm on it. I may also add a way to customize the leading (currently half the descent of the font wich is quite compact), but all in all it should be again minor tweaks. Anyway, thank you for your answer, I should update the branch today with the changes you raised.

[Kevindum]

Done :)

[GlenKPeterson]

OK, you're right about PDRectangle. PDPage.PAGE_SIZE_LETTER is a PDRectangle. That's the default value of mediaBox, that's what we have to pass to pdPage.setMediaBox(). There's no sense storing something different. Also, the end-user might like to pass things like PDPage.PAGE_SIZE_A4. Sold.

I've added the unit tests to prove that nothing is broken when I merge your changes. You might want to run them first. I also tried checking the javadoc into git, so everyone can see that on github.io now without downloading or building anything: https://glenkpeterson.github.io/PdfLayoutManager/apidocs/

Instead of public XyDim pageDim(), how about public float pageWidth() and public float pageHeight()? That way we don't create an extra XyDim object or share mutable state.

Sigh... I've often thought I should have made the top of the page y=0. It seems weird to have the bottom of the page be 0 in portrait, but 230 in landscape. There may be a reason I didn't change it, but I can't remember that reason now. Hopefully something to do with the underlying assumptions of PDFBox or the PDF spec... It's a big and breaking change if we go there. Not today!

yPageTop() and yPageBottom() currently just give you the raw extents of the page. I think yPageTop() returns 755 whether you're in portrait or landscape (weird - I know - see previous paragraph) and the bottom of the page is adjusted relative to that. I'm willing to consider making the top of every page 0 and going down, but I don't want to combine that change with this one because that's going to be huge.

Returning bottomMargin instead of 0 for portrait looks wrong to me. Either leave it alone or return (portrait ? 0 : 230) + bottomMargin;.

If you take topMargin and bottomMargin out and the tests pass, I can probably merge your changes as is (probably on the weekend). If you want to leave them in, you need to justify their existence.

Why are topMargin and bottomMargin part of LogicalPage now? What's the benefit of having them there? Assuming they are needed, why would you ever need to re-set them after the LogicalPage is constructed? If we add these variables, I don't think I want setters for them. Why would you need getters? I need to understand how adding this to LogicalPage helps the end-user.

[Kevindum]

You're absolutely right about the margins, I wasn't really happy either with my changes, and it was a bit confusing. Maybe it's something to add later, possibly after fixing yPageTop and yPageBottom, but as you said, not today ! So I rollbacked those to stick with the original content. I've also replaced pageDim() as requested, and passed the unit tests, so everything should be OK now.

[GlenKPeterson]

Merged (with some changes). Released! Thank you! You are the first new contributor to this project. Yay!

If you want to make other changes, please look at how I edited yours to streamline the process in the future. I think together we found a bug (that yPageTop has an assumed margin). Also, I don't feel strongly that the upper-left corner should be 0, 0, but I do feel that way. I'm open to discussion on that topic.

Thanks again - this is a great day for PdfLayoutManager!

[Kevindum]

Thanks to you !

[GlenKPeterson]

As we both suspected, LogicalPage.yPageTop() was wrong because it did not account for varying page sizes or Portrait vs. Landscape. I think I've fixed it, but yPageBottom() seems to have the same limitations.

These issues were there at least a year before you came along, so I'm not blaming, just alerting. I'm currently working on a fix, but it's probably going to break some client code if that code relied on the previous wrong numbers. It's also kinda confusing. I didn't really think this aspect through very well.

Since you made a change so nearby and we'd talked about it, I thought you were likely to be affected and might want to know.

P.S. Love your XKCD icon!