feat: Use GtkFileChooserNative to support the XDG Desktop Portal specification #19159
Conversation
|
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
|
I would appreciate guidance in getting this PR to completion with regard to CI builds, styling, implementation, and testing. Off the bat, there are a few issues with this PR:
I am going to take a look at the Chromium source code, and see what I find. |
|
Please note that this was not on my dev machine, so I could not compile to check my work. |
|
Lint should be fixed. Apologies. |
|
@TingPing @ckerr since you guys were a part of the last PR, can you provide assistance on this one? @TingPing specifically if you see anything that can be done about casting a |
|
Thanks a lot for picking this up!
Hardcoding Since this already depends on GLib I suggest using GModule which avoids the linking and header portability concerns. You'd then use |
|
Thanks for the link @TingPing. I will take a look at |
|
Call for action: I do not currently run a KDE Plasma desktop. If someone who does could inevitably test this PR, that would be great. |
|
@TingPing I was more wondering if the |
|
This is going to require a little more effort than previously thought. Still doable I think, but I am under the impression |
|
The behavior of Both I am going to need some guidance on Any skilled Electron contributor would be a huge help at this point. |
|
I have also learned that preview widgets are ignored in the Native API. What should be the best course of action regarding that? Leaving the preview widget code in regardless of what dialog is actually being used seems best to me instead of making conditional checks around the code. |
|
I have gotten the build where it only fails on the So...I think this leaves a couple of options. Ask the Chromium developers for an overloaded function to be added, or mirror the function in Electron. I am thinking mirroring the function in Electron will work best. |
|
I have come up with the following patch diff --git a/shell/browser/ui/file_dialog_gtk.cc b/shell/browser/ui/file_dialog_gtk.cc
index ef6d3a545..50e536ad5 100644
--- a/shell/browser/ui/file_dialog_gtk.cc
+++ b/shell/browser/ui/file_dialog_gtk.cc
@@ -77,7 +77,10 @@ class FileChooserDialog {
if (parent_) {
parent_->SetEnabled(false);
- libgtkui::SetGtkTransientForAura(dialog_, parent_->GetNativeWindow());
+ if (GTK_IS_WIDGET(dialog_))
+ libgtkui::SetGtkTransientForAura(dialog_, parent_->GetNativeWindow());
+ else
+ SetGtkTransientForAura(parent_->GetNativeWindow());
if (GTK_IS_WINDOW(dialog_)) {
gtk_window_set_modal(GTK_WINDOW(dialog_), TRUE);
} else {
@@ -225,6 +228,19 @@ class FileChooserDialog {
CHROMEG_CALLBACK_0(FileChooserDialog, void, OnUpdatePreview, GtkFileChooser*);
DISALLOW_COPY_AND_ASSIGN(FileChooserDialog);
+
+ // Sets |dialog| as transient for |parent|, which will keep it on top and
+ // center it above |parent|. Do nothing if |parent| is nullptr.
+ void SetGtkTransientForAura(aura::Window* parent) {
+ if (!parent || !parent->GetHost())
+ return;
+
+ void (*dl_gtk_native_dialog_set_transient_for)(void*, GtkWindow*) = nullptr;
+ g_module_symbol(
+ gtk_module_, "gtk_native_dialog_set_transient_for",
+ reinterpret_cast<void**>(&dl_gtk_native_dialog_set_transient_for);
+ dl_gtk_native_dialog_set_transient_for(dialog_, reinterpret_cast<GtkWindow*>(parent));
+ }
};
void FileChooserDialog::OnFileDialogResponse(GtkFileChooser* widget, int response) {I have decided to merge this patch for the time being. It can be reverted though. |
The type casting macros in GLib do two things: They do a type check and then a regular C style cast. They do not change the pointer in any way. |
It always should have been watching the EDIT: I see it just hides the dialog on delete. This is fine I suppose but this doesn't need to have that signal to do it, it can be done from the response. |
Nothing can be done about that. Native dialogs cannot be modified. |
|
Company and mclasen were helpful on #gtk yesterday. @TingPing when you close out of a GtkDialog, what is the response code? Is it the same as hitting cancel? |
|
|
I can probably include a workaround for that event then. Is this even still an issue on KDE systems. I made a post on /r/kde, and people seem to get the plasma file chooser in electron applications. |
Which electron application was that? My electron app does not have a native KDE dialog. If you package with Flatpak it will be a native dialog due to portals used there afaik. |
|
Does anyone know why Electron has a separate file chooser implementation than Chromium? @polarathene I have responded on Reddit so I'll let you know |
The Chromium implementation of file chooser was limited of features, and could not meet Electron's feature requests. |
|
As I understand it Chromium might be moving in a direction where they always use the ChromiumOS file chooser and not native toolkits also. So Electron probably wants to retain that native integration. (I pointed them in the direction of the file chooser portal but I don't really follow Chromium much) |
|
Does anyone know how to pull an electron package out of circle CI so I could use it in a node project, or does the CI not produce those? |
shell/browser/ui/file_dialog_gtk.cc
Outdated
| void* (*dl_gtk_file_chooser_native_new)(const char*, GtkWindow*, | ||
| GtkFileChooserAction, const char*, | ||
| const char*) = nullptr; | ||
| bool found = g_module_symbol( |
There was a problem hiding this comment.
we shouldn't call g_module_symbol() if gtk_module_ is false -- maybe it should be
| bool found = g_module_symbol( | |
| bool found = (gtk_module_ != nullptr) && g_module_symbol( |
There was a problem hiding this comment.
gtk_module_ should never be NULL, good idea to handle that ofc but it should be a loud warning when your app using GTK is loading a non-existing so.
There was a problem hiding this comment.
I am thinking we could make gtk_symbol_ a static class var, that way the only time we go searching for the lib is on the first invocation thoughts? Then it could be (gtk_module_ != NULL) || g_module_symbol...
There was a problem hiding this comment.
One question for that: when do we release the module?
But if we go the static field route, we could also cache the results of the lookups by having them all be static fields as well.
There was a problem hiding this comment.
Also, to be clearer about my gtk_module_ != nullptr suggestion: the version I was reviewing had the test later in the code logic:
if (gtk_module_ != nullptr && found ...My suggestion was to move this test to before the g_module_symbol() call.
There was a problem hiding this comment.
@ckerr good point on the release. Static fields for the functions might be a better solution. If none of the fields are NULL, don't do a module search. Let's say I go through the process of dynamically calling all the functions, while saving them as static class functions at the same time, do the dynamically loaded functions reside in memory regardless if we create a GModule object or not? @TingPing
There was a problem hiding this comment.
It doesn't matter if its never released, the module is loaded as long as the process is a live anyway.
There was a problem hiding this comment.
@ckerr I will defer to your opinion on any static class variables (function pointers/module).
|
@ckerr thanks for taking a look at this. I will review and update the PR according to your suggestions hopefully later this weekend. |
`GtkFileChooserNative` has merged and will be landed in Electron 14 (electron/electron#19159)
|
Hi, [Edit] : no consistent result depending on distributions. Cannot have ubuntu family work. Did have some success on Arch. Weird. |
|
No one’s been able to figure out the cause of this feature seemingly not working on Ubuntu (or in Flatpak) so I’ve opened a follow up: #31258 If anyone has ideas on what might be the problem please share them. |
Contains: - Chromium 91 -> 96, including multiple webrtc and security fixes - Linux file chooser portal fixes (electron/electron#19159) Closes: #629
Contains: - Chromium 91 -> 96, including multiple webrtc and security fixes - Linux file chooser portal fixes (electron/electron#19159) Closes: jitsi#629
Description of Change
With this commit, users on KDE/plasma will finally have support in Electron for their native file chooser dialogs. Fixes #2911.
Checklist
npm testpassesRelease Notes
Notes: Added support for alternative file choosers on Linux through the usage of the XDG Desktop Portal in
GtkFileChooserNative.This PR is backwards compatible to support platforms like Ubuntu 16.04/14.04 whose GTK versions are less than 3.20.