Fix wayland window identifier#76
Conversation
A6GibKm
force-pushed
the
wayland-handle
branch
11 times, most recently
from
f3b6641 to
99fb5ba
Compare
| let backend = unsafe { | ||
| wayland_backend::sys::client::Backend::from_foreign_display(display_ptr as *mut _) | ||
| }; | ||
| let conn = wayland_client::Connection::from_backend(backend); |
There was a problem hiding this comment.
Do we need to keep the connection alive? It is dropped at the end of the scope.
There was a problem hiding this comment.
I moved the code a bit so to use at most a single connection on each time we request a handle, but idk if we can reuse them...
There was a problem hiding this comment.
I don't think we have much of an option to avoid this.
Plus the from_foreign_display will act as a guest backend, so all the handles will still belong to the same underlying display.
A6GibKm
force-pushed
the
wayland-handle
branch
5 times, most recently
from
48209c5 to
2c92ca5
Compare
| /// Both pointers have to be valid surface and display pointers. You must | ||
| /// ensure the `display_ptr` lives longer than the returned | ||
| /// `WindowIdentifier`. | ||
| pub async unsafe fn from_wayland_raw( |
There was a problem hiding this comment.
I'm actually considering whether this function is necessary. Supporting this function introduces a bunch of safety requirements and lifetime questions we need to ask about the connection. Removing this function would make ashpd less scary internally and only displace maybe 20-30 lines of code to the user.
I think what we can do is explain where the user would create the required types from the surface and display pointers (wayland-backend) and then explain what lifetime constraints apply to such handles WaylandWindowIdentifier has (must not outlive the connection).
This would also mean the client_system feature would not be needed as ashpd would not be responsible for that and would operate on wayland-backend common types or wayland-client types.
There was a problem hiding this comment.
Actually no we have to have this, RawWindowHandle forces our hand here
There was a problem hiding this comment.
Actually no we have to have this, RawWindowHandle forces our hand here
Yes :(
| pub struct WaylandWindowIdentifier { | ||
| exported: ZxdgExportedV2, | ||
| exported_v1: Option<ZxdgExportedV1>, | ||
| exported_v2: Option<ZxdgExportedV2>, | ||
| type_: WindowIdentifierType, | ||
| } |
There was a problem hiding this comment.
Do we need a Drop implementation to destroy the inner exported handles?
Do note that destroying exported handles will revoke any imports performed on other clients and the handle string will become inert.
There was a problem hiding this comment.
So how it should be handled? Should we expose a method for the user to drop the handle?
There was a problem hiding this comment.
gtk3 and gtk4 appear to destroy their exported handles. So yes Drop should destroy the exported protocol object.
There was a problem hiding this comment.
Yes, I wanted to be consistent with gtk's behaviour. Although, the issue with gtk is it errors out when you try to re-export a handle for an already exported surface instead of returning the same handle. Which make the WindowIdentifier not very nice with gtk, that is why I made it a kind of reference counted from ashpd's side and only unexport when the last usage is dropped.
I am thinking of going directly through the wayland-rs crates in the future and avoid using gtk's provided methods.
| pub struct WaylandWindowIdentifier { | ||
| exported: ZxdgExportedV2, | ||
| exported_v1: Option<ZxdgExportedV1>, | ||
| exported_v2: Option<ZxdgExportedV2>, |
There was a problem hiding this comment.
We could use an Inner enum where v1 and v2 are variants go make the implementation a bit more idomatic.
A6GibKm
force-pushed
the
wayland-handle
branch
3 times, most recently
from
2093487 to
0a1d367
Compare
|
@i509VCB Do you think this one is ready to be merged for now? we can update later once a new stable release is out. |
A6GibKm
force-pushed
the
wayland-handle
branch
2 times, most recently
from
46e80e4 to
d0aaa90
Compare
A6GibKm
changed the title
|
I will go ahead and land this in order to fix the CI, we can do further changes on another PR if needed. |
4 participants
No description provided.