[wayland] Fix window sizing when restoring a window that was initially fullscreen
ClosedPublic

Authored by dos on Nov 6 2018, 2:11 PM.

Details

Summary

When creating a surface and setting it as fullscreen before attaching its buffer, KWin does not know the original dimensions of the surface and tries to use an invalid value when unsetting the fullscreen flag. This patch fixes this by sending a configure with the size of 0,0 - which according to xdg-shell spec means that the client is requested to set its size by itself.

Test Plan
  1. Create a fullscreen Wayland window.
  2. Toggle fullscreen off.
  3. KWin should send a configure event with size 0,0 instead of 1,1.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
dos created this revision.Nov 6 2018, 2:11 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 6 2018, 2:11 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
dos requested review of this revision.Nov 6 2018, 2:11 PM
davidedmundson accepted this revision.Nov 6 2018, 2:40 PM
davidedmundson added a subscriber: davidedmundson.

Ideally we need to have workspace position the window once we get the new buffer and then know the size, but that's a bigger patch that can wait for another day.

This revision is now accepted and ready to land.Nov 6 2018, 2:40 PM

I would love to see a test case for it.

dos updated this revision to Diff 52182.Feb 20 2019, 11:51 PM

Added a test case.

autotests/integration/shell_client_test.cpp
568

why should it be 4?

580

I don't see why the top left should always be 0,0.

That's writing tests for what the current code happens to do, not testing for what things should be doing. Arguably the window could be placed according to the placement algorithm.

Lets just check the size.

dos updated this revision to Diff 52184.Feb 21 2019, 1:01 AM

Good points. Guess I got overly excited that I finally figured out the test infra :D Updated; also removed the unused sizeChangeRequestedSpy.

dos marked 2 inline comments as done.Feb 21 2019, 1:01 AM
davidedmundson accepted this revision.Feb 21 2019, 1:04 AM

Do you have commit access?

zzag added a subscriber: zzag.Feb 21 2019, 9:10 AM

I don't think it will work well on Unity-like setups or multiple-monitor setups.

dos added a comment.Feb 21 2019, 12:29 PM

Any idea what to use instead of QPoint(0, 0) to fix that? (I'm new to this codebase). It's already way better than current behavior, but yeah, won't hurt to make it even better ;)

zzag added a comment.Feb 21 2019, 2:53 PM
In D16710#416579, @dos wrote:

Any idea what to use instead of QPoint(0, 0) to fix that? (I'm new to this codebase). It's already way better than current behavior, but yeah, won't hurt to make it even better ;)

Well, you could get coordinates of the top-left corner of the placement area for the client, though I personally think it would be better to place the client. If I recall correctly, we do something like that on X11 when maximize restore geometry is not valid.

dos added a comment.Feb 22 2019, 1:50 AM
In D16710#416696, @zzag wrote:

Well, you could get coordinates of the top-left corner of the placement area for the client, though I personally think it would be better to place the client. If I recall correctly, we do something like that on X11 when maximize restore geometry is not valid.

Something like this?

diff --git a/shell_client.cpp b/shell_client.cpp
index 66956aed3..5e314ef22 100644
--- a/shell_client.cpp
+++ b/shell_client.cpp
@@ -986,6 +986,8 @@ void ShellClient::setFullScreen(bool set, bool user)
             // this can happen when the window was first shown already fullscreen,
             // so let the client set the size by itself
             setGeometry(QRect(QPoint(0, 0), QSize(0, 0)));
+            QRect area = workspace()->clientArea(PlacementArea, this);
+            placeIn(area);
         }
     }
     updateWindowRules(Rules::Fullscreen|Rules::Position|Rules::Size);

Those two (0,0)s above feel hacky, should I set the position to the placement area corner regardless of placing the window afterwards? Or is there a better way to send configure request with (0,0) size to the client?

zzag added a comment.EditedFeb 22 2019, 8:46 AM

That probably won't work. For now, we could just use the top-left corner of the placement area instead of QPoint(0, 0).

dos updated this revision to Diff 52297.Feb 22 2019, 1:23 PM

OK then, updated!

zzag added a comment.Feb 22 2019, 3:01 PM

Thanks, looks good to me. :-)

This revision was automatically updated to reflect the committed changes.