-
Notifications
You must be signed in to change notification settings - Fork 44
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
WIP: Add activation token support #70
base: master
Are you sure you want to change the base?
Conversation
d46bb5d
to
53fdb75
Compare
c75f0b9
to
0deb5a5
Compare
f9cff40
to
3ee93b2
Compare
0e837ed
to
d2c5fe1
Compare
8f08328
to
8578f4f
Compare
eccbae8
to
6447075
Compare
@@ -29,7 +29,7 @@ gdk3x11 = {package = "gdkx11", version = "0.16", optional = true} | |||
gdk3wayland = {package = "gdkwayland", version = "0.16", optional = true} | |||
gtk3 = {package = "gtk", version = "0.16", optional = true} | |||
|
|||
gdk4wayland = {package = "gdk4-wayland", version = "0.5", optional = true} | |||
gdk4wayland = {package = "gdk4-wayland", version = "0.5", optional = true, features = ["wayland_crate"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct, the feature should be enabled only if gtk4_wayland
is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that looks correct to me
}; | ||
|
||
#[derive(SerializeDict, Type, Debug, Default)] | ||
#[zvariant(signature = "dict")] | ||
struct OpenDirOptions { | ||
handle_token: HandleToken, | ||
activation_token: Option<String>, | ||
activation_token: ActivationToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is optional so why make it no longer the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is ActivationToken::None which is the default so wrapping in option seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it is simpler to take Impl<Into<Option<T>>
that way you don't have to handle both cases in your app side :) see all the other options.
/// Create an instance of [`ActivationToken`] from a Wayland surface and the | ||
/// application's id. | ||
pub async fn from_wayland_surface( | ||
app_id: &str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have plenty of places where we use the app_id
so maybe we should have a type for it that wraps a String
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its way more convenient for the user to just write their app_id in a &str, ideally it would be preferable to somehow get it from the wayland toplevel or gtkwindow..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, you could make those functions take a impl Into<AppID>
and impl From<&str> for AppID
but as it is something that can be done in a separate PR, I will handle it then you can just rebase.
#[cfg(all(feature = "gtk3", feature = "wayland"))] | ||
Self::Gtk3(activation_token) => activation_token.token.as_str(), | ||
Self::Raw(string) => string.as_str(), | ||
Self::None => "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between None and Raw? You can make the default impl use Raw("")
? :)
pub async fn launch( | ||
&self, | ||
desktop_file_id: &str, | ||
activation_token: ActivationToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should definitely be impl Into<Option<ActivationToken>>
#[must_use] | ||
/// Sets the activation token for the application. | ||
pub fn activation_token(mut self, activation_token: ActivationToken) -> Self { | ||
if activation_token.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is why I think this should definitely be optional instead of these workarounds
/// See https://wayland.app/protocols/xdg-activation-v1 | ||
#[derive(Debug, Type)] | ||
#[zvariant(signature = "s")] | ||
pub enum ActivationToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a TODO here to implement a X11 only thing.
TODO: