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

use XDG Desktop Portal on Linux & BSDs #41

Merged
merged 10 commits into from
Jan 18, 2022
Merged

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jan 18, 2022

This new backend does not support MessageDialog nor
AsyncMessageDialog because there is no corresponding API in the
XDG Desktop Portal.

The GTK backend is still available with the new gtk3 Cargo
feature.

Fixes #36

use crate::backend::FilePickerDialogImpl;
impl FilePickerDialogImpl for FileDialog {
fn pick_file(self) -> Option<PathBuf> {
let connection = block_on(zbus::Connection::session()).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the zbus::Connection be saved persistently in the FileDialog struct instead of creating a new one with each function invocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bilelmoussaoui what do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this would require turning FileDialog and AsyncFileDialog into traits rather than structs so that backends could implement them on different structs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often you would invoke the function? connecting to the session bus shouldn't be expensive; unless you do multiple calls to various portals at once, there shouldn't be a huge need to store the connection I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invoked any time the application shows a file dialog. If this is cheap, then I think it's probably not worth the trouble refactoring. @PolyMeilex what do you think?

This new backend does not support MessageDialog nor
AsyncMessageDialog because there is no corresponding API in the
XDG Desktop Portal.

The GTK backend is still available with the new `gtk3` Cargo
feature.

Fixes PolyMeilex#36
use ashpd::desktop::file_chooser::{
FileChooserProxy, FileFilter, OpenFileOptions, SaveFileOptions,
};
// TODO: convert raw_window_handle::RawWindowHandle to ashpd::WindowIdentifier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should provide a feature for that in ashpd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PolyMeilex
Copy link
Owner

PolyMeilex commented Jan 18, 2022

Looks good!

But I'd like to keep GTK as the default (sadly), desktop portals are often not present at all, so it should be a conscious decision of downstream devs to not support those users rather than ours. So GTK should be out-out, and portals opt-in (as GTK provides desktop portal support, so if GTK is the default then ours is not needed)

@bilelmoussaoui
Copy link
Contributor

Gtk being the default means a KDE desktop user would get a GtkFileChooser instead of a Qt one

@PolyMeilex
Copy link
Owner

PolyMeilex commented Jan 18, 2022

Gtk being the default means a KDE desktop user would get a GtkFileChooser instead of a Qt one

Yep, I'm aware of that, I'm willing to sacrifice this in favor of making sure that everyone gets a dialog.

Could we perhaps do a hybrid approach, so if ashpd fails we fallback to GTK? That would make sure that KDE users see their dialog. And GTK would still be out-out feature.

@bilelmoussaoui
Copy link
Contributor

That can work yes and would my favourite option of the three

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

desktop portals are often not present at all

Really? Do you have a citation for this or have you tested it yourself? What distros with what desktop environments do not provide the XDG Desktop Portal at all?

I do not think linking large C libraries should be the default.

@PolyMeilex
Copy link
Owner

PolyMeilex commented Jan 18, 2022

desktop portals are often not present at all

Really? Do you have a citation for this or have you tested it yourself? What distros with what desktop environments do not provide the XDG Desktop Portal at all?

I do not think linking large C libraries should be the default.

Basically any non DE environment, i3/awesomewm/openbox/dwm/xmonad, as for citation one of my machines runs sway compositor, and It does not have gtk-portal installed because it's coexistence with wlr-portal is buggy for some reason.

And yeah, I don't want to link GTK either, but I don't want to exclude part of users by default, some consumers of RFD may not even know what Linux is, so I can't expect them to know if they should enable GTK or not.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

does not have gtk-portal installed because it's coexistence with wlr-portal is buggy

I just tried installing Sway and removing all xdg-desktop-portal-* packages. As expected, without any portal installed this branch did not work. After installing xdg-desktop-portal-wlr and xdg-desktop-portal-kde then restarting Sway, I got the KDE file dialog as expected. After removing xdg-desktop-portal-kde and installing xdg-desktop-portal-gtk, I got the GTK file dialog as expected. So I don't know what issue you're referring to. Is there an upstream bug report?

@PolyMeilex
Copy link
Owner

I'm too lazy to find them all, but it's basically unstable, sometimes wlr screen sharing stops working out of nowhere because of gtk portal, sometimes GTK apps take forever to launch (swaywm/sway#5732), problems come and go from time to time, so may people just don't bother to fix them constantly, some don't even know what portals are especially if they don't care about flatpak at all.

In X11 WMs world, it's more of a case of people not wanting to "bloat" their systems. Sounds stupid, but yeah, I want to support those users too.

If XDG portals were universally adopted standard everywhere, I would never bother with implementing GTK backend. It's super tedious to maintain in comparison with a simple dbus based solution, and I'd love to get rid of it, but I can't.

A compromise I would be willing to take would be using Zenity in order to drop GTK dep, but I'm not sure what are it's limitations.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

Could we perhaps do a hybrid approach, so if ashpd fails we fallback to GTK?

How might this be implemented? I'm not clear how that could work.

@PolyMeilex
Copy link
Owner

Not sure to be honest, this crate was written with one active backend at the same time in mind, so this change would not be simple.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

sometimes wlr screen sharing stops working out of nowhere because of gtk portal

I searched the xdg-desktop-portal-wlr issue tracker for issues mentioning "GTK" and as far as I can tell, none of the issues people encountered were caused by xdg-desktop-portal-{gtk, gnome, kde} being installed. AFAICT all of the issues with xdg-desktop-portal-wlr were cased by people not having a required package installed, not having the required systemd services enabled, or otherwise having a misconfigured environment. This is consistent with the xdg-desktop-portal-wlr troubleshooting guide. All of those problems were with using the screencast or screenshot portals, which is all that xdg-desktop-portal-wlr implements. The only issues I saw people encountering with other portals on the xdg-desktop-portal-wlr issue tracker were because they did not have a portal installed (GTK or KDE) that implemented what they were trying to use. So, I don't think there's really an issue making this Rust library depend on the XDG Desktop Portal FileChooser dbus API.

In X11 WMs world, it's more of a case of people not wanting to "bloat" their systems. Sounds stupid, but yeah, I want to support those users too.

Needing to install xdg-desktop-portal-gtk in addition to GTK isn't asking much. If someone really has an objection to that, frankly, I don't care. I don't want to bloat my Rust application with C dependencies; each additional C dependency makes it more cumbersome for new developers to build my application and packagers to package it.

If XDG portals were universally adopted standard everywhere

Anyone packaging a Rust application that uses rfd would need to ensure either xdg-desktop-portal-gtk or xdg-desktop-portal-kde is installed as a runtime dependency. Even if those are not installed by default in some edge cases, installing an application that uses rfd should install it. For the Flatpak case I'm not totally clear how that would work. I would expect distributions to ensure either xdg-desktop-portal-{gtk, kde} is installed together with the Flatpak tooling or the GTK version in the Freedesktop Flatpak runtime to be used as a fallback.

@PolyMeilex
Copy link
Owner

frankly, I don't care. I don't want to bloat my Rust application with C dependencies

So just out-out of GTK feature, and the problem will be gone.
I will most likely do it in my projects as well (as this is usually the last bastion of C code in my builds), but I'm not willing to push this by default for all downstream users.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

We could bikeshed about defaults forever; in the end, it doesn't really matter much because it's easy to edit one line in Cargo.toml.

More importantly, I don't think there's a compelling reason for doing a substantial refactor of this library to implement the previously proposed hybrid using the XDG Desktop Portal by default with a runtime fallback to GTK.

@PolyMeilex
Copy link
Owner

More importantly, I don't think there's a compelling reason for doing a substantial refactor of this library to implement the previously proposed hybrid using the XDG Desktop Portal by default with a runtime fallback to GTK.

Yeah, I agree with that, it's too much hassle for nearly no gain. You either want to support legacy dialogs or not, no need to complicate stuff with middleground solutions.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 18, 2022

I made gtk3 a default feature. Unfortunately, Cargo does not support OS-specific features, which makes this misleading. However it still builds on Windows and macOS because the GTK code is only built on Linux & BSDs.

I think this is ready to merge. I can follow this up with another PR to update the documentation, then I think it would be good to make a new release.

@PolyMeilex
Copy link
Owner

Sure, thanks!
I think it's good to go

@PolyMeilex PolyMeilex merged commit e331eff into PolyMeilex:master Jan 18, 2022
@Be-ing
Copy link
Contributor Author

Be-ing commented Jan 19, 2022

Follow up PR for documentation: #43

Big thanks to @bilelmoussaoui for writing ashpd! Before I found ashpd, I looked into doing this directly with zbus but realized it would have been significantly more work than this was.

@Be-ing Be-ing deleted the xdg branch January 19, 2022 06:05
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 18, 2022

In retrospect, this was well worth the effort to remove the huge C dependencies on GTK. This together with doing some work on my other dependencies to optionally use dlopen instead of linking (slint-ui/slint#956 RustAudio/rust-jack#162) produces an executable that only links glibc and Linux:

moire on  main [$!] is 📦 v0.1.0 via 🦀 v1.58.1 took 2m8s
❯ ldd target/debug/moire
        linux-vdso.so.1 (0x00007fffd3f4a000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f0ebdddb000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f0ebdcff000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f0ebdaf5000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f0ebe7d0000)

This makes cross compiling to other CPU architectures trivial. 😁

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

Successfully merging this pull request may close these issues.

use XDG Desktop Portal on Linux
3 participants