Change File Dialogs to use GtkFileChooserNative for XDG Portal support #6049
Conversation
|
Sorry I wasn't quite clear. The point is not to necessarily use the existing PopupWindowWrapper (so you can create another wrapper for NativeDialogs if needed), but just to get inspiration from it. The main objective is to not use gtk_native_dialog_run() so that the same code will run (more or less) on GTK4. Look at the example of all the xoj::OpenDlg::showOpenBlaBla() Unpacking what it does, it moves the callback function (and its context: it is typically a lambda so it may carry data attached) to a FileDlg instance, then shows a dialog and returns. This is a bit overly complicated for GtkDialogs (but necessary for general GtkWindows). In any case, it looks like that for GtkNativeDialog, one layer can be dropped (because there is no equivalent to gtk_window_close()). You can probably have a single NativeWrapper playing the roles of both FileDlg and PopupWindowWrapper: it owns the callback (as a member variable of type std::function<void(...)>) and will delete every piece of data once we are done. |
|
@bhennion Ok, now I got it. I created an own wrapper which manages the destruction of the dialog (currently only for saving). The goal is to create one wrapper for saving and opening files, where you specify which one to use at creation of the wrapper. It's a bit awkward the way I deconstruct the wrapper, since it needs to be a pointer. Later, the response event deletes it, which is needed since I have no other event indicating the dialog has been closed. Also doesn't seem to work correctly on my system (which isn't that huge of a deal). Everything else seems to work on my system. |
There was a problem hiding this comment.
Here are a couple of comments/questions.
We should also be careful that the GTK documentation lists a handful of features that are not working on various platforms.
Are we relying on those features? If so, this needs to be remedied (I think we are using MIME type for filters for instance - we need to avoid them on Windows)
|
As far as I can tell we might rely on GtkFileFilters with custom filters (will double check this). |
|
Ok did try using it on Windows, which works. However, it does not use the native Windows dialog, which seems related to GtkFileFilters, should be quite easy to fix (will look into that). |
|
@bhennion I would replace the open file dialog now too. Would you prefer two separate wrapper (one for open and one for save) or a combined one (with just a wrapper)? I personally think a combined wrapper would be better. Edit: I implemented it now using a wrapper and I have done some cleanups. I now look into native dialogs for Windows. |
|
Native Windows implementation mostly works now. I couldn't find the reason for PageTemplateDialog and I couldn't find the code to determine the date (since the error must be here). |
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
There was a problem hiding this comment.
I tested it. It doesn't quite work: it is really easy to override a file without being warned about it (on KDE, both with and without GTK_USE_PORTAL).
The point is, if you type as a name foobar when you save, the saved file will be foobar.xopp.
Without GTK_USE_PORTAL, you will not be warned at all even if foobar.xopp exists (but foobar does not).
With GTK_USE_PORTAL=1, you will only be warned about it if the box "Automatically choose file name and extension" is ticked (not quite sure what the original english string is).
|
@bhennion I moved the ExportTypes to a separate file. I also changed the logic for replacing files, but now you get double replace dialogs on KDE (which I guess is better than none) if files are the same. There seems no way to completely disable it in gtk3. So we have to choose between:
|
fix typo
There was a problem hiding this comment.
The code looks nice and the MimeType type cleans things up well
On question: have you checks if there were other occurrences of mimetypes in the code? If so, you can use the new class and the constexpr defs there as well -- the constexpr definitions make typos almost impossible.
As for the choice in behaviour: losing data because the user has not been warned must be avoided at any cost.
I don't believe in "DE/platform"-based code. Behaviour may vary from one version (e.g. of Dolphin) to the other. Moreover, having XDG_CURRENT_DESKTOP set to KDE does not imply the portal configuration is that of KDE. The user may be using some third party file manager and have configured their portal to use that.
The requirements for the File Chooser portal do not mention file overriding at all, so we cannot assume anything, nor can we report to that or that implementation "you don't check if a file already exists" (even for Linux targets).
MacOS and Windows raise another set of questions.
I'm leaning towards a double check on Dolphin's portal, but we have to check the behaviour in other common situations before deciding. That includes:
- KDE with and without GTK_USE_PORTAL
- Gnome with and without GTK_USE_PORTAL
- MacOS (is there something to configure there?)
- Windows (is there something to configure there?)
As a note for other maintainers: moving to native dialogs is a step we will have to take in the long run: past GTK4.10, we should use GtkFileDialog which is a rewrite of GtkFileChooserNative.
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
|
I also double-checked there shouldn't be any Mime type usages anymore. |
This reverts commit ff12afd.
What dialogs need to be checked?
Did I miss one? |
|
Template page format file (.xopt) in Journal->Configure Page Template |
|
And for export, I guess it'd be best to check both the "Export as..." and the "Export as PDF" cases. |
|
I found a case that is not working (on KDE, without GTK_USE_PORTAL): Click on |
|
Another issue: if you click on Save As and choose a file name that already exists (foobar or foobar.xopp yield the same result), you are (rightfully) asked to confirm your choice. If you click on "Select another filename", the file dialog now shows the working directory (so my build directory). It should be in the same directory as before or in the last saved directory recorded in the settings. [The same issue appears in any situation where you are asked to confirm] |
|
I also got a SegFault: Click on Export As... and select a file name without extension (the selected filter is PDF by default). Click on export and then Ok in the next dialog: SegFault. In the terminal, I get the warning and the crashlog |
|
About the SegFault: the problem is probably related to https://discourse.gnome.org/t/gtk4-c-gtkfilefilter-gtkfilechooser-vs-gtkfiledialog/23528 This probably means we'll have to rethink how we do things in CustomExportJob::showDialogAndRun() |
What about implementing it like in the Libadwaita redesign mockup: Of course it can be done without Libadwaita. When the chosen format is png: |
|
Yes, it seems to be what the GTK people are suggesting here for instance. It seems reasonable to me. |
|
We may also consider never changing the filename ourselves (to add an extension) and trust the "native" dialog to check and ask if a file already exists.
|
This PR changes the file dialogs to use GtkFileChooserNative for XDG Portal support. This means that using GTK_USE_PORTAL=1 xournalpp would use the DE's File Picker instead of the GTK one. Which is very convenient especially on KDE (since the dolphin file picker is not as similar as the GTK and gnome one is).
As far as my testing can tell everything should work except the export to PDF dialog. It strangely behaves when closing the dialog instead of exporting the file (loading cursor and UI wouldn't respond).
This should close #5460 when merged.