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

Support for open/save dialogs on x11. #1774

Closed
wants to merge 3 commits into from
Closed

Support for open/save dialogs on x11. #1774

wants to merge 3 commits into from

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented May 11, 2021

This is a first stab at doing open/save in the x11 backend (addressing #936). Very much in a WIP state, but I'm opening this because I have a few unresolved questions.

This is using the freedesktop.org DBus specification to launch a native file dialog, as suggested in #936. Of course it would be nice to have a druid-native dialog also, but this has the advantages of (a) being less work and (b) having support in the flatpak sandbox.

Dependencies

This introduces a number of new dependencies:

  • zbus/zvariant/serde: these are pretty much required for using DBus, I think.
  • zvariant_derive: could be avoided; it's currently saving about 50 lines of serde boilerplate.

I'm not currently depending on the ashpd, although I'd kind of like to. It would save probably around 100 lines right now, but has some advantages and disadvantages:

  • it's new, and doesn't seem to be used much yet
  • on the other hand, the maintainer is super responsive (and has been to me in the past)
  • it has a bunch of stuff we don't need
  • but we might want some of them in the future (e.g. notifications, trash, printing)

@ForLoveOfCats
Copy link
Collaborator

ForLoveOfCats commented May 11, 2021

I am extremely excited about this. I think that the x11 and Wayland shells are the best long term path forward for Druid and this is a huge step toward that. I especially love the fact that because of the portal I get a nice cushy KDE file dialog on my Plasma setup :)

I'm not very fond of pulling in a bunch more dependencies but taking a cursory look at zbus shows that it is not an entirely trivial problem that it is solving so that seems like a reasonable trade off.

@jneem
Copy link
Collaborator Author

jneem commented May 12, 2021

Yeah, unfortunately zbus isn't small (it adds 500k to a release binary), but I definitely don't think we should roll our own DBus implementation. The alternative seems to be dbus, which is a wrapper around the C library. I'm more inclined to go for the pure-rust version, though (plus, it's part of the official upstream DBus organization).

The other dependencies are less critical, but also much smaller 🤷

@runiq
Copy link

runiq commented May 12, 2021

Nothing but love for this PR <3

@maan2003 maan2003 added the shell/x11 concerns the X11 backend label May 13, 2021
@jneem jneem marked this pull request as ready for review May 21, 2021 14:54
@jneem
Copy link
Collaborator Author

jneem commented May 21, 2021

Ok, I think this is in slightly better shape now. zbus code size is still an issue. I dug into it a little and there seems to be a lot of monomorphized serde code...

@psychon
Copy link
Contributor

psychon commented May 22, 2021

If you want, I can actually try this out, but just from looking at the code: What happens if org.freedesktop.portal.FileChooser is unowned? I'd expect the DBus daemon to reply with some kind of error, but the code seems to ignore that error...?

(I might also just be misunderstanding how DBus works and this might all be fine)

@maan2003
Copy link
Collaborator

works!
But if I have 2 X servers running at tty1 and tty2. application is running on tty2. the file selector opens on tty1 😕

@jneem
Copy link
Collaborator Author

jneem commented May 26, 2021

I'd expect the DBus daemon to reply with some kind of error, but the code seems to ignore that error...?

Good point! I think long term we'll want to fall back to our own druid-implemented file dialog. But that's a lot of work, so for now I'll just log an error.

@jneem
Copy link
Collaborator Author

jneem commented May 26, 2021

works!
But if I have 2 X servers running at tty1 and tty2. application is running on tty2. the file selector opens on tty1 confused

hm, that's worrying. My understanding (from here was that dbus runs a daemon for each user login session, so I was expecting that to help somehow. I'll see if I can find some time to play around with this later in the week.

@jneem
Copy link
Collaborator Author

jneem commented May 29, 2021

So despite what the dbus page says, arch linux (and maybe other distros, I didn't check) has been doing per-user (instead of per-login-session) dbusses since 2015. I see some discussion here about involving the $DISPLAY variable somehow, but I don't know about more recent status...

@cmyr
Copy link
Member

cmyr commented Jun 29, 2021

What's the status of this? I don't have a great set up for testing on linux right now, but I'd like to help get this merged if there's anything I can do?

@jneem
Copy link
Collaborator Author

jneem commented Jun 29, 2021

It's currently blocked because xdg-desktop-portal doesn't support having more than one X session open, and I'm not sure whether they have any plans to...

@jneem jneem added the S-blocked waits for another PR or dependency label Jun 29, 2021
@maan2003
Copy link
Collaborator

I am in favor of leaving multiple X servers as a todo and merging this because running multiple X servers isn't very common anyways 🤷

@jneem
Copy link
Collaborator Author

jneem commented Jun 30, 2021

I agree that it isn't common, but the main thing that bothers me is that fixing this isn't actually something that we can do: we're using someone else's protocol and so we rely on them accepting a fix. If they aren't willing to do that, then this isn't a long-term solution.

@JAicewizard
Copy link
Contributor

Is there any reason we rely on flatpak?

@jneem
Copy link
Collaborator Author

jneem commented Jul 11, 2021

xdg-desktop-portal defines the dbus protocols that we use. It was originally created for flatpak because applications that use those protocols are easy to sandbox.

@bilelmoussaoui
Copy link

note that the portals are also used by snap

@jneem jneem mentioned this pull request Mar 17, 2022
@jneem
Copy link
Collaborator Author

jneem commented Mar 17, 2022

Superseded by #2153

@jneem jneem closed this Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked waits for another PR or dependency shell/x11 concerns the X11 backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants