-
Notifications
You must be signed in to change notification settings - Fork 771
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
Refactor OpenFile logic - make it gtk4-compatible #5602
Conversation
758af5a
to
79becb9
Compare
I decided to include in this PR the changes to Control::save() and Control::saveAs() as well. My reasoning for this is: better to test as thoroughly as possible to end-state rather than test half-way through. The changes here could have critical consequences I have not forseen, because non-blocking dialogs means non-blocking Control::save(), Control::close(), Control::openFile(). I tried to be as careful as I can, but thorough testing is required. |
33ff44a
to
22e9050
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
7562f84
to
e645e9d
Compare
resolved conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some extensive testing (mostly on Linux, but also on Windows and MacOS) and haven't found any regressions. Good job! Two things I noticed (which are also present on master):
emergencysave.xopp
is added to the list of recently opened documents, which doesn't make much sense.- When a file in the "new" format with PDF background like
xournalpp/test/files/packaged_xopp/pdfBackground/new.xopp
is emergency-saved, the PDF background cannot be restored via the emergencysave.xopp file.
From the code side I have only some minor remarks. Maybe @Febbe can take a look at move_only_function.h
, since that takes a lot of C++ knowledge.
* Question: in case the current file has not been saved yet, do we want: | ||
* 1. First ask to save it, save it or discard it and then show the FileChooserDialog to open a new file | ||
* 2. First show the FileChooserDialog, and if a valid file has been selected and successfully opened, ask to | ||
* save or discard the current file | ||
* For now, this implements option 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with option 1 as well. Priority has to be to avoid loss of data. Please update the PR description (you wrote there that you kept the behaviour from master, where the filechooser shows up first).
@@ -0,0 +1,57 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really can't give you any feedback on this file (since I don't know C++ well enough). Maybe @Febbe can take a look at it.
Thanks a lot for the review and testing! I applied your requested changes and rebased on latest master.
Mmm... That would need to be fixed in control/RecentManager.cpp:RecentManager::getRecentFiles()
Now that you mention it, the attach mode does not work, even in v1.2.3. The background is not attached in the .xopp file, but written alongside it with .bg.pdf extension. |
* Xournal util functions | ||
* | ||
* @author Xournal++ Team | ||
* https://github.com/xournalpp/xournalpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2 cents:
Is this implementation a ported version of the C++23 move_only_function ?
If that is the case, are any differences between this and C++23 impl?
I would like to see a documentation of what it does and how to use it plus a link to C++23 move_only_function (if applies).
I think it will increase the chances that a future developer use this kind of helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. I added comments saying exactly that, and precising what differences there were.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Those functions are now non-blocking and (possibly) call dialogs that are compatible with gtk4.
Unless @Febbe objects I suggest to move forward with this PR and merge it. |
I currently have no time to do a full review. I second that. We can still push a fix to correct unwanted behavior. |
Alright! Merging in 24 hours then! |
|
||
namespace xoj::util { | ||
template <class T> | ||
class move_only_function final {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be only a class template prototype (remove {}
). Since an invalid specialisation should result in a compile error directly.
This PR builds on #5585 and refactors Control::openFile and similar functions so that the dialogs they call become non-blocking and compatible with gtk4.
The logic is pretty complicated here. I tried to make it as easy to read and maintain as possible.
There are a couple of notable things to look at when reviewing:
Annotate PDF
while having an unsaved document, or opening a file with missing PDF background,, or a combination of the two, and so on). I tried to test them all, but there are many and I could have missed one.Open File
, which dialog should pop up first: the FileChooser or the Save or discard dialog? I changed this behaviour compared to master (master shows the FileChooser first, this implements the other way around). Any opinion is welcome.I also fixed a couple of inocuous bugs (e.g. if a file is not saved and the user tries to open a PDF file, the "Save or Discard" dialog appears twice on master).
For reviewers, only look the last commit, the other ones are from #5585 and will be rebased away once #5585 is merged.Edit: #5585 is merged and this has been rebasedThere are still old gtk3-still dialogs in the code base.
The next step is to refactor Control::save() and Control::saveAs(), in a follow up PR.Edit: this is now part of this PR