(#332) LayerSurfaceLayoutTest includes a test for a -1 exclusion zone #333
Conversation
There was a problem hiding this comment.
I'm a bit confused at what this test is doing, and I think it's because the test isn't testing what you think it is.
I think it would be more obvious what it's doing if either:
- You set up the exclusive surfaces first, then set up the
exclusive=-1surface and checked its position, or - You set up the
exclusive=-1surface, checked its position, then set up the exclusive surfaces and asserted that no reconfiguration occured.
(It's possible that this is more usefully two tests)
tests/wlr_layer_shell_v1.cpp
Outdated
| } | ||
|
|
||
| auto const expected_placement = layout.placement_rect(non_exlusive_zone); | ||
| surface.attach_visible_buffer(expected_placement.size.width.as_int(), expected_placement.size.height.as_int()); |
There was a problem hiding this comment.
Why are we attaching a new buffer to the surface? Why are we attaching a buffer with the same size as the previous buffer, but where that size has a different name?
There was a problem hiding this comment.
Additionally, you're never committing this buffer, so it's not going to do anything?
tests/wlr_layer_shell_v1.cpp
Outdated
| EXPECT_THAT(layer_surface.last_size().height, Eq(expected_config_size.height)); | ||
| } | ||
|
|
||
| auto const expected_placement = layout.placement_rect(non_exlusive_zone); |
There was a problem hiding this comment.
Why are we naming this differently? expected_placement == initial_rect
I am attempting to test that ignoring exclusive zones while you are a fullscreened surface gives you the whole output, regardless of there being attached surfaces with exclusive zones defined.
These two tests sound like a better idea tho |
tests/wlr_layer_shell_v1.cpp
Outdated
| auto const expected_placement = layout.placement_rect(output_rect()); | ||
| surface.attach_visible_buffer(expected_placement.size.width.as_int(), expected_placement.size.height.as_int()); |
There was a problem hiding this comment.
Why are we attaching a new buffer with the same size (but with a different variable name for that size) here?
tests/wlr_layer_shell_v1.cpp
Outdated
| if (request_size.width.as_int()) | ||
| EXPECT_THAT(layer_surface.last_size().width, Eq(request_size.width)); | ||
| if (request_size.height.as_int()) | ||
| EXPECT_THAT(layer_surface.last_size().height, Eq(request_size.height)); |
There was a problem hiding this comment.
I'm pretty sure this isn't testing what you expect it to be testing? Particularly: we've attached a buffer of size initial_rect.size. That means that the surface has size initial_rect.size; the client buffer size is authoritative.
What this is testing is that the WM has suggested (in the configure event) that we make the surface size request_size.size.
fixes #332
Implemented a test for
zwlr_layer_surface_v1_set_exclusive_zone(-1), which means that the surface should ignore all exclusion zones (e.g. a background)