sumatrapdfreader / sumatrapdf

SumatraPDF reader
http://www.sumatrapdfreader.org
GNU General Public License v3.0
12.94k stars 1.67k forks source link

Multi-column page layout #4184

Open kjk opened 2 months ago

kjk commented 2 months ago

This is to record work on showing multiple columns. It subsumes #246, #776, #1171

The work is done on multi-column branch. It's ok to rebase multi-column branch against master branch and force push, to keep it in sync with master.

Desired features:

It requires re-thinking of our View options, both in UI and how it's persisted in settings.

How do other programs handle this in their UI?

kjk commented 2 months ago

To set number of columns we could have menu items with explicit numbers or add a modal dialog similar to IDD_DIALOG_GOTO_PAGE where user can enter number of columns. We would clip it to valid range 1 - N.

1 maps to single page, 2 to facing and the rest needs to be remembered in settings.

kjk commented 2 months ago

The combinations of view options and zoom levels is confusing already and will be even more. It would be good to design a modal dialog for setting those options in a way to provides a UI hint on how things will look. I assume there are programs that already do something like that and we can steal their design.

kjk commented 2 months ago

We also need to improve our render cache. At the very least significantly bump the number of cached pages because it was designed with assumption it only needs to keep few pages around. With 4 columns and 16 pages visible at a time, we re-render the pages on scroll.

Cache eviction logic should be changed. We should unconditionally evict pages rendered at no-longer-valid zoom. But eviction of rendered pages at currently valid zoom should be based on memory used by rendered bitmaps vs. amount of free memory in the system.

We should also have a bool evict_emergency() function that we can call if we fail to make a significant allocation. It would delete all non-visible pages (aggressive) and return true if evicted something, in which case we would retry an allocation. We don't want to retry every failed allocation, just big ones (> 1MB?) and only in specific contexts. If we fail to allocate 1 KB then we're likely not going to recover anyway. So we would probably add a retry_malloc or add a global flag similar to gAllowAllocFailure (gRetryAlloc) that we would strategically set around allocations that should be retried.

Also, we should increase number of render threads. Our system was designed long time ago and only uses a single thread that picks items to render from a queue.

Today it makes more sense to have up to N threads (where N is number of cores).

Instead of keeping a rendering thread we could launch them on demand and throttle number of actively working threads via semaphore.

Which brings another point: apparently they way I did multi-threading is not how mupdf wants it. Each mupdf-based document has a single fz_context which contains locks. Turns out that each thread should use fz_context from fz_clone_context().

All this is big job in itself and probably deserves its own bug / branch but without at least increasing cache size multi-column view will be bad experience for users.

kjk commented 2 months ago

Another thing: it seems in what is now non-continuous view it also makes sense to provide multiple rows, if the space is there.

We also need to decide how to use remaining space: have it as left/right margin, distribute between pages. In HTML there's display: flex and it has different justify-content settings. We need to decide which one of those we want to implement (maybe multiple?)

changlichun commented 2 months ago

I was mostly inspired by office word, which allows the number of columns to change while zooming. They have such a setting dialog(I dont have a English version). image When set to multi page, the column number will auto change while zooming.

They do not support non-continuousely mode.

Maybe we can have a option like show pages continuously to triggle fix or float columns. And a dialog like custom zoom to set the column number.

changlichun commented 2 months ago

One known issue now. When columns>2, setting to fit page or fit width will decrease the column number unexceptly.

kjk commented 2 months ago

Yes, something like that word dialog but adapted.

I would say we need liquid (exact name to be determined) in addition to fit width, fit page etc. It shows as many columns as will fit based on zoom level.

In non-continuous mode it also fits as many rows as will fit in a window.

Then instead of multi-page I would do columns text box where user can enter a fixed number of columns. We either auto-size the column in fit window mode or have scrolling if fixed zoom level.

I don't want to do grid. That's just a bit too much and makes no sense in continuous mode. Ideally in non-continuous mode we fit as many rows as fits in window or we just stick with current single row.

Layout code is tricky and with those additions will get even more tricky. A good time to add tests to verify all permutations. The algo is well defined: it gets width of viewport, layout mode (zoom, fit, continuous or not), number and size of pages at 100%. It calculates the size of the viewport and positions of all pages in it.

Currently the code is tied to all other info about pages but could be transformed to only use the layout info / bounding boxes of rectangles (pages) and spit out their position. We could then write unit tests for specific configurations and for ad-hoc testing we could save position of rectangles to text file and then post-process it in Go to generate HTML that visualizes the layout of the page within viewport.

There's also an outstanding layout issue that we should address when touching this code.

When I wrote layout I assumed that all pages have the same size and therefore for layouts where we auto-fit pages to fit within viewport (window), we need to calculate a single zoom value for all pages.

This breaks for non-uniform pages so a single big page in fit width mode makes smaller pages render too small. We want all pages have the same width in a column which means we need to calculate / remember custom zoom for each page.

And finally, we have single layout logic for all possible variations, which might be more complex than necessary. Maybe it would be simpler to split the code into trivial layouts (fixed zoom and number of columns) and complex layouts (where we need to calculate zoom for pages to fit them into viewport).