bilelmoussaoui / ashpd

A Rust wrapper around XDG portals DBus interfaces
https://bilelmoussaoui.github.io/ashpd/ashpd/
MIT License
240 stars 44 forks source link

desktop: Make trait SessionPortal public #235

Closed dimtpap closed 1 month ago

dimtpap commented 1 month ago

Make the SessionPortal trait public so that users can use it in trait bounds.

For example I'd like to have a newtype around Session that calls close on Drop. Currently I can't do that in a generic way, because I can't use the trait as a bound since it's private.

struct CloseOnDrop<'s, Portal: ashpd::desktop::session::SessionPortal>(Session<'s, Portal>);

impl<P: ashpd::desktop::session::SessionPortal> Drop for CloseOnDrop<'_, P> {
    fn drop(&mut self) {
        pollster::block_on(self.0.close());
    }
}
error[E0603]: module `session` is private
   --> src/backend/connection.rs:192:25
    |
192 | impl<P: ashpd::desktop::session::SessionPortal> Drop for CloseOnDrop<'_, P> {
    |                         ^^^^^^^  ------------- trait `SessionPortal` is not publicly re-exported
    |                         |
    |                         private module

To solve this, I re-exported the SessionPortal trait in the desktop module.

Because that would make users be able to implement the trait on any struct, I used the sealed trait trick.

I added trait Sealed the root lib.rs because I thought you may want to use this trick in the future for other traits in the library, but feel free to tell me if you prefer another location.

bilelmoussaoui commented 1 month ago

Oups, let me fix the CI on main first.

dimtpap commented 1 month ago

An alternative could be to remove the trait or the bound from struct Session completely if users can't make Session structs on their own. This would remove the need from users to specify bounds anywhere they use Session but would still type check to prevent misuse of different types of sessions

bilelmoussaoui commented 1 month ago

An alternative could be to remove the trait or the bound from struct Session completely if users can't make Session structs on their own. This would remove the need from users to specify bounds anywhere they use Session but would still type check to prevent misuse of different types of sessions

This is needed to avoid passing the wrong session. Eg, create one from the location portal and pass it to the screencast one, at compile time

dimtpap commented 1 month ago

This is needed to avoid passing the wrong session. Eg, create one from the location portal and pass it to the screencast one, at compile time

I know, but the SessionPortal trait is unnecessary for this. Session can have a PhantomData of T that is not bound by any trait and the type-checker would still catch the misuse.

Just saying what an alternative could be by the way, my preferred approach is what this PR does. I'm assuming you'd rather constrain the types that Session can take

bilelmoussaoui commented 1 month ago

This is needed to avoid passing the wrong session. Eg, create one from the location portal and pass it to the screencast one, at compile time

I know, but the SessionPortal trait is unnecessary for this. Session can have a PhantomData of T that is not bound by any trait and the type-checker would still catch the misuse.

Just saying what an alternative could be by the way, my preferred approach is what this PR does. I'm assuming you'd rather constrain the types that Session can take

Right, the changes in this PR are lgtm, thanks!