Add support for variable page sizes. (Resolves #59) #61
Conversation
angelosilvestre
changed the title
| PageListViewportGestures( | ||
| controller: _controller, | ||
| lockPanAxis: true, | ||
| child: PageListViewport.variedPageSized( |
There was a problem hiding this comment.
@dodgog I'm reviewing this PR with @angelosilvestre - what do you think about this widget API? Do you see anything missing in terms of what you need for varied page sizes?
There was a problem hiding this comment.
This looks fine, yes. I can see the alternative being: passing a list of page sizes and also a function onGetNaturalSizeForPage() [doesn't have arguments]. Or is there a design constraint requiring onGetNaturalPageSize to be a different function for each page?
There was a problem hiding this comment.
A list of page sizes sounds good to me, but I didn't understand why we would have an onGetNaturalSizeForPage() without arguments if the list of page sizes is provided.
Could you clarify that?
There was a problem hiding this comment.
The api looks good. It's either a list of sizes, or a PageSizeResolver with a cache.
It may not always be cheap to obtain a list of sizes in advance, that's why onGetSize can be helpful to dynamically build the viewport.
Just to confirm my understanding: does the total length of the viewport need to be known in advance in order to render the widget?
There was a problem hiding this comment.
The api looks good. It's either a list of sizes, or a PageSizeResolver with a cache.
It may not always be cheap to obtain a list of sizes in advance, that's why onGetSize can be helpful to dynamically build the viewport.
Currently, only a PageSizeResolver is allowed. Do you want another option to give a list of sizes instead?
Just to confirm my understanding: does the total length of the viewport need to be known in advance in order to render the widget?
The pageCount is required in order to render the widget.
There was a problem hiding this comment.
I think there's no need for a list of sizes. The consumer of the api could probably implement a convenience resolver which would have cached values, for example a getSize(i) function which would map to sizesList[i] as you do in the example app roughly. This is good for now. Can proceed with final reviews and merging probably.
Let's not act on this now, but I'm wondering if it will be easy/worth it to make the calculation and per-page layout lazy? Would have to involve some complex scrolling approximations, but how hard is it?
angelosilvestre
changed the title
|
@angelosilvestre can you coordinate with @dodgog until you two have explicit agreement about the public API in this PR? I probably should spend time reviewing the implementation until we have a good API. |
Add support for variable page sizes. (Resolves #59)
Currently, the
PageListViewportwidget assumes that all pages have the same natural size.This PR adds an option to have pages with different sizes. To do that, the existing
PageListViewportwas split into two widgets, one for fixed-size pages and other for variable-sized pages. I copied the original widget and made the adjustments to work with variable page sizes. To make the gestures work with both widgets, I had to create an abstract classPageListViewportLayout, that exposes the methods needed for the controller and gesture widgets to query the necessary information.This PR also adds widget tests and golden tests.
variable_page_layout.mp4