WIP: Add activation token support #70
Conversation
A6GibKm
force-pushed
the
wayland2
branch
9 times, most recently
from
d46bb5d
to
53fdb75
Compare
A6GibKm
force-pushed
the
wayland2
branch
11 times, most recently
from
c75f0b9
to
0deb5a5
Compare
A6GibKm
force-pushed
the
wayland2
branch
2 times, most recently
from
f9cff40
to
3ee93b2
Compare
A6GibKm
force-pushed
the
wayland2
branch
2 times, most recently
from
0e837ed
to
d2c5fe1
Compare
A6GibKm
force-pushed
the
wayland2
branch
3 times, most recently
from
8f08328
to
8578f4f
Compare
A6GibKm
force-pushed
the
wayland2
branch
9 times, most recently
from
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.
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.
not sure what you mean here?
| }; | ||
|
|
||
| #[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.
It is optional so why make it no longer the case?
There was a problem hiding this comment.
There is ActivationToken::None which is the default so wrapping in option seems redundant.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
We should add a TODO here to implement a X11 only thing.
TODO: