From 5502ce9a9c0420bc4e12c4e224b926020a2e1170 Mon Sep 17 00:00:00 2001 From: Antonio Russo Date: Mon, 7 Aug 2023 07:14:18 +0000 Subject: [PATCH] placeSmart: Properly track width and height `Placements::placeSmart` searches for an optimal position for windows, attempting to minimize overlap. The core of this algorithm tracks the client's height and width in `ch` and `cw`, which have been adjusted by -1. This simplifies logic determining the bottom and right points of a window when you are starting at the top and left points. However, this decision requires adjusting that number by +1 when doing the opposite: determining the top and left points when you start with the bottom and right points. placeSmart cycles through window locations, searching for acceptable nooks and crannies to fit a window in, nicely. It begins by checking for places to put the top left corner of the window which abut another constraint. If that fails, it then tries to place the bottom right abutting a constraining feature. After finding a suitable bottom (or right) location, the top (or left) location must be determined, requiring the -1 adjustment to be undone. This patch adds that +1 back in. # The bug it solves This error can be seen by opening a bunch of windows that are placed using the "Minimal Overlapping" rule. The open space on the screen will be tiled from left to right, and then top to bottom in the windows. Once no more windows can be placed like that, the next window will be placed at the extreme bottom-right corner. However, it will be one pixel too low and one pixel too far to the right---if you try to move the window, it will "snap" to the correct spot. This single pixel may seem minor or even irrelevant, but when you use the "Present Windows" desktop effect on a multiple-monitor setup, this one pixel will cause the window to show up on both monitors. --- autotests/integration/placement_test.cpp | 26 ++++++++++++++++-------- src/placement.cpp | 14 ++++++------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/autotests/integration/placement_test.cpp b/autotests/integration/placement_test.cpp index 0746bf6b14..3dec510e8a 100644 --- a/autotests/integration/placement_test.cpp +++ b/autotests/integration/placement_test.cpp @@ -136,23 +136,33 @@ std::pair> TestPla void TestPlacement::testPlaceSmart() { + const auto outputs = workspace()->outputs(); + const QList desiredGeometries{ + QRect(0, 0, 600, 500), + QRect(600, 0, 600, 500), + QRect(0, 500, 600, 500), + QRect(600, 500, 600, 500), + QRect(680, 524, 600, 500), + QRect(680, 0, 600, 500), + QRect(0, 524, 600, 500), + QRect(0, 0, 600, 500), + }; + setPlacementPolicy(PlacementSmart); std::vector> surfaces; - QRegion usedArea; - for (int i = 0; i < 4; i++) { + for (const QRect &desiredGeometry : desiredGeometries) { auto [windowPlacement, surface] = createAndPlaceWindow(QSize(600, 500)); + surfaces.push_back(std::move(surface)); + // smart placement shouldn't define a size on windows QCOMPARE(windowPlacement.initiallyConfiguredSize, QSize(0, 0)); QCOMPARE(windowPlacement.finalGeometry.size(), QSize(600, 500)); - // exact placement isn't a defined concept that should be tested - // but the goal of smart placement is to make sure windows don't overlap until they need to - // 4 windows of 600, 500 should fit without overlap - QVERIFY(!usedArea.intersects(windowPlacement.finalGeometry.toRect())); - usedArea += windowPlacement.finalGeometry.toRect(); - surfaces.push_back(std::move(surface)); + QVERIFY(outputs[0]->geometry().contains(windowPlacement.finalGeometry.toRect())); + + QCOMPARE(windowPlacement.finalGeometry.toRect(), desiredGeometry); } } diff --git a/src/placement.cpp b/src/placement.cpp index d63096f420..17cb558de0 100644 --- a/src/placement.cpp +++ b/src/placement.cpp @@ -190,17 +190,17 @@ void Placement::placeSmart(Window *window, const QRectF &area, PlacementPolicy / y_optimal = y; // client gabarit - int ch = window->height() - 1; - int cw = window->width() - 1; + int ch = window->height(); + int cw = window->width(); bool first_pass = true; // CT lame flag. Don't like it. What else would do? // loop over possible positions do { // test if enough room in x and y directions - if (y + ch > area.bottom() && ch < area.height()) { + if (y + ch > area.y() + area.height() && ch < area.height()) { overlap = h_wrong; // this throws the algorithm to an exit - } else if (x + cw > area.right()) { + } else if (x + cw > area.x() + area.width()) { overlap = w_wrong; } else { overlap = none; // initialize @@ -257,7 +257,7 @@ void Placement::placeSmart(Window *window, const QRectF &area, PlacementPolicy / // really need to loop? test if there's any overlap if (overlap > none) { - possible = area.right(); + possible = area.x() + area.width(); if (possible - cw > x) { possible -= cw; } @@ -294,7 +294,7 @@ void Placement::placeSmart(Window *window, const QRectF &area, PlacementPolicy / // ... else ==> not enough x dimension (overlap was wrong on horizontal) else if (overlap == w_wrong) { x = area.left(); - possible = area.bottom(); + possible = area.y() + area.height(); if (possible - ch > y) { possible -= ch; @@ -325,7 +325,7 @@ void Placement::placeSmart(Window *window, const QRectF &area, PlacementPolicy / } y = possible; } - } while ((overlap != none) && (overlap != h_wrong) && (y < area.bottom())); + } while ((overlap != none) && (overlap != h_wrong) && (y < area.y() + area.height())); if (ch >= area.height()) { y_optimal = area.top();