Multi-Column Layout Pagination is Broken

[matwood]

If you look here https://github.com/readium/readium-shared-js/blame/develop/js/views/reflowable_view.js#L694 and then here https://github.com/readium/readium-shared-js/blame/develop/js/views/reflowable_view.js#L504 there are a number of issues that I want to fix.

• First the math does not work: (a-b)/c = j is not reversed with (j+b)*c It needs to be (j*c)+b The error shows up on books that use the columnGap.
• Second the rounding causes 1 px errors that propagate as the pages are turned. In multi-column layouts with tight margins the errors show up quickly by clipping text on the left.
  I’m proposing to add a new variable to the paginationInfo which will store the pageOffsetSize instead of rebuilding it from the column size every time.

Thoughts? I open to fixing in a different manner.

[danielweck]

Let's see:

(1)

_paginationInfo.pageOffset = (_paginationInfo.columnWidth + _paginationInfo.columnGap) * _paginationInfo.visibleColumnCount * _paginationInfo.currentSpreadIndex;

https://github.com/readium/readium-shared-js/blob/develop/js/views/reflowable_view.js#L504

and

(2)

_paginationInfo.columnWidth = Math.round(((_htmlBodyIsVerticalWritingMode ? _lastViewPortSize.height : _lastViewPortSize.width) - _paginationInfo.columnGap * (_paginationInfo.visibleColumnCount - 1)) / _paginationInfo.visibleColumnCount);

https://github.com/readium/readium-shared-js/blob/develop/js/views/reflowable_view.js#L694

[danielweck]

I do not understand "the math does not work". Are you referring to (1) or (2) (see my message above)

[danielweck]

PS: the columnGap is 60px by default (visible blank space between each column of a reflowable two-page spread).
There is an optimal page width (configurable in the cloud reader and Chrome app via a slider control), so the visible "margins" on the left/right of the viewport are arithmetically determined by the window / webview size.

) the columnGap value is fixed.

[matwood]

You're right. I misread a parentheses. Staring at the code too long I guess.

So yes, the only problem is the rounding. The fix I want to implement will take rounding out of the equation and simplify the paging code with less recombining of the parts.

[danielweck]

Did Juan's latest odd/even width fix help in any way? Is that a separate problem?
By the way, thanks for spending time thinking about this, much appreciated.

[matwood]

No problem! Hopefully you didn't spend too much time verifying the calculations were correct.

I'm looking at the development branch a few days old. Is the odd/even fix in there?

From a higher level stand point, it might make sense to simplify the code by decoupling the page turn size from the column size within the page. While minor, it is also doing a lot of calculations for each page turn that could be done once on a view port resize. In the proposed solution, turning the page would be multiplying the pageOffsetSize to the currentSpreadIndex or page.

[danielweck]

Juan's fix:
37a5141

[matwood]

I do not think that fix works when the columnGap is an odd number. Is the columnGap normalized somewhere else?

[danielweck]

Good point @matwood
Added comment: 37a5141#commitcomment-17056184

[danielweck]

@jccr 's useful reply about odd/even columnGap: 37a5141#commitcomment-17057065

[matwood]

I don't know the code base as well as you guys, but I think @jccr 's response supports my argument for decoupling the columns inside the page from determining the 'page' size for the left/right or up/down shifts. Knowing that at a minimum the page flip is correct makes it easier/less risky manipulate the layout inside the page.

We can discuss further on tomorrows call.

[matwood]

I created a PR here: #288

In my testing it worked, but please double check my assumptions with the math. The simplification here was to calculate the full page size whenever update pagination is called and use that calculation to turn the pages. Constantly rebuilding the page size from the columns on the page opened up issues with rounding and unnecessary calculations.

[danielweck]

Similar (identical) issue? #181