Test XDG shell (stable) version 5 #277
Conversation
| } | ||
| catch (wlcs::ProtocolError const& err) | ||
| { | ||
| return; |
There was a problem hiding this comment.
This should be testing that we're sending the correct protocol error; it looks like that should be xdg_toplevel::invalid_parent.
There was a problem hiding this comment.
Only if the protocol version is new enough to support xdg_toplevel::invalid_parent. We could test this, but I really don't see why it matters.
There was a problem hiding this comment.
Technically this shouldn't be a protocol error until xdg-shell v5. A truly pedantic test would bind to xdg-shell < v5, set up a parent loop, and assert no protocol error. 😉
It's not a big thing (because protocol errors can't be used for error recovery), but it can be helpful for client developers to send the specified error. As an aspirational conformance suite, we should be checking adherence to the protocol; that specifies a specific error, it's cheap to check it, we should.
There was a problem hiding this comment.
(I don't expect you to write the “parent loops are fine on <v5” test, but we should gate this test on ≥v5, and test the correct error is sent)
| @@ -523,6 +560,29 @@ class LayerV1PopupManager : public XdgPopupManagerBase | |||
| int popup_surface_configure_count{0}; | |||
| }; | |||
|
|
|||
| auto surface_actually_at_location( | |||
There was a problem hiding this comment.
This is pragmatic, but I think we're approaching the point where we might want to develop some more WLCS/compositor hooks to do these sorts of things more explicitly. Not blocking this, just flagging.
| std::make_pair(manager.state.value().x, manager.state.value().y), | ||
| Eq(std::make_pair(-popup_width, -popup_height))) << "popup initially placed in incorrect position"; | ||
|
|
||
| manager.move_parent_using_pointer({5, 5}); |
There was a problem hiding this comment.
Why are we moving using the pointer, rather than server.move_surface_to?
There was a problem hiding this comment.
Because in Mir, server.move_surface_to doesn't generate a window manager request.
There was a problem hiding this comment.
More seriously, ok. Add it to the pile of things tracked by #278.
| { | ||
| XdgPopupStableManager manager{this}; | ||
|
|
||
| manager.client.bind_if_supported<xdg_wm_base>(wlcs::AtLeastVersion{XDG_POSITIONER_SET_REACTIVE_SINCE_VERSION}); |
There was a problem hiding this comment.
We should probably have a cleaner way to do this; again, not blocking.
tests/xdg_toplevel_stable.cpp
Outdated
| wlcs::XdgToplevelStable toplevel{xdg_shell_surface}; | ||
| EXPECT_CALL(toplevel, wm_capabilities).Times(1); | ||
| client.roundtrip(); | ||
| surface.attach_buffer(100, 100); |
There was a problem hiding this comment.
Why do we need to attach a buffer here? As far as I can tell the relevant xdg_surface events should be sent before the first buffer is attached and the surface is mapped, and if this attach is needed then isn't this test racy, as attach_buffer doesn't wait for any server response?
2862: Detect cyclic parent-child relationships r=AlanGriffiths a=wmww Required to pass the new WLCS tests added by canonical/wlcs#277 Co-authored-by: Sophie Winter <git@phie.me>
|
Looks good; I still have this question, and we should gate the |
Why the silence? bors r=RAOF |
|
bors cancel |
|
Canceled. |
|
bors r=RAOF |
|
Build succeeded: |
No description provided.