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

desktop: Make trait SessionPortal public #235

Merged
merged 1 commit into from
Sep 15, 2024
Merged

desktop: Make trait SessionPortal public #235

merged 1 commit into from
Sep 15, 2024

Conversation

dimtpap
Copy link
Contributor

@dimtpap dimtpap commented Sep 15, 2024

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.

@dimtpap dimtpap changed the title Make SessionPortal public desktop: Make trait SessionPortal public Sep 15, 2024
@bilelmoussaoui
Copy link
Owner

Oups, let me fix the CI on main first.

@dimtpap
Copy link
Contributor Author

dimtpap commented Sep 15, 2024

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
Copy link
Owner

bilelmoussaoui commented Sep 15, 2024

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
Copy link
Contributor Author

dimtpap commented Sep 15, 2024

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

Make SessionPortal public so that users can use it in bounds
Apply the sealed trait trick so that users are unable to implement it
@bilelmoussaoui
Copy link
Owner

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!

@bilelmoussaoui bilelmoussaoui merged commit a82a4e3 into bilelmoussaoui:master Sep 15, 2024
10 checks passed
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.

2 participants