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
More Wayland core support: menus are placed correctly. #123
Conversation
o some stubs implemented; o windows are only created when we know their size.
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.
A few comments/questions inline
{ | ||
resize_handler = handler; | ||
} | ||
|
||
void set_hide_handler(std::function<void()> const& handler) | ||
void set_hide_handler(std::function<void(bool visible)> const& handler) |
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.
So this will result in hide_handler(true) actually making it visible? Confusing!
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'll rename it
mir::shell::SurfaceSpecification mods; | ||
mods.state = mir_window_state_fullscreen; | ||
if (output) | ||
if (surface_id.as_value()) |
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.
surface_id can not have a value? /me raises eyebrow.
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.
surface_id has a value of 0 until we create a window in the Mir scene, after which it identifies the window. (I'll listen to ideas on how to manage this state better.)
mir_surface->surface_id = surface_id; | ||
|
||
{ | ||
// The shell isn't guaranteed to respect the requested size |
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.
Any idea if this is a Wayland policy too?
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 direct knowledge, but I don't see how else things can be done e.g. for a tiling WM.
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.
Could you please split out the functional changes from the whitespace/namespace changes? It makes it really difficult to see what the functional change is here.
The functional changes themselves look correct, modulo my query about static_cast
.
@@ -1477,7 +1482,7 @@ class WlSeat | |||
{ | |||
auto me = reinterpret_cast<WlSeat*>(data); | |||
auto resource = wl_resource_create(client, &wl_seat_interface, | |||
std::min(version, 6u), id); | |||
std::min(version, 6u), id); |
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 doesn't seem to match our style guide, of 4-space continuations?
uint32_t /*flags*/) override | ||
{ | ||
auto const session = session_for_client(client); | ||
auto* tmp = wl_resource_get_user_data(parent); | ||
auto& parent_surface = *static_cast<WlSurface*>(tmp); |
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.
Is this safe? wl_resource_get_user_data(parent) will be a wayland::Surface* type-erased to void*. Shouldn't that be static_cast<WlSurface*>(static_castwayland::Surface*(tmp)), in order to ensure the correct offset?
uint32_t /*flags*/) override | ||
{ | ||
auto const session = session_for_client(client); | ||
auto* tmp = wl_resource_get_user_data(parent); | ||
auto& parent_surface = *static_cast<WlSurface*>(tmp); |
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.
Likewise, is this safe?
.of_type(mir_window_type_freestyle) | ||
.of_size(geom::Size{640, 480}) | ||
.with_buffer_stream(mir_surface.stream_id); | ||
auto* mir_surface = static_cast<WlSurface*>(tmp); |
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.
Hm, I know this is existing code, but is it actually safe?
wl_resource_get_user_data(surface) will return a void* pointing to a wayland::Surface; I don't think static_cast-ing directly to a WlSurface* is guaranteed to do the right thing?
Shouldn't it be static_cast<WlSurface*>(static_cast<wayland::Surface*>(tmp))
?
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 right. Fix on the way.
485d8e5
to
d716ce5
Compare
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 objections here!
Ooops, yes. |
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.
Yeah, I reviewed this a while ago but wasn't logged in. Oops.
Build succeeded |
Some stubs implemented and windows are only created when we know their size.