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

[WIP] feat: Use GtkFileChooserNative if available to support XDG portals #19159

Open
wants to merge 9 commits into
base: master
from

Conversation

@tristan957
Copy link

tristan957 commented Jul 8, 2019

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

Release Notes

no-notes

This PR is backwards compatible.

@welcome

This comment has been minimized.

Copy link

welcome bot commented Jul 8, 2019

💖 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:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

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.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 8, 2019

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 currently am not sure how to cast a GtkFileChooserNative to a GtkNativeDialog
  • Does the include of dlfcn.h need to be guarded in anyway?

I am going to take a look at the Chromium source code, and see what I find.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 8, 2019

Please note that this was not on my dev machine, so I could not compile to check my work.

@tristan957 tristan957 force-pushed the tristan957:master branch from ab648f2 to 5d27666 Jul 8, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 8, 2019

Lint should be fixed. Apologies.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 8, 2019

@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 GtkFileChooserNative to GtkNativeDialog without the macro GTK_NATIVE_DIALOG that would be extremely helpful. This is dependent on whether or not GTK stays dynamically loaded or not.

@tristan957 tristan957 force-pushed the tristan957:master branch from 5d27666 to e1eb7bc Jul 8, 2019
@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Jul 9, 2019

Thanks a lot for picking this up!

@TingPing specifically if you see anything that can be done about casting a GtkFileChooserNative to GtkNativeDialog without the macro GTK_NATIVE_DIALOG that would be extremely helpful.

Honestly I think its acceptable to just ignore in this specific case and cast away to void*. (EDIT: Oh sorry, C++ is more strict about that right. I'm not sure off-hand then)

Hardcoding -ldl is not ideal as that isn't used on all unix-like systems.

Since this already depends on GLib I suggest using GModule which avoids the linking and header portability concerns.

You'd then use g_module_open(), g_module_symbol(), etc: https://developer.gnome.org/glib/stable/glib-Dynamic-Loading-of-Modules.html

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 9, 2019

Thanks for the link @TingPing. I will take a look at GModule later today and update the PR accordingly.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 9, 2019

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.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 9, 2019

@TingPing I was more wondering if the GtkFileChooserNative object is located in memory at the same spot as the GtkNativeDialog or does GTK_NATIVE_DIALOG move the pointer. I don't have a complete grasp of the GObject type system just yet. I can write it, but I don't know technicalities like this. If they are at the same spot, I can avoid the cast.

@tristan957 tristan957 force-pushed the tristan957:master branch 4 times, most recently from 8fcc326 to 4d36308 Jul 9, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 9, 2019

This is going to require a little more effort than previously thought. Still doable I think, but I am under the impression _dialog is going to have to change its type to a GtkFileChooser. I am also going to have to dip into libgtkui::SetGtkTransientForAura to account for this.

@electron-cation electron-cation bot removed the new-pr 🌱 label Jul 9, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 9, 2019

The behavior of FileDialog::RunAsynchronous needs to be re-evaluated as "delete-event" does not exist for GtkNativeDialog & GtkFileChooserNative. Also there is no "present with time" api, so that will need to be reevaluated.

Both CHROME_CALLBACKS need to be adjusted.

I am going to need some guidance on libgtkui::SetGtkTransientForAura. I cannot find the definition. I just need to overload the function to have a SetGtkTransientForAura(void* dialog, aura::Window* parent). I need use the underlying GtkNativeDialog if GTK_IS_WIDGET(dialog) fails because that API provides gtk_native_dialog_set_transient_for.

Any skilled Electron contributor would be a huge help at this point.

@tristan957 tristan957 force-pushed the tristan957:master branch 3 times, most recently from 72a53d3 to 6dd34aa Jul 9, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 10, 2019

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.

@tristan957 tristan957 force-pushed the tristan957:master branch 2 times, most recently from baf26ce to a36ba29 Jul 10, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 10, 2019

I have gotten the build where it only fails on the libgtkui::SetGtkTransientForAura call. Looking at the codebase, this seems like something in the chromium code base. Relevant: https://github.com/chromium/chromium/blob/5250f0fc05c7af7cf4512714ac832749547b1bd0/chrome/browser/ui/libgtkui/gtk_util.cc#L137.

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.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Jul 10, 2019

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.

@tristan957 tristan957 force-pushed the tristan957:master branch 2 times, most recently from c735f82 to fb7b5ac Jul 10, 2019
@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

Well fellow devs, it is done. I did not realize you had to use GTK_USE_PORTAL=1.
it_works

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

So some things left to do are decide on the shared object file to load because Neon apparently doesn't ship a symlink to libgtk-3.so, so maybe other distributions do the same?

@ckerr what can be done to move this toward the next electron release?

@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 8, 2020

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

Are you talking about the memory header?

@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 8, 2020

My review was:

    gtk_module_ = g_module_open("libgtk-3.so.0", G_MODULE_BIND_LAZY);

The unversioned soname is often not installed and GTK3 is ABI frozen at this point.

@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 8, 2020

You may want to use gtk_file_chooser_set_current_name() to avoid the window being called xdg-desktop-portal-kde

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

@TingPing I am a little confused as to why the GtkFileChooserNative constructor is not handling that. The first argument seems to be the title.

@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 8, 2020

Also the reason the Open button is gtk-open is because of this:

#if GTK_CHECK_VERSION(3, 90, 0)
// GTK stock items have been deprecated. The docs say to switch to using the
// strings "_Open", etc. However this breaks i18n. We could supply our own
// internationalized strings, but the "_" in these strings is significant: it's
// the keyboard shortcut to select these actions. TODO: Provide
// internationalized strings when GTK provides support for it.
const char* const kCancelLabel = "_Cancel";
const char* const kNoLabel = "_No";
const char* const kOkLabel = "_OK";
const char* const kOpenLabel = "_Open";
const char* const kSaveLabel = "_Save";
const char* const kYesLabel = "_Yes";
#else
G_GNUC_BEGIN_IGNORE_DEPRECATIONS
const char* const kCancelLabel = GTK_STOCK_CANCEL;
const char* const kNoLabel = GTK_STOCK_NO;
const char* const kOkLabel = GTK_STOCK_OK;
const char* const kOpenLabel = GTK_STOCK_OPEN;
const char* const kSaveLabel = GTK_STOCK_SAVE;
const char* const kYesLabel = GTK_STOCK_YES;
G_GNUC_END_IGNORE_DEPRECATIONS
#endif

Stock items are deprecated in GTK3 and should not be used ever, it should be safe to delete that entire conditional.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

I will fix the so name here in a sec.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

Ok I will delete those lines as well.

@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 8, 2020

@TingPing I am a little confused as to why the GtkFileChooserNative constructor is not handling that. The first argument seems to be the title.

Its always possible its a -kde specific bug if the -gtk portal works as expected. Neon will ofc have outdated versions of everything also.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

It doesn't look like a title gets set for GTK at all. I am not sure why that is.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 8, 2020

The comment from the GTK_STOCK code doesn't make sense. Is Electron not using gi18n for internationalization? What support are they expecting?

tristan957 added 9 commits Jul 8, 2019
With this commit, users on KDE/plasma will finally have support in
Electron for their native file choosers dialogs.
Not sure why this is
@tristan957 tristan957 force-pushed the tristan957:master branch from 3d82a4b to 754fb14 Mar 8, 2020
@TingPing

This comment has been minimized.

Copy link
Contributor

TingPing commented Mar 9, 2020

The comment from the GTK_STOCK code doesn't make sense. Is Electron not using gi18n for internationalization? What support are they expecting?

Well they aren't translating them as-is. You can mark them as translatable.

This isn't a problem though, thats how you use GTK. You translate strings you show to users.

@gusbemacbe

This comment has been minimized.

Copy link

gusbemacbe commented Mar 11, 2020

@bpasero, @TingPing, @tristan957 and @parkerlreed

Everybody trying to get it to use the portal you need to set the env var GTK_USE_PORTAL=1, be on KDE, and have xdg-desktop-portal-kde installed.

I have already xdg-desktop-portal-kde installed before, but I enved GTK_USE_PORTAL=1 and it did not open the KDE dialog. But I understood perfectly what happened... Electron that opens the GTK dialog is that Electron we installed via npm or natively from the official repository or PPA, and that Electron is still 7, so we had to change the command:

v10.0.0-nightly.20200226
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                $ GTK_USE_PORTAL=1 ./electron main.js

It opens the KDE dialog with success. The only solution is to upgrade to Electron 10, with the Tristan's merged PR.

@bpasero, @roblourens, @sandy081 and @Tyriar

Is there how to switch from 7 to 10 on VSCode? Your VSCode is still Electron 7.

@gusbemacbe

This comment has been minimized.

Copy link

gusbemacbe commented Mar 11, 2020

Update

@bpasero and @Tyriar

It seems that Electron 9 and Electron 10 do not work with VSCode OSS. :-/

@bpasero

This comment has been minimized.

Copy link
Contributor

bpasero commented Mar 11, 2020

I would appreciate if people could stop pinging me, I currently do not have the resources to follow this development, sorry. If you feel there is an issue with VSCode, please report it in our repository.

@gusbemacbe

This comment has been minimized.

Copy link

gusbemacbe commented Mar 11, 2020

I would appreciate if people could stop pinging me, I currently do not have the resources to follow this development, sorry. If you feel there is an issue with VSCode, please report it in our repository.

OK, I will stop. As for the issue, the issue is to build and run Code OSS with Electron 10, so it can not be asked on your repository, then better on Stack Overflow.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Mar 11, 2020

Yes, you have also pinged people who have nothing to do with Electron's integration within VS Code. We have a process in place to adopt new Electron versions over time so we will hit this eventually. New Electron versions aren't meant to just work with VS Code as they almost always require some migration/fixes to go in.

@tristan957

This comment has been minimized.

Copy link
Author

tristan957 commented Mar 11, 2020

@gusbemacbe slow it down homie. Rome was not built in a day. All I can do is push the PR forward, and eventually we will see it downstream. No rush!

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

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.