Unity 8

Merge lp:~lukas-kde/unity8/fixLauncherDismiss into lp:unity8

fixLauncherDismiss
Merge into trunk

Proposed by Lukáš Tinkl on 2016-01-05

Status:

Merged

Approved by:

Michał Sawicz on 2016-01-11

Approved revision:

2110

Merged at revision:

2128

Proposed branch:

lp:~lukas-kde/unity8/fixLauncherDismiss

Merge into:

lp:unity8

Diff against target:

93 lines (+24/-11)

3 files modified

qml/Launcher/Launcher.qml (+4/-9)
qml/Launcher/LauncherPanel.qml (+4/-2)
tests/qmltests/Launcher/tst_Launcher.qml (+16/-0)

To merge this branch:

bzr merge lp:~lukas-kde/unity8/fixLauncherDismiss

Related bugs:

Bug #1530940: Dismissing the launcher in the dimmed/shadow portion requires two taps	High	Fix Released
Bug #1531339: Clicking on empty space in launcher gives haptic feedback	Undecided	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2016-01-11
Daniel d'Andrada (community)			Abstain on 2016-01-07
Michael Terry		2016-01-05	Approve on 2016-01-06
Review via email: mp+281640@code.launchpad.net

Commit Message

Fix dismissing the launcher when clicking/tapping outside

Description of the Change

Fix dismissing the launcher when clicking/tapping outside in the darkened area and wrongly playing haptics when clicking empty space.

Fixes bug lp#1530940

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

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

Yes

* Did you make sure that your branch does not contain spurious tags?

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?

N/A

PS Jenkins bot (ps-jenkins) wrote on 2016-01-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7016/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2016-01-05:

- Why change onPressed to onClicked?
- Maybe use root.hide() instead of root.switchToNextState("")
- Do you need to set a sensingArea? Default sensing area is whole root window according to docs, which sounds reasonable.

review: Needs Information

PS Jenkins bot (ps-jenkins) wrote on 2016-01-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7019/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2016-01-05:

OK, I love the Launcher.qml diff, thanks! :)

Since the two-tap-dismiss bug was a regression, this feels like a reasonable and easy thing to add a test for.

Why the LauncherPanel.qml changes? Seems reasonable to just close the quicklist in that case, like we did before. Do you like the close-launcher behavior better or was this a design-request or something?

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

> Why the LauncherPanel.qml changes? Seems reasonable to just close the
> quicklist in that case, like we did before. Do you like the close-launcher
> behavior better or was this a design-request or something?

It wasn't anyone's request but it's just for the sake of consistency with unity7 behavior.

Michael Terry (mterry) wrote on 2016-01-05:

OK, makes sense, and good catch. :)

I still think a test here would be good though.

PS Jenkins bot (ps-jenkins) wrote on 2016-01-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7020/rebuild

review: Needs Fixing (continuous-integration)

Lukáš Tinkl (lukas-kde) wrote on 2016-01-06:

> OK, makes sense, and good catch. :)
>
> I still think a test here would be good though.

There is already a test (Launcher::test_dragndrop_cancel) but it's failing with touch (works fine with click tho :S); looking into that

PS Jenkins bot (ps-jenkins) wrote on 2016-01-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7022/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2016-01-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7024/rebuild

review: Needs Fixing (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-01-06:

I wonder if the bug you're trying to fix is related to a more general problem:
https://bugs.launchpad.net/ubuntu/+source/qtmir/+bug/1531517

PS Jenkins bot (ps-jenkins) wrote on 2016-01-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7025/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2016-01-06:

OK, looks good to me then. Fixes the bug for me (or at least, works around a Mir bug by using InverseMouseArea). And fixes the haptic issue.

review: Approve

Daniel d'Andrada (dandrader) wrote on 2016-01-06:

I would hold this MP until we know what's the problem in bug 1531517.

Michael Terry (mterry) wrote on 2016-01-06:

@Daniel, yeah for sure there's some Mir oddities going on if switching to InverseMouseArea fixes it. This MP is more of a workaround for that, I believe.

If bug 1531517 fixes the underlying issue, then good.

But this MP seems reasonable regardless -- InverseMouseArea is the right widget for the job. Plus the haptic fix in this MP is also good.

Though I will note that the "signal hidePanel()" bit should be dropped, as discussed on IRC. Marking fix needed until that's done.

review: Needs Fixing

Lukáš Tinkl (lukas-kde) wrote on 2016-01-06:

Added a test for dismissing the launcher (twice) plus reverted the quick list closing on clicking outside it (as discussed with mterry)

Michael Terry (mterry) wrote on 2016-01-06:

OK, that was fast. I'm back on board with this fix. I'll approve, but will defer to Daniel's desire to wait and not top-approve yet.

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2016-01-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7026/rebuild

review: Needs Fixing (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-01-07:

> I would hold this MP until we know what's the problem in bug 1531517.

Turned out to be a missing touch-pressed event on multi-finger taps/gestures. Don't think it affects this bug.

review: Abstain

Michael Terry (mterry) wrote on 2016-01-07:

Cool. So I'll top approve then.

PS Jenkins bot (ps-jenkins) wrote on 2016-01-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7042/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:

Lukáš Tinkl

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'qml/Launcher/Launcher.qml'
2	--- qml/Launcher/Launcher.qml 2015-11-24 17:44:18 +0000
3	+++ qml/Launcher/Launcher.qml 2016-01-11 15:31:33 +0000
4	@@ -184,18 +184,13 @@
5	}
6	}
7
8	- MultiPointTouchArea {
9	+ InverseMouseArea {
10	id: closeMouseArea
11	- anchors {
12	- left: launcherDragArea.right
13	- top: parent.top
14	- right: parent.right
15	- bottom: parent.bottom
16	- }
17	+ anchors.fill: panel
18	enabled: root.shadeBackground && root.state == "visible"
19	- visible: enabled // otherwise it will get in the way of cursor selection for some reason
20	+ visible: enabled
21	onPressed: {
22	- root.state = ""
23	+ root.hide();
24	}
25	}
26
27
28	=== modified file 'qml/Launcher/LauncherPanel.qml'
29	--- qml/Launcher/LauncherPanel.qml 2015-12-03 10:18:14 +0000
30	+++ qml/Launcher/LauncherPanel.qml 2016-01-11 15:31:33 +0000
31	@@ -392,7 +392,6 @@
32	}
33
34	onClicked: {
35	- Haptics.play();
36	var index = Math.floor((mouseY + launcherListView.realContentY) / launcherListView.realItemHeight);
37	var clickedItem = launcherListView.itemAt(mouseX, mouseY + launcherListView.realContentY)
38
39	@@ -407,9 +406,11 @@
40	quickList.model = launcherListView.model.get(index).quickList;
41	quickList.appId = launcherListView.model.get(index).appId;
42	quickList.state = "open";
43	- return
44	+ return;
45	}
46
47	+ Haptics.play();
48	+
49	// First/last item do the scrolling at more than 12 degrees
50	if (index == 0 \|\| index == launcherListView.count - 1) {
51	if (clickedItem.angle > 12) {
52	@@ -636,6 +637,7 @@
53	rotation: 90
54	}
55	}
56	+
57	InverseMouseArea {
58	anchors.fill: quickListShape
59	enabled: quickList.state == "open" \|\| pressed
60
61	=== modified file 'tests/qmltests/Launcher/tst_Launcher.qml'
62	--- tests/qmltests/Launcher/tst_Launcher.qml 2015-12-03 10:18:14 +0000
63	+++ tests/qmltests/Launcher/tst_Launcher.qml 2016-01-11 15:31:33 +0000
64	@@ -625,6 +625,7 @@
65	if(data.mouse) {
66	mouseClick(root)
67	} else {
68	+ touchRelease(draggedItem)
69	tap(root)
70	}
71
72	@@ -739,6 +740,21 @@
73	mouseRelease(draggedItem);
74	}
75
76	+ function test_launcher_dismiss() {
77	+ dragLauncherIntoView();
78	+ verify(launcher.state == "visible");
79	+ mouseClick(root);
80	+ waitUntilLauncherDisappears();
81	+ verify(launcher.state == "");
82	+
83	+ // and repeat, as a test for regression in lpbug#1531339
84	+ dragLauncherIntoView();
85	+ verify(launcher.state == "visible");
86	+ mouseClick(root);
87	+ waitUntilLauncherDisappears();
88	+ verify(launcher.state == "");
89	+ }
90	+
91	function test_quicklist_positioning_data() {
92	return [
93	{tag: "top", flickTo: "top", itemIndex: 0},