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

Multilayout - more columns, more arrangement of pages. #872

Merged
merged 17 commits into from
Feb 15, 2019

Conversation

JJones780
Copy link
Collaborator

Kudos to the xournal++ team for an excellent program!
I've added a few capabilities for my own use ( but got carried away ) and cleaned it up for inclusion if it meets your standards.
I'm able to make changes for the next short while.. so please let me know.

Multilayout:
Added capability to have many columns (pairing works as expected - will force even # columns).
Added capability to order pages horizontally or vertically and order them: left-to-right/right-to-left, top-to-bottom/bottom-to-top combinations .

More potential arrangements easy to code in ( like 2 columns of 4 across, or randomized order !?! etc). Please let me know.

Also coded for fixed rows instead but not in menu yet.

Added setting for page offset: 0 or 1 makes sense but larger numbers work depending on columns etc.

New Preferences/View setting:
Pairs Offset: First Page Offset
New View menus:
Columns: 1,2,3,4,5,6,7,8
Layout:
Horizontal/Vertical
Left to Right/Right to Left
Top to Bottom/Bottom to Top

TODO: A few more gui additions.
Note: simple linear search works well enough that I haven't gotten around to a proper quadtree or other to replace the binary search ( which was modified to do paired pages ).

Presentation mode not working.
Check dpi settings. My 4k + 1080p zoomed out display config might be confusing it?
…ffering page sizes.

Column Menu now shows current setting on startup.
numColumns setting now saved and then loaded on startup

/
Initial mappings: Horizontal, Vertical, VerticalPaired.
Issues: Unable to annotate all pages... other code ( PagePosition Handler etc?) must be assuming layout.
PagePosition system had become overly complex and didn't work well with LayoutMapper ( no longer naturally sorted ).
Considered quadtree but just went with a linear search for now. No noticeable impact on performance.
…ayout now bitflag flexible.

TODO: allow user to access them via menu or prefs.
Copy link
Member

@LittleHuba LittleHuba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature and very clean code already. I just highlighted some minor things. Nothing to really worry about.
I did not test it yet but it does look good so far.

src/gui/Layout.cpp Outdated Show resolved Hide resolved
src/gui/Layout.cpp Outdated Show resolved Hide resolved
src/gui/Layout.cpp Outdated Show resolved Hide resolved
src/control/settings/Settings.cpp Outdated Show resolved Hide resolved
src/control/settings/Settings.h Show resolved Hide resolved
src/gui/LayoutMapper.h Outdated Show resolved Hide resolved
src/gui/pageposition/PagePositionCache.h Outdated Show resolved Hide resolved
src/gui/pageposition/PagePositionHandler.cpp Show resolved Hide resolved
src/util/XournalTypeList.h Show resolved Hide resolved
ui/main.glade Show resolved Hide resolved
Thanks to 'LittleHuba' for review notes
Note: Prefer weak enums but then have issues with name collisions.
@JJones780
Copy link
Collaborator Author

Thanks LittleHuba for reviewing my changes. Please let me know if anything else needs attention.

Note that the Presentation mode seems to be broken (before my changes). Zooming in presentation mode is broken; as well as other behaviour?

@LittleHuba
Copy link
Member

You're welcome! Thanks for the PR! It is always appreciated if new features are added to make Xournal++ even better than it currently is.

The Presentation mode is heavily broken. There are a few tickets open concerning the zoom issues and other things. You didn't cause it, so you don't have to fix it. (But you are most welcome to do that in a different PR 😉 )

I still need to test everything, as I'm currently somewhat busy. The code looks nice and clean now, except for the issue with the LayoutMapper (see above).

When that is resolved and nothing comes up in the tests I'll merge it.
Thanks once again!

…g in multiple arguments.

Columns enum changed to Vertically.
thanks to LittleHuba for review and recommendations.
…dRows.

TODO: menu to set viewFixedRows and show/hide numRows/numColumns accordingly
@andreasb242
Copy link
Contributor

It looks good except the formatting. But if it's working then I'll fix it.

@andreasb242
Copy link
Contributor

I checked the pull, generally it looks good. But on dual page the first page needs to be at the right side, not on the left side, because if you print it e.g. as book, you get the first page at the right side, and the second page is on the back, therefore on the left side if you open the Book / whatever.
And with changing the name, of the button, you break the compatibility, the button is simply gone on my custom toolbar, thats not nice.

@JJones780
Copy link
Collaborator Author

Thanks for taking a look Andreas,
The dual page offset is now configurable in the preferences ( under View ). It should default to 1 and appear as you suggest. I'll re-check.
I have noticed another minor formatting bug that crept in - which I plan to fix asap as well.
Custom toolbar versioning: I had a hint of this and meant to look into it already, sorry. I'll push a solution to this asap and see what you guys think.

If you want to let me have a crack at this for the next little while I'd be happy to work at it and avoid issues with merging.

Back in 3 hours.

@andreasb242 andreasb242 merged commit 5f2ddbb into xournalpp:master Feb 15, 2019
@andreasb242
Copy link
Contributor

Thank you, and feel free to add a new pull request for new features!

By the way, I'll fix the default value to 1:

this->numPairsOffset = 1;
;-)

*
* A layout manager - map where( row,column) to which page( document index)
*
* @author Justin Jones
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead writing your name here, add you to the author list in the about dialog, this is probably never read by any user ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'll earn my way on to the about dialog in the future with a few more commits ;)

Thanks for checking and fixing the numPairsOffset.

@LittleHuba
Copy link
Member

Thanks for taking over @andreasb242. And thanks @JJones780 for this great PR! I hope you continue bringing such great features to Xournal++!

@JJones780
Copy link
Collaborator Author

JJones780 commented Feb 15, 2019

I'm pleased to have been able to contribute to Xournal++

I noticed a few little follow up issues - I'll make a new branch and push them your way soon. I'm offiline much of the next week though.
For my reference:

  • Tooltip markup in prefs broken
  • Layout::updateCurrentPage()
  • Re-interpreting " Add Vertical Space" and "Add Horizontal Space" to multicolumns. <--comments?

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

Successfully merging this pull request may close these issues.

None yet

3 participants