Revert "Fix toplevel window resize pingpong (#3575)" and fix better

[AlanGriffiths]

Fixes: #3600
Fixes: #3592
Fixes: #3573

[tarek-y-ismail]

Not sure if I'm testing incorrectly (don't know how to build ubuntu-frame with a local Mir build), but running mir-kiosk, then wpe-webkit-mir-kiosk.cog still exhibits the bug (not taking up all the screen). Additionally, the old bug appears again.

[AlanGriffiths]

don't know how to build ubuntu-frame with a local Mir build

You don't need to: snap refresh --channel=edge/mir-pr3599 ubuntu-frame

mir-kiosk, then wpe-webkit-mir-kiosk.cog

Are you running the local build?

$BUILD_DIR/miral-app -kiosk -terminal wpe-webkit-mir-kiosk.cog

[tarek-y-ismail]

You don't need to: snap refresh --channel=edge/mir-pr3599 ubuntu-frame

Are you running the local build?
Indeed I am, doubled checked that I'm on the correct branch and that the build is up to date. Running the command you supplied results in the same output.

Both result in the same output (window management-wise, WPE seems a bit broken here...)

[AlanGriffiths]

Running the command you supplied results in the same output

Just double checking: you did replace $BUILD_DIR didn't you?

[Saviq]

Bug looks fixed, to me.

$ snap list frame-it ubuntu-frame  
Name          Version         Rev    Tracking       Publisher       Notes
frame-it      1.2             18     latest/stable  alangriffiths✪  classic
ubuntu-frame  198-mirv2.18.0  10866  24/edge/…      canonical✓      -

$ frame-it wpe-webkit-mir-kiosk.cog

[AlanGriffiths]

I do, however, find that kgx still isn't handled correctly:

frame-it kgx

NOT fullscreened. (Although it doesn't have rounded corners, so it "thinks" it is)

[update]

And the wayland traffic is weird...

$ WAYLAND_DISPLAY=wayland-99 WAYLAND_DEBUG=client dbus-run-session kgx 2>&1 | grep -e xdg_toplevel@.* -e xdg_surface@.* -e create_buffer.* -e zwp_linux_buffer_params_v1.*create_immed.*
[ 511800.460]  -> [email protected]_xdg_surface(new id xdg_surface@26, wl_surface@20)
[ 511800.468]  -> [email protected]_toplevel(new id xdg_toplevel@27)
[ 511800.470]  -> [email protected]_parent(nil)
[ 511800.472]  -> [email protected]_title("Console")
[ 511800.473]  -> [email protected]_app_id("org.gnome.Console")
[ 511800.568]  -> [email protected]_maximized()
[ 511800.569]  -> [email protected]_fullscreen()
[ 511808.843] [email protected]_capabilities(array[12])
[ 511808.851] [email protected](0, 0, array[0])
[ 511808.853] [email protected](15)
[ 511808.862]  -> [email protected]_configure(15)
[ 511808.864] [email protected](0, 0, array[0])
[ 511808.865] [email protected](16)
[ 511808.866]  -> [email protected]_configure(16)
[ 511808.867] [email protected](1292, 1054, array[4])
[ 511808.868] [email protected](17)
[ 511808.869]  -> [email protected]_configure(17)
[ 511808.945]  -> [email protected]_title("alan@Octopull-Framework16: ~")
[ 511813.147]  -> [email protected]_min_size(360, 294)
[ 511813.160]  -> [email protected]_max_size(0, 0)
[ 511846.658]  -> [email protected]_window_geometry(0, 0, 1292, 1054)
[ 511846.838]  -> [email protected]_immed(new id wl_buffer@32, 1292, 1054, 875713089, 0)
[ 511847.680] [email protected](0, 0, array[8])
[ 511847.683] [email protected](18)
[ 511847.685]  -> [email protected]_configure(18)
[ 511849.483]  -> [email protected]_window_geometry(0, 0, 960, 1054)
[ 511849.621]  -> [email protected]_immed(new id wl_buffer@34, 960, 1054, 875713089, 0)
[ 511850.301]  -> [email protected]_buffer(new id wl_buffer@33, 0, 24, 24, 96, 0)
[ 511851.580]  -> [email protected]_immed(new id wl_buffer@36, 960, 1054, 875713089, 0)

Oh! we're still sending a configure(0, 0, array[8]) before receiving a correctly sized buffer. (Because we got the "correct" set_window_geometry() I guess)

[tarek-y-ismail]

Bug looks fixed, to me.

Very interesting, I have the same versions for frame-it and ubuntu-frame, but the output is similar to the image I posted earlier. What could be the cause? I assume I'm the outlier here?

[AlanGriffiths]

I assume I'm the outlier here?

Yes. It is likely a timing issue, not something you are doing wrong. (The WAYLAND_DEBUG log might clarify things (see this command, but isn't essential)

Anyway, I think this approach (resetting requested_size here) is fundamentally flawed as we're potentially sending incorrect configure events as a result.

Simply reverting #3600 (to get the behaviour of 2.17) is also unattractive as that reintroduces different problems. I'm still thinking...

[tarek-y-ismail]

Yes. It is likely a timing issue

Hmm, that would mean that it should be inconsistent on both sides, right?

[AlanGriffiths]

Hmm, that would mean that it should be inconsistent on both sides, right?

No. The timing could be different between your kit and ours

[AlanGriffiths]

OK, while I rethink "fix better" I'm going to drop the first attempt. That means everyone has build of the "Revert" to experiment with.

[AlanGriffiths]

Still got to assess test results.

[AlanGriffiths]

Still got to assess test results.

OK, the reason most of the tests break is that they are sensitive to the number and content of configure events we send...

        EXPECT_CALL(*xdg_surface, configure(testing::_))
            .WillOnce(
                [&initial_configure_received, xdg_surface = static_cast<struct xdg_surface*>(*xdg_surface)](uint32_t serial)
                {
                    xdg_surface_ack_configure(xdg_surface, serial);
                    initial_configure_received = true;
                });

...this blows up with more than one configure. As most of the tests that break are not testing the number of configure events, this is wrong.

Separately, the reason we were only sending one configure was spurious:

1. BasicWindowManager::place_new_surface() defaults the size to 640x480; and,
2. WindowWlSurfaceRole::current_size() returns the size as 640x480 unless the client explicitly sets it

Because these compare equal, WaylandSurfaceObserver::content_resized_to() didn't send an initial resize reflecting the place_new_surface() default. (We should probably not default the size in place_new_surface() nor send a resize unless the WM sets it.)

The reason the remaining tests fail is that they explicitly test that the compositor does not provide a default size for windows. (Which is also wrong: this is not required by the protocol.)

(Still mulling over the best way forward)

[AlanGriffiths]

OK, as noted above, there's a bunch of weird stuff. This does not fix that, just the cased noted in the linked bugs.

I'll follow up with a PR that addresses "weird stuff", but if manual testing (yes, I know!) verifies my findings this should be good to cherry-pick into 2.18.1

[tarek-y-ismail]

Can confirm that bomber behaves well on miral-shell. And wpe-webkit-mir-kiosk.cog, kgx, and bomber are all sized correctly inside mir-kiosk.

What the last commit essentially do is "ignore the resize request if the newly requested size is the same as the last requested size, or the window size". I think we stumbled on this before but I'm not sure why we didn't pursue it?