Create constant padding page layout option #5270

tmoerschell · 2023-10-21T23:46:33Z

Fixes #5268.
I'm not too sure what branch to base it on. It's currently on master, but it's possible to rebase it on release-1.2. I took it on as a sub-issue of #5269, as it required knowing all about the padding around the pages. It would be nice to be able to merge this one and then use it as a base for the zoom center fix.

bhennion · 2023-10-22T09:11:41Z

I would not do this in the release branch. This is not fixing a bug: some previous dev went out of their way to get the behaviour we have. A simple grid, which is the result we get with this PR, is much simpler to implement than the current Layout/LayoutMapper implementation.

Overall, we should have a debate about what the best behaviour should be. This includes questions:

what use cases lead to pages of different sizes?
is being vertically/horizontally centered really the most natural thing in those situations?
how should pages 200 to 210 be displayed if page 1 is a bit wider? Should the padding still be there even though we are very far from the problematic page?

Those questions should be answered before anything else, and of course 1. will influence the answers to 2. and 3..

As for possible usecases, I guess rotating pages is one. We should also keep in mind something like #3430 (should it ever be implemented).

tmoerschell · 2023-10-22T20:42:34Z

Oh I'm sorry, I didn't know this was actually the wanted behavior. The current implementation looks kinda odd: it does include some padding to compensate for different page widths, but not enough to align the pages. I have now also checked the PR that introduced this code (#872), and it doesn't seem to mention a reason for this.

About the branch issue, I agree that it isn't a good idea to merge this into the release branch. But the PR I want to build upon afterwards was, and the changes aren't in the master branch. So I wonder: do those get merged into the master branch as well after some time? Why not right away?

The use case I have is inserting portrait A4 pages between 3:2 slides to take additional notes.
Adding some extra space to PDF pages or rotating them is certainly also something that could happen in the future.

I personally don't actually use the multi-column layout. I just came across it trying to fix another issue. I'm always writing something on a single page, so I rarely have multiple pages in my view anyways. I'd think what is happening between the pages is of little importance to most users. For me, it should just be as pretty as possible.

Anyways, I think there's mainly two options you could go about the layout:

Keep a constant padding between pages, disregarding alignment issues. A horizontal layout may have rows of different lengths and a vertical layout may have columns of different heights.
Put the pages into a simple grid, disregarding padding consistency within rows and columns.

One issue with the grid would certainly be that if one has a page that is a lot bigger than the others, it will cause big gaps between other pages. But who would ever do that?

I don't know what an infinite canvas would look like, or if there already is an idea about how to implement it. A simple version of that could be just pages with an arbitrarily large, automatically extending size. This could cause wildly different page sizes. I guess a solution to that could then be to crop/scale over-sized pages when zoomed out.

bhennion · 2023-10-23T14:13:30Z

Oh I'm sorry, I didn't know this was actually the wanted behavior. The current implementation looks kinda odd

I agree it is kinda odd. To be clear, I don't think your changes are not a good idea, but I think we should investigate how it'll influence various use cases (if we can find someone actually using it).

So I wonder: do those get merged into the master branch as well after some time? Why not right away?

We can certainly merge the release branch into the master branch whenever we want. No problem there. You can create a PR for that, containing exactly 1 merge commit, so people can check it out and make sure nothing wrong happened in the process. Then we'll fast forward it onto the master branch.

I personally don't actually use the multi-column layout. I just came across it trying to fix another issue. I'm always writing something on a single page, so I rarely have multiple pages in my view anyways. I'd think what is happening between the pages is of little importance to most users. For me, it should just be as pretty as possible.

rolandlo · 2023-10-27T20:09:14Z

I agree it is kinda odd. To be clear, I don't think your changes are not a good idea, but I think we should investigate how it'll influence various use cases (if we can find someone actually using it).

I do use a 1-row or 2-row layout on my external monitor for displaying my solutions when correcting an exam. Usually the pages have all the same format though. Different page formats can appear when you have a document, where some pages are landscape and others are portrait. Another case may appear when the PDF was generated from (camera or scanned) images. There are also some books and journals where some of the first pages have a different format.

The solution proposed in this PR looks logical from the point of view that all layout settings are symmetrical with respect to vertical/horizontal. But the current layout method has its advantages, too.

tmoerschell · 2023-12-04T00:12:11Z

Ok, finally made some progress with this: I implemented constant padding layout.
It's the first time I touched something on the GUI / action database side. I hope I didn't miss too many things.

There are some parts of the code that I'm not too fond of (especially the repeated loops in Layout.cpp). I'm sure it must be possible to this in a better way, but couldn't find it.

Do also note that this is not fully functional yet: the visual part is good, but it is in some cases not possible to draw on the pages, for example, because most notably I haven't adapted Layout::getPageViewAt(x, y) yet.

tmoerschell · 2023-12-05T13:14:58Z

Ok, everything is good now, except for getPaddingAbovePage() and getPaddingLeftOfPage(), which I plan to change in #5295 anyways, so I'd leave it for then.

Can't guarantee that I caught everything, but I will put it up for review again.

tmoerschell · 2023-12-05T17:32:03Z

Also a quick note that there is currently (on master and release branches) a real issue with documents containing multiple page sizes and layout in multiple columns: it is sometimes not possible to draw on the whole page area as shown in this screenshot.

This can be fixed by just the first commit (which makes me think the current behavior wasn't really intentional). Would it be worth it to put that one also on the release branch?

tmoerschell

Here are a the main issues I have with my code. I would appreciate feedback on this. It might also explain some of the oddities I wrote.

src/core/control/settings/Settings.cpp

tmoerschell · 2023-12-05T17:41:08Z

src/core/control/settings/SettingsEnums.h

+enum LayoutType {
+    LAYOUT_TYPE_GRID = 0,
+    LAYOUT_TYPE_CONST_PADDING = 1,
+};
+


I hesitated between creating a custom enumeration and defining the setting as a simple bool layoutConstPadding or something. I'm also not really sure the enum declaration is in the right place here

src/core/gui/Layout.h

tmoerschell · 2023-12-05T17:48:26Z

src/core/gui/Layout.cpp

+    struct PageLayoutDescription {
+        double paddingLeft = 0.0;
+        double paddingTop = 0.0;
+        double width = 0.0;
+        double height = 0.0;
+        std::optional<size_t> optionalPage = std::nullopt;
+    };


I think it's a bit ugly to have that struct definition in the function implementation. Is there a better way?

Also we don't actually need width and height at the same time. We could replace it with a single variable size which would be one or the other, depending on the layout orientation. But that would make the understanding and the control flow a little more complicated, so I figured it's ok like this?

tmoerschell · 2023-12-05T17:50:47Z

src/core/gui/Layout.cpp


+    std::vector<PageLayoutDescription> pages;  // access with r * rows + c


It would be a lot easier to use a C-style 2-dimensional array, but I haven't found a good equivalent in C++. Suggestions?

tmoerschell · 2023-12-05T17:55:39Z

src/core/gui/Layout.cpp

+    // Iterate over ALL possible rows and columns 3 times:
+    // We don't know which page, if any, is to be displayed in each row, column -  ask the mapper object!
+    // Once we know the dimensions of all pages, define the paddings above and left of each page as required.
+    // Finally, assign page coordinates by propagating page paddings and sizes from the border.


This whole section (until line 445) is taking a lot of space. Also I'm not sure how good it is to iterate over every page 3 times. This is the shortest code I could come up with for this, but I'm 80% convinced there must be a better way I'm not aware of.

src/core/gui/Layout.cpp

In particular: * Store all grid cell bounds for const padding * Add getGridPositionAt() and use it in updateVisibility()

tmoerschell · 2024-03-17T22:23:17Z

I've improved some of the things I was unhappy with now, so I'd like to request a review from someone on this :-)

tmoerschell added a commit to tmoerschell/xournalpp that referenced this pull request Oct 27, 2023

Merge PR xournalpp#5270 (fix page layout)

353230d

tmoerschell added a commit to tmoerschell/xournalpp that referenced this pull request Oct 27, 2023

Merge PR xournalpp#5270 (fix page layout)

32499a6

tmoerschell mentioned this pull request Oct 27, 2023

Fix zoom center #5295

Draft

2 tasks

tmoerschell added a commit to tmoerschell/xournalpp that referenced this pull request Oct 29, 2023

Merge PR xournalpp#5270 (fix page layout)

af3b373

tmoerschell marked this pull request as draft November 18, 2023 11:36

tmoerschell added 3 commits November 25, 2023 08:31

Fix horizontal page alignement

8639399

Center pages vertically in layout row

8aee375

Simplify layoutPages loop

03430b0

tmoerschell force-pushed the fix-page-layout branch from 6e54f26 to 03430b0 Compare November 25, 2023 17:06

Implement constant padding option

4038356

Adapt Layout::getPageViewAt()

ec176f1

tmoerschell force-pushed the fix-page-layout branch from 46cbf9e to ec176f1 Compare December 5, 2023 11:44

Adapt Layout::updateVisibility()

04c91bd

tmoerschell marked this pull request as ready for review December 5, 2023 13:14

tmoerschell changed the title ~~Align pages horizontally and center them vertically~~ Create constant padding page layout option Dec 5, 2023

tmoerschell commented Dec 5, 2023

View reviewed changes

Various improvements

c14fc4f

In particular: * Store all grid cell bounds for const padding * Add getGridPositionAt() and use it in updateVisibility()

tmoerschell force-pushed the fix-page-layout branch from dc2dbd9 to c14fc4f Compare March 17, 2024 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create constant padding page layout option #5270

Create constant padding page layout option #5270

tmoerschell commented Oct 21, 2023

bhennion commented Oct 22, 2023

tmoerschell commented Oct 22, 2023

bhennion commented Oct 23, 2023

rolandlo commented Oct 27, 2023

tmoerschell commented Dec 4, 2023

tmoerschell commented Dec 5, 2023

tmoerschell commented Dec 5, 2023

tmoerschell left a comment

tmoerschell Dec 5, 2023

tmoerschell Dec 5, 2023

tmoerschell Dec 5, 2023

tmoerschell Dec 5, 2023

tmoerschell commented Mar 17, 2024


		std::vector<PageLayoutDescription> pages; // access with r * rows + c

Create constant padding page layout option #5270

Are you sure you want to change the base?

Create constant padding page layout option #5270

Conversation

tmoerschell commented Oct 21, 2023

bhennion commented Oct 22, 2023

tmoerschell commented Oct 22, 2023

bhennion commented Oct 23, 2023

rolandlo commented Oct 27, 2023

tmoerschell commented Dec 4, 2023

tmoerschell commented Dec 5, 2023

tmoerschell commented Dec 5, 2023

tmoerschell left a comment

Choose a reason for hiding this comment

tmoerschell Dec 5, 2023

Choose a reason for hiding this comment

tmoerschell Dec 5, 2023

Choose a reason for hiding this comment

tmoerschell Dec 5, 2023

Choose a reason for hiding this comment

tmoerschell Dec 5, 2023

Choose a reason for hiding this comment

tmoerschell commented Mar 17, 2024