Unity 8

debian/control (+1/-1)
plugins/Greeter/Unity/Launcher/CMakeLists.txt (+1/-1)
plugins/Greeter/Unity/Launcher/launchermodelas.cpp (+0/-7)
plugins/Greeter/Unity/Launcher/launchermodelas.h (+0/-1)
plugins/Unity/Launcher/CMakeLists.txt (+1/-1)
plugins/Unity/Launcher/launcheritem.cpp (+0/-9)
plugins/Unity/Launcher/launchermodel.cpp (+29/-24)
plugins/Unity/Launcher/launchermodel.h (+0/-1)
qml/Launcher/LauncherDelegate.qml (+1/-7)
qml/Launcher/LauncherPanel.qml (+1/-0)
tests/mocks/Unity/Launcher/CMakeLists.txt (+1/-1)
tests/mocks/Unity/Launcher/MockLauncherModel.h (+1/-1)
tests/plugins/Unity/Launcher/launchermodeltest.cpp (+8/-0)

To merge this branch:

bzr merge lp:~mzanetti/unity8/alerting-pips

Related bugs:

Bug #1575147: Launcher: make the pip blue on alerts	Undecided	Fix Released
Bug #1584348: Launcher thinking app is running still when it's dead	High	Fix Released
Bug #1586621: Launcher 'active' dot doesn't dynamically update	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration	2016-05-13	Needs Fixing on 2016-06-06
Lukáš Tinkl (community)		2016-05-13	Approve on 2016-05-18
Ubuntu Unity PS integration team		2016-05-13	Pending
Review via email: mp+294629@code.launchpad.net

This proposal supersedes a proposal from 2016-05-13.

Commit message

Add support for the persistent alert state.

Paint pips blue when an app is in alert state

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.

https://code.launchpad.net/~mzanetti/unity-api/launcher-drop-setAlerting/+merge/294628
See prereq too

* Did you perform an exploratory manual test run of your code change and any related functionality?

yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

yes

* If you changed the UI, has there been a design review?

yes, change requested by design. This reveals an issue in the design spec. Here's my conversation with John:

<mzanetti> I have a small issue with the persistent alert state
we are currently coloring pips blue. Problem is, if the app is not running and the alert happened because of a push notification, there is no pip which could be blue
the count emblem will still be there though
<JohnLea> humm, yes this is a small problem
I think it is ok for the moment, but a better solution could be to use something else as the persistent alert notification
<mzanetti> you tell me: a) ignore the problem or b) force a pip even if no surface
<JohnLea> for now do a)
<mzanetti> ack
<JohnLea> in the mean time I'll speak to Matthieu and ask him to have a look at other options, perhaps indicating something on the app icon itself
but that's for the future

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-05-13: Posted in a previous version of this proposal

* Are there any related MPs required for this MP to build/function as expected? Please list.

yes, listing them in a minute

* Did you perform an exploratory manual test run of your code change and any related functionality?

yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

yes

* If you changed the UI, has there been a design review?

yes, change requested by design. This reveals an issue in the design spec. Here's my conversation with John:

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-13: Posted in a previous version of this proposal

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1214/rebuild

review: Needs Fixing (continuous-integration)

lp:~mzanetti/unity8/alerting-pips updated on 2016-05-13

2401. By Michael Zanetti on 2016-05-13: cleanup
2402. By Michael Zanetti on 2016-05-13: fixes
2403. By Michael Zanetti on 2016-05-13: fixes
2404. By Michael Zanetti on 2016-05-13: fix tests

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-13:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1215/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-13:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1217/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-13:

+ color: root.alerting ? UbuntuColors.blue : "white"

Please use a palette value (sth like theme.palette.normal.activity)

review: Needs Fixing

lp:~mzanetti/unity8/alerting-pips updated on 2016-05-13

2405. By Michael Zanetti on 2016-05-13: fix usage of color vs palette

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-13:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1218/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-13:

In make tryLauncher, "set alert" doesn't work for first item (index 0)

review: Needs Fixing

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-05-13:

> In make tryLauncher, "set alert" doesn't work for first item (index 0)

Yes, I looked into that for a while too, but turns out this is ok. If you look at item 0 you'll see that it has the focused pip. According to the design, if an app is focused, the launcher should not honor alerts, given the user already sees the app.

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-13:

Hmm, that's a bit inconsistent... shouldn't the pip at least turn blue?

review: Needs Information

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-05-13:

> Hmm, that's a bit inconsistent... shouldn't the pip at least turn blue?

Focusing an app is the way to clear the alert. So if the app is already focused, the alert should be ignored. The user will see the important bits in the app anyways. If we would activate the alert (blue pip) then the user would do whatever the app wants attention for, the user would switch away to another app, and the alert highlight would still be there, even though the actual reason for the alert is done already. So I think the design makes sense.

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-13:

> > Hmm, that's a bit inconsistent... shouldn't the pip at least turn blue?
>
> Focusing an app is the way to clear the alert. So if the app is already
> focused, the alert should be ignored. The user will see the important bits in
> the app anyways. If we would activate the alert (blue pip) then the user would
> do whatever the app wants attention for, the user would switch away to another
> app, and the alert highlight would still be there, even though the actual
> reason for the alert is done already. So I think the design makes sense.

Alright, makes sense, thanks for clearing up

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-13:

Last thing from me, do you think it'd makes sense to add a test for the blue pip (and alert state) when an app is alerted, and clearing it when it's focused?

review: Needs Information

lp:~mzanetti/unity8/alerting-pips updated on 2016-05-18

2406. By Michael Zanetti on 2016-05-18: test if the alert is properly cleared on focus

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-05-18:

> Last thing from me, do you think it'd makes sense to add a test for the blue
> pip (and alert state) when an app is alerted, and clearing it when it's
> focused?

sure thing. done

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-18:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1236/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-05-18:

Yup, works fine for me

* Did you perform an exploratory manual test run of the code change and any related functionality?

Yes

* Did CI run pass? If not, please explain why.

No, incompatible unity-api; passes locally

review: Approve

Revision history for this message

Albert Astals Cid (aacid) wrote on 2016-05-19:

Note: This is top approved but the prerequisite is not.

lp:~mzanetti/unity8/alerting-pips updated on 2016-05-19

2407. By Michael Zanetti on 2016-05-19: merge prereq

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-03:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1339/rebuild

review: Needs Fixing (continuous-integration)

lp:~mzanetti/unity8/alerting-pips updated on 2016-06-06

2408. By Michael Zanetti on 2016-06-03: merge prereq, bump unity-api version requirement
2409. By Michael Zanetti on 2016-06-03: bump package requirement once more
2410. By Michael Zanetti on 2016-06-03: that wasn't necessary
2411. By Michael Zanetti on 2016-06-03: merge prereq
2412. By Michael Zanetti on 2016-06-06: merge prereq

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-06:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1405/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Michael Zanetti

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'debian/control'
2	--- debian/control 2016-06-06 14:15:40 +0000
3	+++ debian/control 2016-06-06 14:15:41 +0000
4	@@ -32,7 +32,7 @@
5	libubuntugestures5-dev,
6	libubuntugestures5-private-dev,
7	libudev-dev,
8	- libunity-api-dev (>= 7.114),
9	+ libunity-api-dev (>= 7.115),
10	libusermetricsoutput1-dev,
11	# Need those X11 libs touch emulation from mouse events in manual QML tests on a X11 desktop
12	libx11-dev[!armhf],
13
14	=== modified file 'plugins/Greeter/Unity/Launcher/CMakeLists.txt'
15	--- plugins/Greeter/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:40 +0000
16	+++ plugins/Greeter/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:41 +0000
17	@@ -1,4 +1,4 @@
18	-pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=8)
19	+pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=9)
20	pkg_check_modules(GSETTINGS_QT REQUIRED gsettings-qt)
21
22	add_definitions(-DSM_BUSNAME=systemBus)
23
24	=== modified file 'plugins/Greeter/Unity/Launcher/launchermodelas.cpp'
25	--- plugins/Greeter/Unity/Launcher/launchermodelas.cpp 2016-06-06 14:15:40 +0000
26	+++ plugins/Greeter/Unity/Launcher/launchermodelas.cpp 2016-06-06 14:15:41 +0000
27	@@ -76,13 +76,6 @@
28	return QVariant();
29	}
30
31	-void LauncherModel::setAlerting(const QString &appId, bool alerting)
32	-{
33	- Q_UNUSED(appId)
34	- Q_UNUSED(alerting)
35	- qWarning() << "This is a read only implementation. Cannot set alert-state of items.";
36	-}
37	-
38	unity::shell::launcher::LauncherItemInterface *LauncherModel::get(int index) const
39	{
40	if (index < 0 \|\| index >= m_list.count()) {
41
42	=== modified file 'plugins/Greeter/Unity/Launcher/launchermodelas.h'
43	--- plugins/Greeter/Unity/Launcher/launchermodelas.h 2015-10-26 14:05:14 +0000
44	+++ plugins/Greeter/Unity/Launcher/launchermodelas.h 2016-06-06 14:15:41 +0000
45	@@ -41,7 +41,6 @@
46
47	QVariant data(const QModelIndex &index, int role) const override;
48
49	- Q_INVOKABLE void setAlerting(const QString &appId, bool alerting) override;
50	Q_INVOKABLE unity::shell::launcher::LauncherItemInterface* get(int index) const override;
51	Q_INVOKABLE void move(int oldIndex, int newIndex) override;
52	Q_INVOKABLE void pin(const QString &appId, int index = -1) override;
53
54	=== modified file 'plugins/Unity/Launcher/CMakeLists.txt'
55	--- plugins/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:40 +0000
56	+++ plugins/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:41 +0000
57	@@ -1,4 +1,4 @@
58	-pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=8)
59	+pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=9)
60	pkg_check_modules(GSETTINGS_QT REQUIRED gsettings-qt)
61
62	add_definitions(-DSM_BUSNAME=systemBus)
63
64	=== modified file 'plugins/Unity/Launcher/launcheritem.cpp'
65	--- plugins/Unity/Launcher/launcheritem.cpp 2016-06-06 14:15:40 +0000
66	+++ plugins/Unity/Launcher/launcheritem.cpp 2016-06-06 14:15:41 +0000
67	@@ -163,9 +163,6 @@
68	if (m_count != count) {
69	m_count = count;
70	Q_EMIT countChanged(count);
71	- if (m_countVisible) {
72	- setAlerting(true);
73	- }
74	}
75	}
76
77	@@ -179,9 +176,6 @@
78	if (m_countVisible != countVisible) {
79	m_countVisible = countVisible;
80	Q_EMIT countVisibleChanged(countVisible);
81	- if (countVisible) {
82	- setAlerting(true);
83	- }
84	}
85	}
86
87	@@ -194,9 +188,6 @@
88	{
89	if (m_focused != focused) {
90	m_focused = focused;
91	- if (focused) {
92	- setAlerting(false);
93	- }
94	Q_EMIT focusedChanged(focused);
95	}
96	}
97
98	=== modified file 'plugins/Unity/Launcher/launchermodel.cpp'
99	--- plugins/Unity/Launcher/launchermodel.cpp 2016-06-06 14:15:40 +0000
100	+++ plugins/Unity/Launcher/launchermodel.cpp 2016-06-06 14:15:41 +0000
101	@@ -99,18 +99,6 @@
102	return QVariant();
103	}
104
105	-void LauncherModel::setAlerting(const QString &appId, bool alerting) {
106	- int index = findApplication(appId);
107	- if (index >= 0) {
108	- QModelIndex modelIndex = this->index(index);
109	- LauncherItem *item = m_list.at(index);
110	- if (!item->focused()) {
111	- item->setAlerting(alerting);
112	- Q_EMIT dataChanged(modelIndex, modelIndex, {RoleAlerting});
113	- }
114	- }
115	-}
116	-
117	unity::shell::launcher::LauncherItemInterface *LauncherModel::get(int index) const
118	{
119	if (index < 0 \|\| index >= m_list.count()) {
120	@@ -361,11 +349,13 @@
121	if (idx >= 0) {
122	LauncherItem *item = m_list.at(idx);
123	item->setCount(count);
124	- if (item->countVisible()) {
125	- setAlerting(item->appId(), true);
126	+ QVector<int> changedRoles = {RoleCount};
127	+ if (item->countVisible() && !item->alerting() && !item->focused()) {
128	+ changedRoles << RoleAlerting;
129	+ item->setAlerting(true);
130	}
131	m_asAdapter->syncItems(m_list);
132	- Q_EMIT dataChanged(index(idx), index(idx), {RoleCount});
133	+ Q_EMIT dataChanged(index(idx), index(idx), changedRoles);
134	}
135	}
136
137	@@ -375,10 +365,12 @@
138	if (idx >= 0) {
139	LauncherItem *item = m_list.at(idx);
140	item->setCountVisible(countVisible);
141	- if (countVisible) {
142	- setAlerting(item->appId(), true);
143	+ QVector<int> changedRoles = {RoleCount};
144	+ if (countVisible && !item->alerting() && !item->focused()) {
145	+ changedRoles << RoleAlerting;
146	+ item->setAlerting(true);
147	}
148	- Q_EMIT dataChanged(index(idx), index(idx), {RoleCountVisible});
149	+ Q_EMIT dataChanged(index(idx), index(idx), changedRoles);
150
151	// If countVisible goes to false, and the item is neither pinned nor recent we can drop it
152	if (!countVisible && !item->pinned() && !item->recent()) {
153	@@ -501,8 +493,10 @@
154	int idx = findApplication(appId);
155	if (idx >= 0) {
156	LauncherItem *item = m_list.at(idx);
157	- setAlerting(item->appId(), true);
158	- Q_EMIT dataChanged(index(idx), index(idx), {RoleAlerting});
159	+ if (!item->focused() && !item->alerting()) {
160	+ item->setAlerting(true);
161	+ Q_EMIT dataChanged(index(idx), index(idx), {RoleAlerting});
162	+ }
163	}
164	}
165
166	@@ -585,16 +579,21 @@
167	disconnect(app, &ApplicationInfoInterface::surfaceCountChanged, this, &LauncherModel::applicationSurfaceCountChanged);
168
169	LauncherItem * item = m_list.at(appIndex);
170	- item->setRunning(false);
171
172	if (!item->pinned()) {
173	beginRemoveRows(QModelIndex(), appIndex, appIndex);
174	m_list.takeAt(appIndex)->deleteLater();
175	endRemoveRows();
176	m_asAdapter->syncItems(m_list);
177	- Q_EMIT dataChanged(index(appIndex), index(appIndex), {RolePinned});
178	+ } else {
179	+ QVector<int> changedRoles = {RoleRunning};
180	+ item->setRunning(false);
181	+ if (item->focused()) {
182	+ changedRoles << RoleFocused;
183	+ item->setFocused(false);
184	+ }
185	+ Q_EMIT dataChanged(index(appIndex), index(appIndex), changedRoles);
186	}
187	- Q_EMIT dataChanged(index(appIndex), index(appIndex), {RoleRunning});
188	}
189
190	void LauncherModel::focusedAppIdChanged()
191	@@ -603,8 +602,14 @@
192	for (int i = 0; i < m_list.count(); ++i) {
193	LauncherItem *item = m_list.at(i);
194	if (!item->focused() && item->appId() == appId) {
195	+ QVector<int> changedRoles;
196	+ changedRoles << RoleFocused;
197	item->setFocused(true);
198	- Q_EMIT dataChanged(index(i), index(i), {RoleFocused});
199	+ if (item->alerting()) {
200	+ changedRoles << RoleAlerting;
201	+ item->setAlerting(false);
202	+ }
203	+ Q_EMIT dataChanged(index(i), index(i), changedRoles);
204	} else if (item->focused() && item->appId() != appId) {
205	item->setFocused(false);
206	Q_EMIT dataChanged(index(i), index(i), {RoleFocused});
207
208	=== modified file 'plugins/Unity/Launcher/launchermodel.h'
209	--- plugins/Unity/Launcher/launchermodel.h 2016-06-06 14:15:40 +0000
210	+++ plugins/Unity/Launcher/launchermodel.h 2016-06-06 14:15:41 +0000
211	@@ -45,7 +45,6 @@
212
213	QVariant data(const QModelIndex &index, int role) const override;
214
215	- Q_INVOKABLE void setAlerting(const QString &appId, bool alerting) override;
216	Q_INVOKABLE unity::shell::launcher::LauncherItemInterface* get(int index) const override;
217	Q_INVOKABLE void move(int oldIndex, int newIndex) override;
218	Q_INVOKABLE void pin(const QString &appId, int index = -1) override;
219
220	=== modified file 'qml/Launcher/LauncherDelegate.qml'
221	--- qml/Launcher/LauncherDelegate.qml 2016-06-06 14:15:40 +0000
222	+++ qml/Launcher/LauncherDelegate.qml 2016-06-06 14:15:41 +0000
223	@@ -115,12 +115,6 @@
224	duration: priv.wiggleDuration
225	easing.type: Easing.OutQuad
226	}
227	-
228	- UbuntuNumberAnimation {
229	- target: root
230	- property: "alerting"
231	- to: 0
232	- }
233	}
234
235	Item {
236	@@ -228,7 +222,7 @@
237	objectName: "runningHighlight" + index
238	width: units.gu(0.25)
239	height: units.gu(.5)
240	- color: "white"
241	+ color: root.alerting ? theme.palette.normal.activity : "white"
242	visible: root.itemRunning
243	}
244	}
245
246	=== modified file 'qml/Launcher/LauncherPanel.qml'
247	--- qml/Launcher/LauncherPanel.qml 2016-06-06 14:15:40 +0000
248	+++ qml/Launcher/LauncherPanel.qml 2016-06-06 14:15:41 +0000
249	@@ -272,6 +272,7 @@
250
251	PropertyAction { target: launcherListViewItem; property: "clip"; value: 1 }
252	PropertyAction { target: root; property: "visible"; value: (launcher.visibleWidth === 0) ? 0 : 1 }
253	+ PropertyAction { target: launcherListView; property: "peekingIndex"; value: -1 }
254	}
255
256	onAlertingChanged: {
257
258	=== modified file 'tests/mocks/Unity/Launcher/CMakeLists.txt'
259	--- tests/mocks/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:40 +0000
260	+++ tests/mocks/Unity/Launcher/CMakeLists.txt 2016-06-06 14:15:41 +0000
261	@@ -1,4 +1,4 @@
262	-pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=8)
263	+pkg_check_modules(LAUNCHER_API REQUIRED unity-shell-launcher=9)
264
265	include_directories(
266	${CMAKE_CURRENT_SOURCE_DIR}
267
268	=== modified file 'tests/mocks/Unity/Launcher/MockLauncherModel.h'
269	--- tests/mocks/Unity/Launcher/MockLauncherModel.h 2015-07-23 10:31:56 +0000
270	+++ tests/mocks/Unity/Launcher/MockLauncherModel.h 2016-06-06 14:15:41 +0000
271	@@ -38,7 +38,7 @@
272
273	QVariant data(const QModelIndex& index, int role) const override;
274
275	- Q_INVOKABLE void setAlerting(const QString &appId, bool alerting) override;
276	+ Q_INVOKABLE void setAlerting(const QString &appId, bool alerting); // Only for testing
277	Q_INVOKABLE unity::shell::launcher::LauncherItemInterface *get(int index) const override;
278	Q_INVOKABLE void move(int oldIndex, int newIndex) override;
279	Q_INVOKABLE void pin(const QString &appId, int index = -1) override;
280
281	=== modified file 'tests/plugins/Unity/Launcher/launchermodeltest.cpp'
282	--- tests/plugins/Unity/Launcher/launchermodeltest.cpp 2016-06-06 14:15:40 +0000
283	+++ tests/plugins/Unity/Launcher/launchermodeltest.cpp 2016-06-06 14:15:41 +0000
284	@@ -491,6 +491,10 @@
285
286	// Finally check, that the change to "count" implicitly also set the alerting-state to true
287	QVERIFY(launcherModel->get(index)->alerting() == true);
288	+
289	+ // Focus the app, make sure the alert gets cleared
290	+ appManager->focusApplication("abs-icon");
291	+ QVERIFY(launcherModel->get(index)->alerting() == false);
292	}
293
294	void testCountEmblemAddsRemovesItem_data() {
295	@@ -562,6 +566,10 @@
296
297	// Check that the alerting-status is now true
298	QVERIFY(launcherModel->get(index)->alerting() == true);
299	+
300	+ // Focus the app, make sure the alert gets cleared
301	+ appManager->focusApplication("abs-icon");
302	+ QVERIFY(launcherModel->get(index)->alerting() == false);
303	}
304
305	void testRefreshAfterDeletedDesktopFiles_data() {