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
Experimental support for xdg-shell #135
Conversation
3b45e52
to
2e9df7e
Compare
linodes MIA bors try |
tryBuild failed |
linodes MIA bors try |
tryBuild failed |
linodes MIA bors try |
tryBuild failed |
bors try |
tryBuild failed |
bors try |
tryBuild failed |
bors try |
tryBuild failed |
bors try |
tryBuild succeeded |
5d31164
to
92ebc33
Compare
331142d
to
89620ac
Compare
89620ac
to
b50de25
Compare
bors try |
tryBuild failed |
bors try |
tryBuild failed |
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've a couple of questions, but I'm mostly happy
|
||
// Sometimes, when using xdg-shell, qterminal creates an insanely tall buffer | ||
if (buffer_size.height > geom::Height{10000}) | ||
buffer_size.height = geom::Height{1000}; |
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 do dislike working around client bugs silently. Would you object to printing a warning in scenarios such as 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.
Also, I happen to know that sometimes Qt creates windows with junk dimensions, but sets them only before making them visible. That might be what's happening?
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 dislike this too I just did this to prevent memory being exhausted, I've no objection to logging a warning here. (But I still hope for a better solution before enabling by default - maybe some spelunking through the Qt code will be inspiring.)
Note that Qt has submitted a buffer here, this is not simply failing to set dimensions before use.
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.
Hmm, peculiar behaviour by Qt then
|
||
mir::optional_value<geometry::Size> size; | ||
mir::optional_value<geometry::Rectangle> aux_rect; | ||
optional_value<MirPlacementGravity> surface_placement_gravity; |
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.
mir:: prefix inconsistent here.
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.
Fixing
void get_popup(uint32_t id, struct wl_resource* parent, struct wl_resource* positioner) override | ||
{ | ||
auto* tmp = wl_resource_get_user_data(positioner); | ||
auto const* const pos = static_cast<XdgPositionerV6*>(static_cast<wayland::XdgPositionerV6*>(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.
Why the 2 casts? The outer one alone should be enough
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'm sure it is always optimized out, but the standardise doesn't support your assertion.
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 did check with gcc, it was happy just using the outer. No objection though
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 can static_cast from "void*" to "anything*" without the compiler complaining - you're telling it you know what you're doing.
A cast from Base* to Derived* isn't guaranteed to be a no-op (it happens to be a no-op for single, non-virtual inheritance on every implementation I've used but I don't believe the standard requires it).
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.
Sure. I'm aware it should be a no-op. I'm just more considering the code readability. Again, no objection
{ | ||
new XdgToplevelV6{client, parent, id, shell, this}; | ||
auto* const mir_surface = get_wlsurface(surface); | ||
mir_surface->set_role(this); |
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'm finding this hard to grok. Method doesn't appear to "get" anything. is it a bad name due to the autogenerated interface that's out of our control?
Anyway this creates a XdgToplevelV6 on the heap that associates itself to the XdgSurfaceV6 as a user_data resource, adding a resize handler and the wayland resource implementation. 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.
Yes. It's all described in /usr/share/wayland-protocols/unstable/xdg-shell/xdg-shell-unstable-v6.xml
I don't like this naming, but I think this is the least confusing approach.
auto const surface = get_surface_for_id(session, surface_id); | ||
|
||
shell::SurfaceSpecification modifications; | ||
modifications.input_shape = {input_region}; |
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.
set_window_geometry maps to Mir's input shaping?
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 my reading of the documentation
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'm happy enough
bors r+
Build failed |
Get the code onto trunk, without enabling by default.
Set $MIR_EXPERIMENTAL_XDG_SHELL to enable the (currently flaky) xdg-shell support.