Unity 8

Merge lp:~dandrader/unity8/pixelAlignedWindow into lp:unity8

pixelAlignedWindow
Merge into trunk

Proposed by Daniel d'Andrada on 2016-05-19

Status:

Merged

Approved by:

Albert Astals Cid on 2016-05-23

Approved revision:

2400

Merged at revision:

2453

Proposed branch:

lp:~dandrader/unity8/pixelAlignedWindow

Merge into:

lp:unity8

Prerequisite:

lp:~dandrader/unity8/animatedCursors

Diff against target:

71 lines (+17/-6)

3 files modified

plugins/Cursor/MousePointer.cpp (+8/-3)
plugins/Cursor/MousePointer.h (+4/-1)
qml/Stages/WindowDecoration.qml (+5/-2)

To merge this branch:

bzr merge lp:~dandrader/unity8/pixelAlignedWindow

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration		Approve on 2016-06-05
Albert Astals Cid (community)			Approve on 2016-05-23
Lukáš Tinkl (community)		2016-05-19	Needs Information on 2016-05-20
Review via email: mp+295262@code.launchpad.net

Commit Message

Ensure mouse and window movement are pixel-aligned

To avoid blurry renderings (LP: #1510382)

Description of the Change

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

* 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?
N/A

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

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

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

review: Needs Fixing (continuous-integration)

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

I wonder how can one reproduce it (seeing the screenshots in the attached BR); I've never experienced these glitches. Also, would be nice to have a test, at least for the window movement part.

review: Needs Information

Daniel d'Andrada (dandrader) wrote on 2016-05-20:

> I wonder how can one reproduce it (seeing the screenshots in the attached BR);
> I've never experienced these glitches. Also, would be nice to have a test, at
> least for the window movement part.

I also never caught my attention. But by close visual inspection and adding some logging I did notice (and read proof) that cursor and window were not pixel aligned on my laptop and indeed a bit blurry as a result. The cursor definitely looks better now.

How perceptible is this issue depends on the pixel density of your display and how big the grid unit is. In your case (a 2160p laptop display) it might be hard to see the difference.

Will see about the test.

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

The window is a very noticeable improvement.

On the cursor though i get visual noise with the patch
if i move the mouse horizontally to the right the vertical line of the pointing cursor seems to have some ghost a few pixels to its left
I tried recording it with the phone but i guess the camera is not as good/bad as my eyes

can you try to reproduce?

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2016-05-20:

On 20/05/2016 10:17, Albert Astals Cid wrote:
> Review: Needs Fixing
>
> The window is a very noticeable improvement.
>
> On the cursor though i get visual noise with the patch
> if i move the mouse horizontally to the right the vertical line of the pointing cursor seems to have some ghost a few pixels to its left
> I tried recording it with the phone but i guess the camera is not as good/bad as my eyes
>
> can you try to reproduce?

Please apply that patch:

http://pastebin.ubuntu.com/16520939/

And see if you get any non-integer position.

Also please send me a screenshot on startup, before you move the mouse
so it will be pixel aligned at (0,0) and when you think it looks bad.

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

Ok, so the ghosting happens without the patch, may be an issue somewhere else, let's not block on that.

How sensible is a test for this? Maybe something that connects to the x/y position of the window/cursor when running all the other tests and asserts if the position is not an integer?

review: Needs Information

Daniel d'Andrada (dandrader) wrote on 2016-05-20:

Can't properly test. QtQuickTest::mouseEvent (which is what eventually gets called when you call mouseMove() from a qml test) converts movement from QPointF to QPoint, truncating it. So I can't emulate a subpixel-precision mouse movement in a qml test.

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

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

* Did CI run pass?
Parent branch has unity-api dep, ran fine on my machine.

review: Approve

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/1342/rebuild

review: Needs Fixing (continuous-integration)

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

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

review: Approve (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:

Daniel d'Andrada

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'plugins/Cursor/MousePointer.cpp'
2	--- plugins/Cursor/MousePointer.cpp 2016-05-19 19:51:50 +0000
3	+++ plugins/Cursor/MousePointer.cpp 2016-05-19 19:51:50 +0000
4	@@ -1,5 +1,5 @@
5	/*
6	- * Copyright (C) 2015 Canonical, Ltd.
7	+ * Copyright (C) 2015-2016 Canonical, Ltd.
8	*
9	* This program is free software: you can redistribute it and/or modify it under
10	* the terms of the GNU Lesser General Public License version 3, as published by
11	@@ -43,7 +43,12 @@
12	Q_EMIT mouseMoved();
13	}
14
15	- qreal newX = x() + movement.x();
16	+ m_accumulatedMovement += movement;
17	+ // don't apply the fractional part
18	+ QPointF appliedMovement(int(m_accumulatedMovement.x()), int(m_accumulatedMovement.y()));
19	+ m_accumulatedMovement -= appliedMovement;
20	+
21	+ qreal newX = x() + appliedMovement.x();
22	if (newX < 0) {
23	Q_EMIT pushedLeftBoundary(qAbs(newX), buttons);
24	newX = 0;
25	@@ -53,7 +58,7 @@
26	}
27	setX(newX);
28
29	- qreal newY = y() + movement.y();
30	+ qreal newY = y() + appliedMovement.y();
31	if (newY < 0) {
32	newY = 0;
33	} else if (newY > parentItem()->height()) {
34
35	=== modified file 'plugins/Cursor/MousePointer.h'
36	--- plugins/Cursor/MousePointer.h 2016-05-19 19:51:50 +0000
37	+++ plugins/Cursor/MousePointer.h 2016-05-19 19:51:50 +0000
38	@@ -1,5 +1,5 @@
39	/*
40	- * Copyright (C) 2015 Canonical, Ltd.
41	+ * Copyright (C) 2015-2016 Canonical, Ltd.
42	*
43	* This program is free software: you can redistribute it and/or modify it under
44	* the terms of the GNU Lesser General Public License version 3, as published by
45	@@ -56,6 +56,9 @@
46	QPointer<QWindow> m_registeredWindow;
47	QString m_cursorName;
48	QString m_themeName;
49	+
50	+ // Accumulated, unapplied, mouse movement.
51	+ QPointF m_accumulatedMovement;
52	};
53
54	#endif // MOUSEPOINTER_H
55
56	=== modified file 'qml/Stages/WindowDecoration.qml'
57	--- qml/Stages/WindowDecoration.qml 2016-04-27 15:01:10 +0000
58	+++ qml/Stages/WindowDecoration.qml 2016-05-19 19:51:50 +0000
59	@@ -58,8 +58,11 @@
60	if (priv.dragging) {
61	Mir.cursorName = "grabbing";
62	var pos = mapToItem(root.target.parent, mouseX, mouseY);
63	- root.target.x = pos.x - priv.distanceX;
64	- root.target.y = Math.max(pos.y - priv.distanceY, PanelState.panelHeight);
65	+ // Use integer coordinate values to ensure that target is left in a pixel-aligned
66	+ // position. Mouse movement could have subpixel precision, yielding a fractional
67	+ // mouse position.
68	+ root.target.x = Math.round(pos.x - priv.distanceX);
69	+ root.target.y = Math.round(Math.max(pos.y - priv.distanceY, PanelState.panelHeight));
70	}
71	}
72