Differential D15599 Diff 41931 containmentactions/switchdesktop/desktop.cpp

Changeset View

Standalone View

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>
26	#include <KWindowSystem>
27	#include <KLocalizedString>	27		#include <KLocalizedString>
28		28
		29		using namespace TaskManager;
		30
		31		VirtualDesktopInfo *SwitchDesktop::s_virtualDesktopInfo = nullptr;
		32		int SwitchDesktop::s_instanceCount = 0;
		33
29	SwitchDesktop::SwitchDesktop(QObject *parent, const QVariantList &args)	34		SwitchDesktop::SwitchDesktop(QObject *parent, const QVariantList &args)
30	: Plasma::ContainmentActions(parent, args)	35		: Plasma::ContainmentActions(parent, args)
31	{	36		{
		37		++s_instanceCount;
		38
		39		if (!s_virtualDesktopInfo) {
		40		s_virtualDesktopInfo = new VirtualDesktopInfo();
		41		}
32	}	42		}
33		43
34	SwitchDesktop::~SwitchDesktop()	44		SwitchDesktop::~SwitchDesktop()
35	{	45		{
		46		--s_instanceCount;
		47
		48		if (!s_virtualDesktopInfo) {
			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.
		49		delete s_virtualDesktopInfo;
		50		s_virtualDesktopInfo = nullptr;
		51		}
		52
36	qDeleteAll(m_actions);	53		qDeleteAll(m_actions);
37	}	54		}
38		55
39		56
40	QList<QAction*> SwitchDesktop::contextualActions()	57		QList<QAction*> SwitchDesktop::contextualActions()
41	{	58		{
42	QList<QAction*> list;	59		const int numDesktops = s_virtualDesktopInfo->numberOfDesktops();
		60		const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
		61		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…
		62		const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
43		63
44	const int numDesktops = KWindowSystem::numberOfDesktops();	64		QList<QAction*> actions;
45	const int currentDesktop = KWindowSystem::currentDesktop();	65		actions.reserve(numDesktops);
46		66
47	//Is it either the first time or the desktop number changed?
48	if (m_actions.count() < numDesktops) {	67		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) {	68		for (int i = m_actions.count() + 1; i <= numDesktops; ++i) {
50	QString name = KWindowSystem::desktopName(i);	69		QString name = desktopNames.at(i);
51	QAction *action = new QAction(QStringLiteral("%1: %2").arg(i).arg(name), this);	70		QAction *action = new QAction(this);
52	connect(action, &QAction::triggered, this, &SwitchDesktop::switchTo);	71		connect(action, &QAction::triggered, this, &SwitchDesktop::switchTo);
53	action->setData(i);
54	m_actions[i] = action;	72		m_actions[i] = action;
55	}	73		}
56		74
57	} else if (m_actions.count() > numDesktops) {	75		} else if (m_actions.count() > numDesktops) {
58	for (int i = numDesktops +1; i <= m_actions.count(); ++i) {	76		for (int i = numDesktops +1; i <= m_actions.count(); ++i) {
59	delete m_actions[i];	77		delete m_actions[i];
60	m_actions.remove(i);	78		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	}	79		}
62	}	80		}
63		81
64	for (int i = 1; i <= numDesktops; ++i) {	82		for (int i = 1; i <= numDesktops; ++i) {
65	QAction *action = m_actions.value(i);	83		QAction *action = m_actions.value(i);
66	action->setEnabled(i != currentDesktop);	84		action->setText(QStringLiteral("%1: %2").arg(i).arg(desktopNames.at(i)));
67	list << action;	85		action->setData(desktopIds.at(i));
		86		action->setEnabled(desktopIds.at(i) != currentDesktop);
		87		actions << action;
68	}	88		}
69		89
70	return list;	90		return actions;
71	}	91		}
72		92
73	void SwitchDesktop::switchTo()	93		void SwitchDesktop::switchTo()
74	{	94		{
75	QAction action = qobject_cast<QAction >(sender());	95		const QAction action = qobject_cast<QAction >(sender());
		96
76	if (!action) {	97		if (!action) {
77	return;	98		return;
78	}	99		}
79		100
80	const int desktop = action->data().toInt();	101		s_virtualDesktopInfo->requestActivate(action->data());
81	KWindowSystem::setCurrentDesktop(desktop);
82	}	102		}
83		103
84	void SwitchDesktop::performNextAction()	104		void SwitchDesktop::performNextAction()
85	{	105		{
86	const int numDesktops = KWindowSystem::numberOfDesktops();	106		const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
87	const int currentDesktop = KWindowSystem::currentDesktop();	107		const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
88	KWindowSystem::setCurrentDesktop(currentDesktop % numDesktops + 1);	108		const int currentDesktopIndex = desktopIds.indexOf(currentDesktop);
		109
		110		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…
		111
		112		s_virtualDesktopInfo->requestActivate(desktopIds.at(nextDesktopIndex));
89	}	113		}
90		114
91	void SwitchDesktop::performPreviousAction()	115		void SwitchDesktop::performPreviousAction()
92	{	116		{
93	const int numDesktops = KWindowSystem::numberOfDesktops();	117		const QVariantList &desktopIds = s_virtualDesktopInfo->desktopIds();
94	const int currentDesktop = KWindowSystem::currentDesktop();	118		const QVariant &currentDesktop = s_virtualDesktopInfo->currentDesktop();
95	KWindowSystem::setCurrentDesktop((numDesktops + currentDesktop - 2) % numDesktops + 1);	119		const int currentDesktopIndex = desktopIds.indexOf(currentDesktop);
		120
		121		const int previousDesktopIndex = ((desktopIds.count() + currentDesktopIndex - 2) % desktopIds.count() + 1);
		122
		123		s_virtualDesktopInfo->requestActivate(desktopIds.at(previousDesktopIndex));
96	}	124		}
97		125
98	K_EXPORT_PLASMA_CONTAINMENTACTIONS_WITH_JSON(switchdesktop, SwitchDesktop, "plasma-containmentactions-switchdesktop.json")	126		K_EXPORT_PLASMA_CONTAINMENTACTIONS_WITH_JSON(switchdesktop, SwitchDesktop, "plasma-containmentactions-switchdesktop.json")
99		127
100	#include "desktop.moc"	128		#include "desktop.moc"