Integrate xdg_shell #17
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Seeing as the example works fine on weston but not on mutter, I've posted the issue to the gnome bugtracker here. One person on the wayland IRC yesterday mentioned that the problem might be due to invalid input and as a result mutter refuses to create |
|
It's difficult to say, what kind of "invalid input" would they be mentioning? |
elinorbgr
reviewed
Apr 27, 2017
examples/simple_window.rs
Outdated
| let w = std::cmp::max(width, 100); | ||
| let h = std::cmp::max(height, 100); | ||
| if width <= 0 || height <= 0 { | ||
| println!("WARNING: wayland_window::Handler::configure: Received invalid dimensions: \ |
There was a problem hiding this comment.
These are not invalid dimensions actually, they just indicate that the user doing the resize moved the cursor past the other side of the window than the side they grabbed. It's up to the client to decide how to act in this case. The most classic behavior being to ignore requests to be resized beyond a certain minimum size.
|
Regarding this PR, and the integration of First of all, thanks a lot for starting to tackle this ! 😄 Now, the way I was thinking to organize this:
|
|
Also, regarding the globals advertized:
|
|
Oh, okay, I checked the IRC logs about this input validation, and the serial things. That could have been a cause for trouble indeed... However, it looks like we are indeed sending the proper values, given your logs : here the serial (20109) is correctly the same... Could you generate a wayland trace from a simple program that resizes / moves correctly in mutter, so that we could compare ? (maybe one of the weston sample apps?) |
No problem at all! Thanks for the library and for taking your time to help debug this with me - much appreciated!
Agreed, this all sounds good to me 👍
Sure thing! Resize
The following is the output of the
This is where I press the left mouse button on the right frame in order to begin dragging it to the right: Sorry for the lengthy output! I tried to make the interaction as short as possible however wayland still likes to print a lot of info :) MoveHere's another output from
Here is where I press the left mouse button in order to begin moving the window: |
|
Thanks! As a comparison, do you have a trace of wayland-window with your patch? We need to see what differs. Looks like the xdg_shell has changed the "configure" protocol (requiring an ack apparently). Maybe something is not implemented properly on this regard. I haven't yet taken the time to familiarize myself with xdg_shell. |
|
Ah of course! Here's the full output of
Here is where I press the right edge with the left mouse button in an attempt to begin resizing it:
Yeah I noticed this too - I attempted to do this by calling |
|
I think I found the issue, but we would need a way to confirm it. In mutter, when we try doing a resize, he callback is this function: https://github.com/GNOME/mutter/blob/e6eac46629335ac341e3207df8b2aac15fb9ffa7/src/wayland/meta-wayland-xdg-shell.c#L282-L299 Given we simply have no answer from the compositor, I guess As we can see, in our context (a pointer event), the only way this can return false is if gboolean
meta_wayland_pointer_can_grab_surface (MetaWaylandPointer *pointer,
MetaWaylandSurface *surface,
uint32_t serial)
{
return (pointer->grab_serial == serial &&
pointer->focus_surface == surface);
}As the serial is good, then most likely the second equality evaluates to false. Which is plausible, as at this point, the focus surface is the subsurface we use to draw the borders, while the surface we try to grab is the parent surface. The weston client does not use subsurfaces to draw its contents, as such it does not have any problem. I don't know if it's mutter or us who are at fault here, I'll ask on #wayland IRC. |
|
Well, apparently it is mutter's fault. :) |
|
Really nice work in tracking this down! So to correct the behaviour in mutter,
Do you happen to have the log for this discussion? I'm curious about the discussion behind this, as I'm considering making the patch myself to get things moving along, however I'd like to gather what info I can first. |
|
Oh, it was a very short discussion:
This is how I understand it too. |
This commit mostly attempts to replace wl_shell with xdg_shell to make integration simpler, however the plan is to re-integrate support for wl_shell and move all xdg_shell functionality behind a (defaulted) feature gate in a future commit to allow for backward-compatibility with wl_shell. Several xdg_surface methods have not yet been exposed in the `DecoratedSurface` API. This commit also fixes a spelling mistake: substract -> subtract.
I believe this is mostly implemented however I'm not confident this is the nicest design. I'm unsure how to best modify the `Handler` pattern to handle the different shells. I've changed the Handler::configure method to take a `Configure` enum, which provides the shell-specific data. The example has not yet been updated as I'm unsure how to check whether `xdg_shell` is available and fall back to `wl_shell` otherwise. It seems this process is currently wrapped up by the `wayland_env` macro.
Bug UpdateThe move/resize bug I was experiencing should be fixed in the next gnome update! It seems that making the fix upstream fixes the problem of moving and resizing regardless of this PR. This PR@vberger I have begun making the changes you mentioned in your previous comment. I believe this is mostly implemented however I'm not confident this is The example has not yet been updated as I'm unsure how to check whether |
|
I've looked at the current state of the diff, overall it looks good 👍 Now, to answer your questions:
As xdg_shell is a global that is not expected to appear/disappear, it should be actually quite simple. Taking inspiration from this example: https://github.com/Smithay/wayland-rs/blob/master/wayland-client/examples/simple_client.rs , you don't provide XdgShell as a global to let mut xdg_shell = None;
for &(name, ref interface, version) in env.globals() {
if interface == ZxdgShellV6::interface_name() {
xdg_shell = Some(registry.bind::<ZxdgShellV6>(name, version));
/* also register xdg_shell to its handler in the events_queue */
break;
}
}Now I just realized an issue with the design: if xdg_shell is an optional feature, we have a few enums whose number of variants depends on enabled cargo features, which is not really a good thing, as cargo can enable them on the fly. In general, enabling a cargo feature whould not be a breaking change API-wise, but changing the number of variants of an enum is... Not sure what would be the best course to handle that. The good side is that since the last rust version, |
Thanks for the example, I'll have a go at implementing this now.
Sounds good to me! I'll change this too. |
Conditionally bind to the `XdgShell` or `WlShell` in the `simple_window` example.
|
Ok, I think I've addressed all your points! However, I'm unsure what you mean by this line in your example: /* also register xdg_shell to its handler in the events_queue */Could you provide an example of this? The example seems to work as is, however I could very well be missing something! |
|
Yes, sure, this is something the client will need to do with xdg_shell, (but is out of scope of wayland-window I'd say, but it's good to have it in the example). The client needs to handle this event from the server: https://smithay.github.io/wayland-rs/wayland_protocols/unstable/xdg_shell/client/zxdg_shell_v6/trait.Handler.html by simply answering "pong" to these "ping"s. Otherwise, the server may mark the client as "unresponsive". This is pretty simple: // declare a handler struct to handle the xdg_shell events:
// This one does not need any state
struct XdgShellPingHandler;
// The implementation of this handler trait is very simple
impl xdg_shell_v6::Handler for SdgShellPingHandler {
fn ping(&mut self, evqh: &mut EventQueueHandle, proxy: &ZxdgShellV6, serial: u32) {
proxy.pong(serial);
}
}
/*
* ...
*/
// later, in the match:
let shell = match (xdg_shell, wl_shell) {
(Some(shell), _) => {
// We use xdg_shell, so we'll need to answer to the pings
let ping_handler_id = event_queue.add_handler(XdgShellPingHandler);
event_queue.register::<_,XdgShellPingHandler>(&shell, ping_handler_id);
wayland_window::Shell::Xdg(shell)
},
(_, Some(shell)) => {
wayland_window::Shell::Wl(shell)
},
_ => panic!("No available shell"),
};PS: I'm currently having a look at your changes, but I wanted to answer your question first. |
elinorbgr
reviewed
Jun 18, 2017
src/decorated_surface.rs
Outdated
| } | ||
|
|
||
| /// Set a class for the surface. | ||
| /// | ||
| /// The surface class identifies the general class of applications to which the surface | ||
| /// belongs. A common convention is to use the file name (or the full path if it is a | ||
| /// non-standard location) of the application's .desktop file as the class. | ||
| /// | ||
| /// Note: This is ignored when using `xdg_shell`. Not sure if it should be handled. |
There was a problem hiding this comment.
I belive the xdg_shell equivalent would be https://smithay.github.io/wayland-rs/wayland_protocols/unstable/xdg_shell/client/zxdg_toplevel_v6/struct.ZxdgToplevelV6.html#method.set_app_id
src/decorated_surface.rs
Outdated
| pub fn set_fullscreen(&mut self, | ||
| /// | ||
| /// Note: When using `xdg_shell`, the requested `FullscreenMethod` and `framerate` will be | ||
| /// ignored. Not sure if some way of handling these arguments should be added to the xdg_shell? |
There was a problem hiding this comment.
Yup, looks like xdg_shell has no equivalent for these :/
There was a problem hiding this comment.
Given the functionality is not supported by xdg_shell and wl_shell is supposed to be deprecated anyway, we may simply remove it, and use the default values FullscreenMethod::Default for method and 0 for framerate (which together means "let the server decide", so exactly what xdg_shell does).
| width: i32, height: i32, | ||
| _states: Vec<u8>, | ||
| ) { | ||
| // NOTE: Not sure if/how `_states` should be handled here. |
There was a problem hiding this comment.
I don't know either, and the protocol documentation is not very helpful :(
src/shell/xdg.rs
Outdated
| if let Some(handler) = decorated_surface::handler_mut(self) { | ||
| let (w, h) = decorated_surface::subtract_borders(width, height); | ||
| let configure = super::Configure::Xdg; | ||
| handler.configure(evqh, configure, max(w, 1), max(h, 1)); |
There was a problem hiding this comment.
Either we do the max(w,1) and max(h,1 for both wl_shell and xdg_shell, or neither, but we can't do it for one and not the other.
I'd be in favor of not doing it, and document in the trait definition that the values can be negative if the user tries to resize the window past the left or top border, and that the client should decide how to interpret that. Giving as example simply computing max(w, min_width) and max(h, min_height).
There was a problem hiding this comment.
Ahh I must have left this in by accident, nice catch!
|
See the few comments I left, but other than that this looks really good! 👍 Thanks a lot for your effort in doing that! |
|
No problem! I've addressed each of your most recent comments, let me know if there's anything else, otherwise I think this should be good to go 👍 |
elinorbgr
requested changes
Jun 19, 2017
src/decorated_surface.rs
Outdated
| self.shell_surface.set_toplevel(); | ||
| if let shell::Surface::Wl(ref surface) = self.shell_surface { | ||
| surface.set_toplevel(); | ||
| } |
There was a problem hiding this comment.
We need to unset fullscreen mode on xdg_shell too, by calling XdgToplevelV6::unset_fullscreen()
|
Just found a last thing, but other than that, we're good! |
|
Done! |
elinorbgr
approved these changes
Jun 20, 2017
|
Awesome, thanks! I'll merge this, I have still some stuff to do before releasing the next version (mostly fixing #10 which has been open for far too long, and a few interactions with wayland-client I have in mind). Hopefully it should be a matter of days. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far this commit mostly attempts to replace
wl_shellwithxdg_shellin order to simplify integration ofxdg_shellfor myself. The plan is to re-integrate support forwl_shelland move allxdg_shellfunctionality behind a (defaulted) feature gate in a future commit to allow for backward-compatibility withwl_shell.Several xdg_surface methods have not yet been exposed in the
DecoratedSurfaceAPI including set_api_id, minimize, etc.@vberger Although this commit replaces usage of
wl_shellwithxdg_shellin the example, I'm finding that I still have the exact same problems that I had before mentioned in #16 -configureis still not being called by the mutter (v3.22.3) compositor during resizing or moving, even though it seems to work perfectly fine on weston (v2.0.0). This makes me wonder if the issue is perhaps not related to xdg_shell, and perhaps some other difference between the mutter and weston compositors? On the other hand, maybe I just haven't integratedxdg_shellproperly?I decided to run the example on both mutter and wayland to compare the output of
WAYLAND_DEBUG=1to see if I could spot any differences. I found only two:The first difference was the very start of the log where globals are registered:
weston
mutter
The main difference I noticed between these two (besides the ordering which I assume does not matter) is that weston registers a "xdg_shell", "weston_desktop_shell" and "weston_screenshooter" globals whereas mutter does not, but does register a "gtk_shell1" global. Does it seem strange to you that the mutter compositor does not register "xdg_shell" like weston does?
The second and only other difference I noticed (besides
configurenot being called during resize) was that weston used thesurface.damage_buffermethod to indicate surface damage when theThemedPointerhovered over the resizable edge, whereas mutter used thesurface.damagemethod. I found the related code here. This seems to indicate that mutter uses a pre-4 version of surface. Could this be related?