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

Add NativeFileDialog class #60603

Closed
wants to merge 1 commit into from
Closed

Add NativeFileDialog class #60603

wants to merge 1 commit into from

Conversation

spindlebink
Copy link

@spindlebink spindlebink commented Apr 29, 2022

This PR adds a NativeFileDialog class, interface modeled after FileDialog, which wraps a header-only cross-platform dialog library to display native file dialogs to the user.

Most features of FileDialog are present in NativeFileDialog, except for the Control-specific ones and a couple of others that didn't make sense with the wrapper nature.

I've also provided a rather bad but at least present icon for the class and a class reference that I think covers the whole class. Future versions need better error checking, and I'd also like to eventually tie into the other features of the library I hooked into, which supports notifications and modal dialogs as well, although those'll be separate components/classes.

This is my first real PR, so I'd appreciate any notes regarding either code or otherwise. I hope to contribute more frequently.

Relevant issue: godotengine/godot-proposals#1123

Edit: Had a chat with @YakoYakoYokuYoku about some related PRs (#60501 and #47499) and the current proposal is for this implementation of NativeFileDialog to serve as an API template and preliminary implementation until eventual platform-specific implementations can be written using TrayBroker or a permutation of it.

@spindlebink spindlebink requested review from a team as code owners April 29, 2022 00:29
@Calinou Calinou added this to the 4.0 milestone Apr 29, 2022
@spindlebink spindlebink requested review from a team as code owners April 29, 2022 11:02
@YakoYakoYokuYoku
Copy link

I have an issue with this PR is that it collides with the purpose of #60501 as two of its purposes are to provide unified interaction with the desktop and, for Linux/BSD specifically, a DBus client for desktop services. The former might or might not apply with every communication you do with the desktop, but the latter should not be ignored as the FileChooser provided by xdg-desktop-portal is more reliable than zenity/kdialog in sandboxed applications (and probably in non-sandbox too).

But I don't want this to let you down as you can still do changes to this and take parts from Portable File Dialogs and use them with TrayBroker instead.

@bruvzg
Copy link
Member

bruvzg commented Apr 29, 2022

This implementation won't solve the issue with file system access by sandboxed macOS applications. It's probably won't work from a sandboxed macOS app at all. Sandboxed macOS apps can access most of the file system only via system file dialog, and require cookies to be saved to preserve access right across sessions. See #47499 for limited native file dialog implementation, done specifically for this purpose.

@YakoYakoYokuYoku
Copy link

YakoYakoYokuYoku commented Apr 29, 2022

@bruvzg @spindlebink should we start a discussion/thread on RocketChat?

@reduz
Copy link
Member

reduz commented Jul 29, 2022

I am not against this, I think it looks interesting but I have two recommendations:

  • Do not put this as core, it could be a pfd module
  • Seems a little missing in functions, is this all that is exposed by the pfd library?

@spindlebink
Copy link
Author

This is pretty much the extent of PFD's functionality as far as file dialogs go. It also supports notifications, I think, but I haven't tied into them here as my goal was to do as close a representation of FileDialog as I could for now.

@deakcor
Copy link
Contributor

deakcor commented Aug 20, 2022

Ok I tested this PR and I have some feedbacks :

  • I think should be better if variables and functions are aligned with FileDialog.
    • So renomming start_directory by current_path (because yes it works with a file path to put default filename)
    • file_mode by mode (aligning the enum should be nice too)
  • It always emit file signal even when dialog is cancelled. Maybe adding a check in _emit_signals_if_necessary to check if the result is empty.
  • There is no signal when dialog is closed. Could add in _notification function a hide signal. (Ones again to align with FileDialog behaviors)
  • Paths don't auto modify to work with current OS as Godot do (so for Windows we should manually replace / by \)
  • In open folder mode, starting path doesn't work at all for me, maybe a bug in the lib ?
  • In save file mode, current extension selected isn't added to the returned path so it's impossible to know the selected extension if not manually added in filename. But yes it's due to the lib I saw an issue about it.
  • Also response path should be parsed in utf8 to handle Chinese characters String().utf8(dialog->result().c_str()) something like that

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Sep 8, 2022
@98teg
Copy link
Contributor

98teg commented Dec 9, 2022

* In open folder mode, starting path doesn't work at all for me, maybe a bug in the lib ?

It can be solved by always adding a slash to the end of the path.

98teg added a commit to 98teg/NativeDialogs that referenced this pull request Dec 9, 2022
98teg added a commit to 98teg/NativeDialogs that referenced this pull request Dec 9, 2022
@MewPurPur
Copy link
Contributor

MewPurPur commented Jul 18, 2023

<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg"><path d="M2.5.75A1.75 1.75 0 0 0 .75 2.5v8a1.75 1.75 0 0 0 1.75 1.75h4.75v2H3a.75.75 0 0 0 0 1.5h10a.75.75 0 0 0 0-1.5H8.75v-2h4.75a1.75 1.75 0 0 0 1.75-1.75v-8A1.75 1.75 0 0 0 13.5.75zm11 1.5a.25.25 0 0 1 .25.25v8a.25.25 0 0 1-.25.25h-11a.25.25 0 0 1-.25-.25v-8a.25.25 0 0 1 .25-.25zM4 3.5a.5.5 0 0 0-.5.5v5a.5.5 0 0 0 .5.5h8a.5.5 0 0 0 .5-.5V5.75a.5.5 0 0 0-.5-.5h-2c-1.75 0-1.75-1.75-3.5-1.75z" fill="#fff"/></svg>

Here's a simpler SVG that also doesn't have elements off-screen!

@YuriSizov
Copy link
Contributor

I think this PR is going to be eventually rejected, since #47499 and #79574 add native file dialog support as a simple toggle on FileDialog (only for macOS and Windows so far).

@lostminds
Copy link

lostminds commented Sep 24, 2023

I think this PR is going to be eventually rejected, since #47499 and #79574 add native file dialog support as a simple toggle on FileDialog (only for macOS and Windows so far).

These have now been merged (for 4.2?) so we have another way of accessing native file dialogues via the FileDialog.use_native_dialog property and the same has also been implemented for linux in #80104. So it seems all the functionality of this PR has now been implemented in a different way and this can be closed?

@KoBeWi
Copy link
Member

KoBeWi commented May 14, 2024

Closing as superseded by the aforementioned PRs.
Thanks for your contribution nonetheless.

@KoBeWi KoBeWi closed this May 14, 2024
@KoBeWi KoBeWi removed this from the 4.x milestone May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.