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 for GTK File Dialog #15293

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@denisfalqueto
Copy link

commented Oct 19, 2018

Closes #2911.

GtkFileChooserNative has support for FileChooser XDG Portal.
That allows for native file open/save dialogs on GTK and QT/Plasma
environments.

Signed-off-by: Denis Falqueto denisf@trt3.jus.br

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: Use native file chooser dialog for opening and saving files.

Use GtkFileChooserNative for GTK File Dialog
GtkFileChooserNative has support for FileChooser XDG Portal.
That allows for native file open/save dialogs on GTK and QT/Plasma
environments.

Signed-off-by: Denis Falqueto <denisf@trt3.jus.br>

@denisfalqueto denisfalqueto requested a review from as a code owner Oct 19, 2018

@welcome

This comment has been minimized.

Copy link

commented Oct 19, 2018

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

@ckerr
Copy link
Member

left a comment

I'm a little unsure about the use case for this patch and would like to get some discussion on it before it's merged.

First, this is a first submitter patch and I'm always happy to see people improving Linux support, so I'm happy to see this patch.

But I'm not certain it's wise to require GTK+ 3.20 or higher, as that would break out-of-the-box support for Ubuntu 16.04 and older.

I'm also not sure what the use case for this patch is: Electron apps running on Windows or macOS likely won't be using ui/file_dialog_gtk.cc in the first place, and so wouldn't the native support be moot?

@ssokolow

This comment has been minimized.

Copy link

commented Oct 21, 2018

@ckerr GTK+ provides a mechanism for indirecting native dialogs through a desktop-provided out-of-process common dialog handler when GtkFileChooserNative is used.

There are two main reasons to do this:

  1. The mechanism makes it possible for desktops which don't use GTK+ file choosers, like KDE and LXQt, to supply their native file choosers to Electron applications. (Currently, when running outside of Flatpak, you need to set the GTK_USE_PORTAL environment variable to opt into this behaviour.)
  2. It provides transparent integration with Flatpak sandboxing, since the privileged out-of-process dialog host can poke a hole in the sandbox before returning the path to the Electron app. (There is also work being done toward having Snappy share the same portal infrastructure.)
@TingPing

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

But I'm not certain it's wise to require GTK+ 3.20 or higher, as that would break out-of-the-box support for Ubuntu 16.04 and older.

Would just have to be a run time check, thankfully its only two symbols.

@ckerr

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

@TingPing, I'd be OK with a runtime check. How would this be done?

GTK_CHECK_VERSION is a compile-time test, so that's not going to work.

I suppose we could use dlopen + dlsym, but I'm not sure what the implications are of doing that when we already have a normal initialized GTK+ session?

@TingPing

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

I suppose we could use dlopen + dlsym, but I'm not sure what the implications are of doing that when we already have a normal initialized GTK+ session?

Yup. It is safe:

man dlopen

If the same shared object is loaded again with dlopen(), the same object handle is
returned. The dynamic linker maintains reference counts for object handles, so a dynami‐
cally loaded shared object is not deallocated until dlclose() has been called on it as
many times as dlopen() has succeeded on it. Any initialization returns (see below) are
called just once.

@TingPing

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

@TingPing if you could look into the linux CI failures, we can get this PR moving!

The dialog_ type has to be changed to GtkNativeDialog and everything that references it has to be updated accordingly:

GtkWidget* dialog_;

(but the conversation about dynamically loading the symbols should probably be handled also)

@TingPing

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

@TingPing ping, is there any progress on this?

I didn't write this patch and I don't have the time or desire to compile Electron. I pointed out the (obvious) issue with this PR so I guess the author isn't interested.

@codebytere

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

apologies, tagged the wrong person!

@denisfalqueto ping, is there any progress on this?

@codebytere codebytere changed the title [wip] Use GtkFileChooserNative for GTK File Dialog [wip] feat: use GtkFileChooserNative for GTK File Dialog Feb 4, 2019

@codebytere

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Closing as there has been no activity on this PR or response from the author.

Happy to re-open should there be more activity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.