Differential D15599 Diff 41997 containmentactions/switchdesktop/desktop.cpp

Changeset View

Standalone View

View Options

containmentactions/switchdesktop/desktop.cpp

1/* 1​/*
2 * Copyright 2009 by Chani Armitage <chani@kde.org> 2​ * Copyright 2009 by Chani Armitage <chani@kde.org>
3​ * Copyright 2018 by Eike Hein <hein@kde.org>
3 * 4​ *
4 * This program is free software; you can redistribute it and/or modify 5​ * This program is free software; you can redistribute it and/or modify
5 * it under the terms of the GNU Library General Public License as 6​ * it under the terms of the GNU Library General Public License as
6 * published by the Free Software Foundation; either version 2, or 7​ * published by the Free Software Foundation; either version 2, or
7 * (at your option) any later version. 8​ * (at your option) any later version.
8 * 9​ *
9 * This program is distributed in the hope that it will be useful, 10​ * This program is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 11​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 12​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details 13​ * GNU General Public License for more details
13 * 14​ *
14 * You should have received a copy of the GNU Library General Public 15​ * You should have received a copy of the GNU Library General Public
15 * License along with this program; if not, write to the 16​ * License along with this program; if not, write to the
16 * Free Software Foundation, Inc., 17​ * Free Software Foundation, Inc.,
17 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. 18​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
18 */ 19​ */
19 20​
20#include "desktop.h" 21​#include "desktop.h"
21 22​
23​#include <virtualdesktopinfo.h>
24​
22#include <QAction> 25​#include <QAction>
23#include <QWheelEvent> ​
24 26​
25#include <QDebug> 27​using namespace TaskManager;
26#include <KWindowSystem> 28​
27#include <KLocalizedString> 29​VirtualDesktopInfo *SwitchDesktop::s_virtualDesktopInfo = nullptr;
30​int SwitchDesktop::s_instanceCount = 0;
28 31​
29SwitchDesktop::SwitchDesktop(QObject *parent, const QVariantList &args) 32​SwitchDesktop::SwitchDesktop(QObject *parent, const QVariantList &args)
30 : Plasma::ContainmentActions(parent, args) 33​ : Plasma::ContainmentActions(parent, args)
31{ 34​{
35​ ++s_instanceCount;
36​
37​ if (!s_virtualDesktopInfo) {
38​ s_virtualDesktopInfo = new VirtualDesktopInfo();
39​ }
32} 40​}
33 41​
34SwitchDesktop::~SwitchDesktop() 42​SwitchDesktop::~SwitchDesktop()
35{ 43​{
44​ --s_instanceCount;
45​
46​ if (!s_instanceCount) {
broulikUnsubmitted
Not Done

!s_instanceCount

broulik: `!s_instanceCount`
davidedmundsonUnsubmitted
Not Done

In the other patch VirtualDesktopInfo has a shared d ptr across all instances.

There's no point making VirtualDesktopInfo shared when it internally does it itself anyway. You save practically nothing.

davidedmundson: In the other patch VirtualDesktopInfo has a shared d ptr across all instances. There's no…
davidedmundsonUnsubmitted
Not Done

This comment wasn't addressed.

It goes for the other containment actions patch too.

davidedmundson: This comment wasn't addressed. It goes for the other containment actions patch too.
47​ delete s_virtualDesktopInfo;
48​ s_virtualDesktopInfo = nullptr;
49​ }
50​
36 qDeleteAll(m_actions); 51​ qDeleteAll(m_actions);
37} 52​}
38 53​
39 54​
40QList<QAction*> SwitchDesktop::contextualActions() 55​QList<QAction*> SwitchDesktop::contextualActions()
41{ 56​{
42 QList<QAction*> list; 57​ const int numDesktops = s_virtualDesktopInfo->numberOfDesktops();
58​ const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
59​ const QStringList &desktopNames = s_virtualDesktopInfo->desktopNames();
davidedmundsonUnsubmitted
Not Done

Use of the & is weird.

davidedmundson: Use of the & is weird.
heinAuthorUnsubmitted
Not Done

I know the containers are implicitly shared, but I consider it better coding style to do the correct C++ thing out of habit rather than rely on that.

hein: I know the containers are implicitly shared, but I consider it better coding style to do the…
60​ const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
43 61​
44 const int numDesktops = KWindowSystem::numberOfDesktops(); 62​ QList<QAction*> actions;
45 const int currentDesktop = KWindowSystem::currentDesktop(); 63​ actions.reserve(numDesktops);
46 64​
47 //Is it either the first time or the desktop number changed? ​
48 if (m_actions.count() < numDesktops) { 65​ if (m_actions.count() < numDesktops) {
broulikUnsubmitted
Not Done

This codes doesn't take into account when the virtual desktop names change etc. I think it should re-create the actions every time, and not try to be "smart" by only changing the ones that are "superfluous" or "missing" as the names and order inbetween could have changed

broulik: This codes doesn't take into account when the virtual desktop names change etc. I think it…
heinAuthorUnsubmitted
Not Done

You misread the diff. This was indeed a bug the old code has, but the new code sets the text and data on all actions every time (below).

hein: You misread the diff. This was indeed a bug the old code has, but the new code sets the text…
49 for (int i = m_actions.count() + 1; i <= numDesktops; ++i) { 66​ for (int i = m_actions.count() + 1; i <= numDesktops; ++i) {
50 QString name = KWindowSystem::desktopName(i); 67​ QString name = desktopNames.at(i);
51 QAction *action = new QAction(QStringLiteral("%1: %2").arg(i).arg(name), this); 68​ QAction *action = new QAction(this);
52 connect(action, &QAction::triggered, this, &SwitchDesktop::switchTo); 69​ connect(action, &QAction::triggered, this, &SwitchDesktop::switchTo);
53 action->setData(i); ​
54 m_actions[i] = action; 70​ m_actions[i] = action;
55 } 71​ }
56 72​
57 } else if (m_actions.count() > numDesktops) { 73​ } else if (m_actions.count() > numDesktops) {
58 for (int i = numDesktops +1; i <= m_actions.count(); ++i) { 74​ for (int i = numDesktops +1; i <= m_actions.count(); ++i) {
59 delete m_actions[i]; 75​ delete m_actions[i];
60 m_actions.remove(i); 76​ m_actions.remove(i);
zzagUnsubmitted
Not Done

Will this loop ever be executed?

Also, why not delete m_actions.take(i - 1);?

zzag: Will this loop ever be executed? Also, why not `delete m_actions.take(i - 1);`?
zzagUnsubmitted
Not Done

Could someone please explain why this part is fine?

zzag: Could someone please explain why this part is fine?
zzagUnsubmitted
Not Done

I didn't test it, but I think this one would be better:

qDeleteAll(m_actions.begin() + numDesktops, m_actions.end());
m_actions.resize(numDesktops);
zzag: I didn't test it, but I think this one would be better: ```lang=cpp qDeleteAll(m_actions.begin…
davidedmundsonUnsubmitted
Not Done

There's a bigger question.

What if you have 5 desktops, and you delete desktop 2?

We don't want the action for desktop 5 to be the one that's removed.

This either needs better change tracking or to just rebuild the list every time.

davidedmundson: There's a bigger question. What if you have 5 desktops, and you delete desktop 2? We don't…
zzagUnsubmitted
Not Done

It should probably be ">".

If i == numDesktops, we delete numDesktops - 1, which we then access down below.

zzag: It should probably be ">". If i == numDesktops, we delete numDesktops - 1, which we then…
61 } 77​ }
62 } 78​ }
63 79​
64 for (int i = 1; i <= numDesktops; ++i) { 80​ for (int i = 1; i <= numDesktops; ++i) {
65 QAction *action = m_actions.value(i); 81​ QAction *action = m_actions.value(i);
66 action->setEnabled(i != currentDesktop); 82​ action->setText(QStringLiteral("%1: %2").arg(i).arg(desktopNames.at(i)));
67 list << action; 83​ action->setData(desktopIds.at(i));
84​ action->setEnabled(desktopIds.at(i) != currentDesktop);
85​ actions << action;
68 } 86​ }
69 87​
70 return list; 88​ return actions;
71} 89​}
72 90​
73void SwitchDesktop::switchTo() 91​void SwitchDesktop::switchTo()
74{ 92​{
75 QAction *action = qobject_cast<QAction *>(sender()); 93​ const QAction *action = qobject_cast<QAction *>(sender());
94​
76 if (!action) { 95​ if (!action) {
77 return; 96​ return;
78 } 97​ }
79 98​
80 const int desktop = action->data().toInt(); 99​ s_virtualDesktopInfo->requestActivate(action->data());
81 KWindowSystem::setCurrentDesktop(desktop); ​
82} 100​}
83 101​
84void SwitchDesktop::performNextAction() 102​void SwitchDesktop::performNextAction()
85{ 103​{
86 const int numDesktops = KWindowSystem::numberOfDesktops(); 104​ const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
87 const int currentDesktop = KWindowSystem::currentDesktop(); 105​ const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
88 KWindowSystem::setCurrentDesktop(currentDesktop % numDesktops + 1); 106​ const int currentDesktopIndex = desktopIds.indexOf(currentDesktop);
107​
108​ const int nextDesktopIndex = (currentDesktopIndex % desktopIds.count() + 1);
davidedmundsonUnsubmitted
Not Done

I /think/ this is wrong. Not 100% sure.

We used to want a number wrapped between 1 and numDesktops.

The old
KWindowSystem::setCurrentDesktop(currentDesktop % numDesktops + 1);

is probably more understandable as
KWindowSystem::setCurrentDesktop((currentDesktop +1 ) -1 % numDesktops + 1);

In the new code as we have the IDs in a lookup table so we want our final number to be between 0 and numDesktops-1

Therefore should this be
currentDesktopIndex +1 % desktopIds.count();

?

(same for perform previous)

davidedmundson: I /think/ this is wrong. Not 100% sure. We used to want a number wrapped between 1 and…
109​
110​ s_virtualDesktopInfo->requestActivate(desktopIds.at(nextDesktopIndex));
89} 111​}
90 112​
91void SwitchDesktop::performPreviousAction() 113​void SwitchDesktop::performPreviousAction()
92{ 114​{
93 const int numDesktops = KWindowSystem::numberOfDesktops(); 115​ const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
94 const int currentDesktop = KWindowSystem::currentDesktop(); 116​ const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
95 KWindowSystem::setCurrentDesktop((numDesktops + currentDesktop - 2) % numDesktops + 1); 117​ const int currentDesktopIndex = desktopIds.indexOf(currentDesktop);
118​
119​ const int previousDesktopIndex = ((desktopIds.count() + currentDesktopIndex - 2) % desktopIds.count() + 1);
120​
121​ s_virtualDesktopInfo->requestActivate(desktopIds.at(previousDesktopIndex));
96} 122​}
97 123​
98K_EXPORT_PLASMA_CONTAINMENTACTIONS_WITH_JSON(switchdesktop, SwitchDesktop, "plasma-containmentactions-switchdesktop.json") 124​K_EXPORT_PLASMA_CONTAINMENTACTIONS_WITH_JSON(switchdesktop, SwitchDesktop, "plasma-containmentactions-switchdesktop.json")
99 125​
100#include "desktop.moc" 126​#include "desktop.moc"