-
Notifications
You must be signed in to change notification settings - Fork 82
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
Implement support for xdg-activation #426
Conversation
070cb9f
to
f41bec4
Compare
src/activation.rs
Outdated
/// *Warning*: Many compositors will issue invalid tokens for requests without | ||
/// recent serials. There is no way to detect this from the client-side. | ||
fn serial(&self) -> Option<(&wl_seat::WlSeat, u32)>; | ||
/// Surface of the window requesting the token, if applicable. | ||
/// | ||
/// *Warning*: Many compositors will issue invalid tokens for requests from | ||
/// unfocused surfaces. There is no way to detect this from the client-side. | ||
fn surface(&self) -> Option<&wl_surface::WlSurface>; |
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.
No need to use Option
for both of them, just enforce the use of what should be passed..
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.
Yes there is use in requesting a token without providing these.
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 mean, what could it be, it's all non-documented. You better use app_id
if you want to have a special logic in compositor.
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.
E.g. requesting a token without an active surface. Which might be valid for privileged clients.
Because it is all non-documented, we should actually expose everything the protocol allows instead of assuming a certain compositor policy.
src/activation.rs
Outdated
/// *Warning*: Many compositors will issue invalid tokens for requests without | ||
/// recent serials. There is no way to detect this from the client-side. | ||
fn serial(&self) -> Option<(&wl_seat::WlSeat, u32)>; |
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.
You're returning seat
and serial
from a function called serial
, sounds really odd.
I'd suggest to separate seat
and serial
. Also &Wlseat
should be better.
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's the naming the protocol uses and it expects either both or none of the above. So I can't really make two methods out of it. We could just change the name.
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 mean, whatever protocol uses is up to the protocol, we're writing a toolkit which should have saner names.
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.
this better?
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 guess... Still like separate serial
and seat
a bit more.
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.
Well, if you tell me what the implementation is supposed to do, if it gets either but not the other... just don't set it? That feels wrong, if we can enforce it on a type level.
67adadd
to
db1c2d3
Compare
src/activation.rs
Outdated
/// *Warning*: Many compositors will issue invalid tokens for requests without | ||
/// recent serials. There is no way to detect this from the client-side. | ||
fn serial(&self) -> Option<(&wl_seat::WlSeat, u32)>; |
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 mean, whatever protocol uses is up to the protocol, we're writing a toolkit which should have saner names.
5523cb2
to
ed0b5e6
Compare
ed0b5e6
to
6a1fa55
Compare
6a1fa55
to
0be6b8d
Compare
Implements xdg-activation.
The second commit adds it to the simple-window example as a test. Given how heavily commented that is, I guess that is not really a good place to put it. But without a proper window it doesn't make a whole lot of sense to use it. I could also just drop that commit?