wayland: Add WindowExtWayland::xdg_toplevel
#4076
Conversation
|
We can not expose objects from SCTK like that. We can expose a pointer, but that's about it. |
|
Thanks for the review. I have changed the method to return |
|
You should open your changes against master. |
ultimaweapon
requested review from
jackpot51,
notgull,
daxpedda,
madsmtm and
MarijnS95
as code owners
|
Ops sorry for triggering additional review requests. I have cherry-pick my commits to |
src/platform/wayland.rs
Outdated
| pub trait WindowExtWayland {} | ||
| pub trait WindowExtWayland { | ||
| /// Returns `xdg_toplevel` of the window or null if the window is X11 window. | ||
| fn xdg_toplevel(&self) -> *mut c_void; |
There was a problem hiding this comment.
Could you return an Option<NonNull> instead?
There was a problem hiding this comment.
Okay. Will do after next week since I don't have access to my computer.
ultimaweapon
force-pushed
the
wayland
branch
from
116c68e to
0c671f2
Compare
src/platform/wayland.rs
Outdated
| let id = w.xdg_toplevel().id(); | ||
| let ptr = NonNull::new(id.as_ptr().cast()).expect("xdg_toplevel should not be null"); |
There was a problem hiding this comment.
could you move this logic to wayland/window/mod.rs and return pointer from there instead, so we don't have an export here. We could also not fail when we can not cast.
There was a problem hiding this comment.
Done.
We could also not fail when we can not
cast.
Sorry I don't understand what you mean. Do you mean we should use NonNull::new_unchecked instead of NonNull::new?
There was a problem hiding this comment.
I mean, that you just forward option up and not panic.
changelogmodule if knowledge of this change could be valuable to usersI did not update changelog yet because I don't know if this change is okay or not.
Resolves #4068.