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

Dry run doesn't always return correct height #789

Closed
mojavelinux opened this issue Mar 27, 2017 · 4 comments
Closed

Dry run doesn't always return correct height #789

mojavelinux opened this issue Mar 27, 2017 · 4 comments
Assignees
Milestone

Comments

@mojavelinux
Copy link
Member

mojavelinux commented Mar 27, 2017

The dry run logic doesn't always return the correct height if the content exceeds the height of a page. The problem has to do with page boundaries.

When we want to pre-calculate the height of a document fragment, we move to a scratch document, clone the page settings, and write into the document. We then measure the distance from where the cursor started to where the cursor ended, taking into account page traversals.

However, when we do this, we always start the cursor at the top of the page in the scratch document, regardless of where the cursor is in the main document. When we go back to write the content in the main document, we often end up with a different result because of where the content falls in relation to the page boundaries.

The fix seems simple. Just start at the same position in the scratch document that we do in the main document. The problem is, we sometimes want to know if the content will fit on a single page. In that case, we need to start at the top of the document so we get a clean measurement. This is necessary for the "keep together" logic.

If we're doing "keep together" logic, then we first need to start at the top of the document and write until we hit the end of the page. We need to intercept this event and stop writing. We know we can't fit in a single page. We then need to back up, move the cursor to the location it is in the main document and start again, this time proceeding until we finish writing. We return the height and a flag that says we can't fit on a single page.

If we aren't doing "keep together" logic, then there is no reason to do the first step. We can just move the cursor to the same location it is in the main document, write all the content, then measure.

If we rewrite the logic as described, we'll always get accurate measurements. We'll also have the ability to use dry run for either "keep together" logic or to simply pre-calculate the height of content. This will enable features like #578.

In short, here's the logic for keep together:

  • switch to draft document
  • move cursor to top of new page
  • configure document to stop when advancing to next page (perhaps override on_page_create and throw exception)
  • if content fits on one page
    • return height and a flag that content fits on one page
  • if content exceeds one page
    • remove the hook to stop when advancing to the next page
    • move cursor to y offset from main document
    • start run again, but proceed until all content is written
    • calculate height across pages
    • return height and flag that content does not fit on one page

Here's the logic when not using keep together:

  • switch to draft document
  • move cursor to y offset from main document
  • write all content
  • calculate height across pages
  • return height (perhaps include flag that indicates whether content ran over page, but remember we didn't start from the top)

NOTE: We might be able to avoid a dry run in obvious cases; a kind of engineering estimate.

@mojavelinux
Copy link
Member Author

Here's another point to consider. When Asciidoctor PDF was first released, it couldn't draw a background behind a block that split across pages. So we applied "keep together" logic to most panel-like blocks (listing, sidebar, example, etc). That's no longer necessary. We could return to splitting blocks across pages and only using "keep together" when requested.

@ghost
Copy link

ghost commented May 12, 2017

Hey, I promised to look at this a while ago, but then life happened and I got pulled off to a bunch of other tasks (I'm sure you can relate :)). So I've read through what you've written above, and it all makes sense to me - certainly it matches up to what I was thinking previously. Also very much +1 to your last comment.

There is a slight wrinkle in the ointment which could cause an issue, possibly, though it could be deliberately not handled in the first iteration of this.

Let's say we fix #578, and make "keep_together" configurable for all blocks. Then someone does a "keep together" open block, and within that, another keep_together block (doesn't matter what).

The behaviour I'd like to see in that case is that first the whole lot tries to fit onto a single page, and if it can't, you then try to see if the sub-element can fit onto a single page (and so on and so forth - hopefully that makes sense?).

That maybe is something we can punt on for a while, but whenever someone tries to fix this, it'd be nice to at least have the notion of doing this in mind. It might just fall out of how it's coded.

@mojavelinux
Copy link
Member Author

I'm in the process of working on this. Here's the solution (assuming keep together logic):

  1. In dry run document, start at current y and convert the block; capture the start and end location of the block into a range object
  2. If the range is split across pages, retry from the top of page to recompute range
  3. If the range still gets split, go back and use the original range (no point in starting on new page when it will get split anyway)
  4. In main document, advance to new page if block can fit on a single page without being split
  5. Draw boundaries based on range (instead of block height)

@graphitefriction
Copy link
Member

This issue has been replaced by #2003 which summarizes the new logic being implemented to compute the extent of a block's boundaries.

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

No branches or pull requests

2 participants