Mir

Merge lp:~vanvugt/mir/fix-hide-then-show into lp:mir

fix-hide-then-show
Merge into development-branch

Proposed by Daniel van Vugt on 2017-02-01

Status:	Merged
Approved by:	Daniel van Vugt on 2017-02-14
Approved revision:	4006
Merged at revision:	4028
Proposed branch:	lp:~vanvugt/mir/fix-hide-then-show
Merge into:	lp:mir
Diff against target:	39 lines (+11/-4) 2 files modified src/platforms/mesa/server/kms/cursor.cpp (+1/-4) tests/unit-tests/platforms/mesa/kms/test_cursor.cpp (+10/-0)
To merge this branch:	bzr merge lp:~vanvugt/mir/fix-hide-then-show
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Chris Halse Rogers		2017-02-01	Approve on 2017-02-14
Mir CI Bot	continuous-integration		Approve on 2017-02-02
Review via email: mp+316089@code.launchpad.net

Commit Message

Fix a strange little bug in the DRM cursor code where:
hide(); show(); // WORKS
but
hide(); show(image); // FAILS

Description of the Change

Not known to be a problem in production, but I ran into it in another branch I'm working on. Although on some random occasions in the past Mir on my desktop has lost its cursor... so maybe this explains those.

lp:~vanvugt/mir/fix-hide-then-show updated on 2017-02-01

4004. By Daniel van Vugt on 2017-02-01: Tidy up the test (remove copy/paste stupidity)

Mir CI Bot (mir-ci-bot) wrote on 2017-02-01:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2907/rebuild

review: Approve (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-02-01:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2911/rebuild

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2017-02-02:

^^^
Bug 1646375

lp:~vanvugt/mir/fix-hide-then-show updated on 2017-02-02

4005. By Daniel van Vugt on 2017-02-02: Merge latest trunk

Mir CI Bot (mir-ci-bot) wrote on 2017-02-02:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2921/rebuild

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2017-02-02:

^^^
Bug 1661187

lp:~vanvugt/mir/fix-hide-then-show updated on 2017-02-02

4006. By Daniel van Vugt on 2017-02-02: Try again

Mir CI Bot (mir-ci-bot) wrote on 2017-02-02:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2926/rebuild

review: Approve (continuous-integration)

Chris Halse Rogers (raof) wrote on 2017-02-06:

The comment is now misleading; please either just remove the if() without re-arranging the call, or remove the comment :)

Making the place_cursor_at_locked() call before setting visible=true would be my preference, but that requires further changes...

review: Needs Fixing

Daniel van Vugt (vanvugt) wrote on 2017-02-06:

Incorrect.

The comment refers to image-writing functions that might throw (out of view in this diff). And it does have to go below visible=true or else it does nothing. See:

void mgm::Cursor::place_cursor_at_locked(
    std::lock_guard<std::mutex> const&,
    geometry::Point position,
    ForceCursorState force_state)
{

current_position = position;

if (!visible)
return;
...

Chris Halse Rogers (raof) wrote on 2017-02-14:

Yup, correct.

review: Approve

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 van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/platforms/mesa/server/kms/cursor.cpp'
2	--- src/platforms/mesa/server/kms/cursor.cpp 2016-08-05 07:33:22 +0000
3	+++ src/platforms/mesa/server/kms/cursor.cpp 2017-02-02 09:55:41 +0000
4	@@ -199,13 +199,10 @@
5	}
6	hotspot = cursor_image.hotspot();
7
8	- // The hotspot may have changed so we need to call drmModeSetCursor again if the cursor was already visible.
9	- if (visible)
10	- place_cursor_at_locked(lg, current_position, ForceState);
11	-
12	// Writing the data could throw an exception so lets
13	// hold off on setting visible until after we have succeeded.
14	visible = true;
15	+ place_cursor_at_locked(lg, current_position, ForceState);
16	}
17
18	void mgm::Cursor::move_to(geometry::Point position)
19
20	=== modified file 'tests/unit-tests/platforms/mesa/kms/test_cursor.cpp'
21	--- tests/unit-tests/platforms/mesa/kms/test_cursor.cpp 2017-01-18 02:29:37 +0000
22	+++ tests/unit-tests/platforms/mesa/kms/test_cursor.cpp 2017-02-02 09:55:41 +0000
23	@@ -709,6 +709,16 @@
24	output_container.verify_and_clear_expectations();
25	}
26
27	+TEST_F(MesaCursorTest, show_with_param_places_cursor_on_output)
28	+{
29	+ EXPECT_CALL(*output_container.outputs[10], clear_cursor());
30	+ cursor.hide();
31	+
32	+ output_container.verify_and_clear_expectations();
33	+
34	+ EXPECT_CALL(*output_container.outputs[10], set_cursor(_));
35	+ cursor.show(stub_image);
36	+}
37
38	TEST_F(MesaCursorTest, show_without_param_places_cursor_on_output_output)
39	{