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

LinuxApplicationExt: experiment with platforms specific extensions #1843

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

maan2003
Copy link
Collaborator

No description provided.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool, I think this makes sense. I've left some comments inline, but to me it's really just a matter of sorting out the details.

druid-shell/src/application.rs Outdated Show resolved Hide resolved
druid-shell/src/application.rs Outdated Show resolved Hide resolved
Comment on lines 85 to 86
#[cfg(target_os = "linux")]
pub use application::LinuxApplicationExt;
Copy link
Member

Choose a reason for hiding this comment

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

how we should expose these is a good question.

I like the pattern in std of having the std::os module, which contains std::os::unix, std::os:windows, etcetera; I could imagine having something like that?

hmhm:

If we made the platform module public we could put these in the particular root modules of the specific platforms, but that might get a bit messy.

Maybe what I would prefer instead would be that we have an ext module, and then ext::linux? Another option would be to rename platform to backend or something, and then we could use platform for this, which feels like a better name...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like platform::LinuxApplicationExt or platform::linux::LinuxApplicationExt. renaming existing platform to backend sound reasonable.

I would prefer shorter use statement:
use druid::shell::platform::LinuxApplicationExt;
over
use druid::shell::platform::linux::LinuxApplicationExt;

Copy link
Member

Choose a reason for hiding this comment

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

we could also have druid::platform::linux that reexports things from shell?

Now should we generally distinguish between platform::gtk and platform::x11?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess platform::linux::{GtkApplicationExt, X11ApplicationExt}. X11 and Wayland should be different though

@maan2003
Copy link
Collaborator Author

maan2003 commented Jul 3, 2021

these commits are best understandable separately

  • Move druid-shell/src/platform to druid-shell/src/backend c46c0e3
    renames platform to backend

  • platform -> backend in code 765016f
    fixes all the build errors and changes internal comments

  • public docs and error messages 976bc93
    changes the docs, error messages and README

@maan2003 maan2003 added the S-needs-review waits for review label Jul 3, 2021
@maan2003 maan2003 force-pushed the platorm-ext branch 4 times, most recently from ce969e5 to 4b64400 Compare July 5, 2021 08:51
@maan2003
Copy link
Collaborator Author

maan2003 commented Jul 5, 2021

Removed x11 impl for now because clipboard is changing in #1851

@cmyr
Copy link
Member

cmyr commented Jul 5, 2021

@maan2003 can we split this up, and do the 'platform -> backend' change as a single separate PR?

@maan2003
Copy link
Collaborator Author

maan2003 commented Jul 6, 2021

Sure 👍

@maan2003
Copy link
Collaborator Author

maan2003 commented Jul 6, 2021

rebased

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool, I've been wanting to see something like this for ages, and this feels like a very nice little test case to try it out with.

One question inline, otherwise I'm happy to get this merged!

/// Linux specific extensions to [`Application`]
///
/// [`Application`]: crate::Application
pub trait LinuxApplicationExt {
Copy link
Member

Choose a reason for hiding this comment

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

Is this 'linux' thing, or only a gtk thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a Linux thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a Linux thing? The primary selection (I wouldn't really call it "clipboard"...) is AFAIK an X11 thing and not implemented on Wayland. But my knowledge here might be out of date and I am too lazy to dig into Gtk's Wayland backend right now, so I am just leaving this as a comment here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦‍♂️ I really forgot wayland's existence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is a unstable wayland protocol and is supported by gtk, qt and by kde plasma, gnome and sway, so 🤷‍♂️

@maan2003 maan2003 merged commit 07cc61f into linebender:master Jul 7, 2021
@cmyr
Copy link
Member

cmyr commented Jul 14, 2021

@maan2003: I just realized something about this, and naming: basically I think this should just be called ApplicationExt, and the fact that it is for linux is determined by it's presence in the platform::linux module. This mimics what std does, where there are, say, four different MetadataExt traits, in four different submodules of std::os.

@maan2003
Copy link
Collaborator Author

makes sense, will fix this soon

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.

4 participants