Differential D21996 Diff 61530 autotests/integration/placement.cpp

Changeset View

Standalone View

View Options

autotests/integration/placement.cpp

  • This file was added.
1​/********************************************************************
2​KWin - the KDE window manager
3​This file is part of the KDE project.
4​
5​Copyright (C) 2019 David Edmundson <davidedmundson@kde.org>
6​
7​This program is free software; you can redistribute it and/or modify
8​it under the terms of the GNU General Public License as published by
9​the Free Software Foundation; either version 2 of the License, or
10​(at your option) any later version.
11​
12​This program is distributed in the hope that it will be useful,
13​but WITHOUT ANY WARRANTY; without even the implied warranty of
14​MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15​GNU General Public License for more details.
16​
17​You should have received a copy of the GNU General Public License
18​along with this program. If not, see <http://www.gnu.org/licenses/>.
19​*********************************************************************/
20​#include "cursor.h"
21​#include "kwin_wayland_test.h"
22​#include "platform.h"
23​#include "screens.h"
24​#include "shell_client.h"
25​#include "wayland_server.h"
zzagUnsubmitted
Done

Style: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Includes (needs to be sorted)

zzag: Style: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Includes (needs to be sorted)
26​#include "workspace.h"
27​
28​
29​#include <KWayland/Client/compositor.h>
30​#include <KWayland/Client/plasmashell.h>
31​#include <KWayland/Client/server_decoration.h>
32​#include <KWayland/Client/shell.h>
33​#include <KWayland/Client/shm_pool.h>
34​#include <KWayland/Client/surface.h>
35​#include <KWayland/Client/xdgdecoration.h>
36​#include <KWayland/Client/xdgshell.h>
37​
zzagUnsubmitted
Not Done

You forgot to sort these includes.

zzag: You forgot to sort these includes.
zzagUnsubmitted
Not Done

Argh, please ignore it. It's sort of embarrassing..

zzag: Argh, please ignore it. It's sort of embarrassing..
38​using namespace KWin;
39​using namespace KWayland::Client;
40​
41​static const QString s_socketName = QStringLiteral("wayland_test_kwin_placement-0");
42​
43​struct PlaceWindowResult
zzagUnsubmitted
Done

Style: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces

zzag: Style: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces
44​{
45​ QSize initiallyConfiguredSize;
46​ KWayland::Client::XdgShellSurface::States initiallyConfiguredStates;
47​ QRect finalGeometry;
48​};
49​
50​class TestPlacement : public QObject
51​{
52​ Q_OBJECT
53​private Q_SLOTS:
54​ void init();
55​ void cleanup();
56​ void initTestCase();
57​
58​ void testPlaceSmart();
59​ void testPlaceZeroCornered();
60​ void testMaximize();
61​
62​private:
63​ void setPlacementPolicy(Placement::Policy policy);
64​ /*
65​ * Create a window with the lifespan of parent and return relevant results for testing
66​ * defaultSize is the buffer size to use if the compositor returns an empty size in the first configure
67​ * event.
zzagUnsubmitted
Not Done

These two can be ordinary functions. Also, "doxygen" comments have to terminate with **/

zzag: These two can be ordinary functions. Also, "doxygen" comments have to terminate with `**/`
davidedmundsonAuthorUnsubmitted
Not Done

Why would someone be generating doxygen of an internal class of an autotest?

Yes they could be functions, but there's a semantic link and we're not making a public API or anything

davidedmundson: Why would someone be generating doxygen of an internal class of an autotest? Yes they could be…
zzagUnsubmitted
Not Done

Why would someone be generating doxygen of an internal class of an autotest?

Perhaps I should have asked you this question. Initially, this method had a legit Doxygen comment with "wrong" terminator.

zzag: > Why would someone be generating doxygen of an internal class of an autotest? Perhaps I…
davidedmundsonAuthorUnsubmitted
Not Done

Yeah, I had it because I wanted to indicate the comment was describing what the function does, not commenting the code.

But then as that went into a pointless discussion about doxygen, I didn't bother.

davidedmundson: Yeah, I had it because I wanted to indicate the comment was describing what the function does…
68​ */
69​ PlaceWindowResult createAndPlaceWindow(const QSize &defaultSize, QObject *parent);
70​};
71​
72​void TestPlacement::init()
73​{
74​ QVERIFY(Test::setupWaylandConnection(Test::AdditionalWaylandInterface::XdgDecoration |
75​ Test::AdditionalWaylandInterface::PlasmaShell));
76​
77​ screens()->setCurrent(0);
78​ KWin::Cursor::setPos(QPoint(512, 512));
79​}
80​
81​void TestPlacement::cleanup()
82​{
83​ Test::destroyWaylandConnection();
84​}
85​
86​void TestPlacement::initTestCase()
87​{
88​ qRegisterMetaType<KWin::ShellClient*>();
89​ qRegisterMetaType<KWin::AbstractClient*>();
90​ QSignalSpy workspaceCreatedSpy(kwinApp(), &Application::workspaceCreated);
91​ QVERIFY(workspaceCreatedSpy.isValid());
92​ kwinApp()->platform()->setInitialWindowSize(QSize(1280, 1024));
93​ QMetaObject::invokeMethod(kwinApp()->platform(), "setVirtualOutputs", Qt::DirectConnection, Q_ARG(int, 2));
94​ QVERIFY(waylandServer()->init(s_socketName.toLocal8Bit()));
95​
96​ kwinApp()->setConfig(KSharedConfig::openConfig(QString(), KConfig::SimpleConfig));
97​
98​ kwinApp()->start();
99​ QVERIFY(workspaceCreatedSpy.wait());
100​ QCOMPARE(screens()->count(), 2);
101​ QCOMPARE(screens()->geometry(0), QRect(0, 0, 1280, 1024));
102​ QCOMPARE(screens()->geometry(1), QRect(1280, 0, 1280, 1024));
103​ waylandServer()->initWorkspace();
104​}
105​
106​void TestPlacement::setPlacementPolicy(Placement::Policy policy)
107​{
108​ auto group = kwinApp()->config()->group("Windows");
109​ group.writeEntry("Placement", Placement::policyToString(policy));
zzagUnsubmitted
Not Done

I personally see nothing wrong with duplicating this piece of code in each test.

zzag: I personally see nothing wrong with duplicating this piece of code in each test.
davidedmundsonAuthorUnsubmitted
Not Done

Till we change something...

davidedmundson: Till we change something...
110​ group.sync();
111​ Workspace::self()->slotReconfigure();
112​}
113​
114​
115​PlaceWindowResult TestPlacement::createAndPlaceWindow(const QSize &defaultSize, QObject *parent)
116​{
117​ PlaceWindowResult rc;
118​
119​ // create a new window
120​ auto surface = Test::createSurface(parent);
121​ auto shellSurface = Test::createXdgShellStableSurface(surface, surface, Test::CreationSetup::CreateOnly);
122​ QSignalSpy configSpy(shellSurface, &XdgShellSurface::configureRequested);
zzagUnsubmitted
Not Done

Missing QVERIFY.

zzag: Missing QVERIFY.
davidedmundsonAuthorUnsubmitted
Not Done

You can't QVERIFY outside of a test function. It has an empty return.

If this was failing the shellClient test would be exploding, I think most people know to fix that first, which can leave this test to focus on placement.

davidedmundson: You can't QVERIFY outside of a test function. It has an empty return. If this was failing the…
zzagUnsubmitted
Not Done

Oops, sorry.

zzag: Oops, sorry.
123​ surface->commit(Surface::CommitFlag::None);
124​ configSpy.wait();
125​
126​ rc.initiallyConfiguredSize = configSpy[0][0].toSize();
127​ rc.initiallyConfiguredStates = configSpy[0][1].value<KWayland::Client::XdgShellSurface::States>();
128​ shellSurface->ackConfigure(configSpy[0][2].toUInt());
129​
130​ QSize size = rc.initiallyConfiguredSize;
131​
132​ if (size.isEmpty()) {
133​ size = defaultSize;
134​ }
135​
136​ auto c = Test::renderAndWaitForShown(surface, size, Qt::red);
137​
138​ rc.finalGeometry = c->geometry();
139​ return rc;
140​}
141​
142​void TestPlacement::testPlaceSmart()
143​{
144​ setPlacementPolicy(Placement::Smart);
zzagUnsubmitted
Not Done

Is it correct? It's null.

zzag: Is it correct? It's null.
davidedmundsonAuthorUnsubmitted
Not Done

It was not. Thanks.

davidedmundson: It was not. Thanks.
145​
146​ QScopedPointer<QObject> testParent(new QObject); //dumb QObject just for scoping surfaces to the test
147​
148​ QRegion usedArea;
149​
150​ for (int i = 0; i < 4; i++) {
151​ PlaceWindowResult windowPlacement = createAndPlaceWindow(QSize(600, 500), testParent.data());
152​ // smart placement shouldn't define a size on clients
zzagUnsubmitted
Not Done

Name of this variable is misleading. It leaves impression that this is a ShellClient. Perhaps don't use auto?

zzag: Name of this variable is misleading. It leaves impression that this is a ShellClient. Perhaps…
davidedmundsonAuthorUnsubmitted
Not Done

Done, but we need to take this "auto" discussion to frameworks-devel and make an actual policy.

davidedmundson: Done, but we need to take this "auto" discussion to frameworks-devel and make an actual policy.
zzagUnsubmitted
Not Done

A bit radical claim: I suggest to prohibit auto in kwin because many people abuse it. We could use it with only a few things, like iterators.

auto foo = bar();

is just wrong. I know that I did this too, but it's just wrong.

Anyways, that's out of scope of this patch.

zzag: A bit radical claim: I suggest to prohibit `auto` in kwin because many people abuse it. We…
153​ QCOMPARE(windowPlacement.initiallyConfiguredSize, QSize(0, 0));
154​ QCOMPARE(windowPlacement.finalGeometry.size(), QSize(600, 500));
155​
156​ // exact placement isn't a defined concept that should be tested
157​ // but the goal of smart placement is to make sure windows don't overlap until they need to
158​ // 4 windows of 600, 500 should fit without overlap
159​ QVERIFY(!usedArea.intersects(windowPlacement.finalGeometry));
160​ usedArea += windowPlacement.finalGeometry;
161​ }
162​}
163​
164​void TestPlacement::testPlaceZeroCornered()
zzagUnsubmitted
Done

A better name for the test: testPlaceZeroCornered.

zzag: A better name for the test: testPlaceZeroCornered.
165​{
166​ setPlacementPolicy(Placement::ZeroCornered);
167​
168​ QScopedPointer<QObject> testParent(new QObject);
169​
170​ for (int i = 0; i < 4; i++) {
171​ PlaceWindowResult windowPlacement = createAndPlaceWindow(QSize(600, 500), testParent.data());
172​ // smart placement shouldn't define a size on clients
173​ QCOMPARE(windowPlacement.initiallyConfiguredSize, QSize(0, 0));
174​ // size should match our buffer
175​ QCOMPARE(windowPlacement.finalGeometry.size(), QSize(600, 500));
176​ //and it should be in the corner
177​ QCOMPARE(windowPlacement.finalGeometry.topLeft(), QPoint(0, 0));
178​ }
179​}
180​
181​void TestPlacement::testMaximize()
182​{
183​ setPlacementPolicy(Placement::Maximizing);
184​
185​ // add a top panel
186​ QScopedPointer<Surface> panelSurface(Test::createSurface());
187​ QScopedPointer<QObject> panelShellSurface(Test::createXdgShellStableSurface(panelSurface.data()));
188​ QScopedPointer<PlasmaShellSurface> plasmaSurface(Test::waylandPlasmaShell()->createSurface(panelSurface.data()));
189​ plasmaSurface->setRole(PlasmaShellSurface::Role::Panel);
190​ plasmaSurface->setPosition(QPoint(0, 0));
191​ Test::renderAndWaitForShown(panelSurface.data(), QSize(1280, 20), Qt::blue);
192​
193​ QScopedPointer<QObject> testParent(new QObject);
194​
195​ // all windows should be initially maximized with an initial configure size sent
196​ for (int i = 0; i < 4; i++) {
197​ PlaceWindowResult windowPlacement = createAndPlaceWindow(QSize(600, 500), testParent.data());
198​ QVERIFY(windowPlacement.initiallyConfiguredStates & XdgShellSurface::State::Maximized);
199​ QCOMPARE(windowPlacement.initiallyConfiguredSize, QSize(1280, 1024 - 20));
200​ QCOMPARE(windowPlacement.finalGeometry, QRect(0, 20, 1280, 1024 - 20)); // under the panel
201​ }
202​}
203​
204​WAYLANDTEST_MAIN(TestPlacement)
205​#include "placement.moc"