unity-mir

Merge lp://qastaging/~gerboland/unity-mir/fix-compatibility-with-mir-0.1.8 into lp://qastaging/unity-mir

fix-compatibility-with-mir-0.1.8
Merge into trunk

Proposed by Gerry Boland on 2014-04-11

Status:	Merged
Merged at revision:	212
Proposed branch:	lp://qastaging/~gerboland/unity-mir/fix-compatibility-with-mir-0.1.8
Merge into:	lp://qastaging/unity-mir
Prerequisite:	lp://qastaging/~alan-griffiths/unity-mir/compatibility-with-mir-0.1.8
Diff against target:	439 lines (+54/-175) 12 files modified src/modules/Unity/Application/application_manager.cpp (+0/-1) src/modules/Unity/Application/inputarea.cpp (+0/-1) src/modules/Unity/Application/mirsurfacemanager.cpp (+1/-2) src/unity-mir/CMakeLists.txt (+0/-2) src/unity-mir/initialsurfaceplacementstrategy.cpp (+33/-4) src/unity-mir/initialsurfaceplacementstrategy.h (+8/-5) src/unity-mir/sessionlistener.cpp (+10/-1) src/unity-mir/sessionlistener.h (+2/-0) src/unity-mir/shellserverconfiguration.cpp (+0/-19) src/unity-mir/shellserverconfiguration.h (+0/-5) src/unity-mir/surfacefactory.cpp (+0/-81) src/unity-mir/surfacefactory.h (+0/-54)
To merge this branch:	bzr merge lp://qastaging/~gerboland/unity-mir/fix-compatibility-with-mir-0.1.8
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-04-11	Needs Fixing on 2014-04-14
Alan Griffiths		2014-04-11	Needs Fixing on 2014-04-14
Review via email: mp+215454@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-04-10.

Commit message

Mir 0.1.8 refactoring allows removal of custom SurfaceFactory and instead use InitialSurfacePlacementStrategy to set surface depth and other properties. The shell surface notification now comes via SessionListener

Description of the change

* Are there any related MPs required for this MP to build/function as expected? https://code.launchpad.net/~gerboland/platform-api/fix-compatibility-with-mir-0.1.8/+merge/215455

* Did you perform an exploratory manual test run of your code change and any related functionality?
Y
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-10: Posted in a previous version of this proposal

FAILED: Continuous integration, rev:211
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/316/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/179/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/180/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/179/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/316/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-11:

FAILED: Continuous integration, rev:211
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/317/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/180/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/181/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/180/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/317/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-14:

100 + static bool mainShellSurfaceFound = false;

Wouldn't a class member be better?

In practice, this only makes a difference if InitialSurfacePlacementStrategy is instantiated twice. This is probably unlikely in production code (it implies that the server is torn down and rebuilt in the same process but I can imagine that happening in a test harness and there's no good reason have surprising behavior if it does.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-14:

100 + static bool mainShellSurfaceFound = false;
101 +
102 + if (session.process_id() == 0 && !mainShellSurfaceFound) {
103 + DLOG("Shell main surface found, placed at shell depth");
104 + placedParameters.depth = shellSurfaceDepth;
105 + placedParameters.input_mode = mir::input::InputReceptionMode::receives_all_input;
106 + mainShellSurfaceFound = true;

and

145 + static bool mainShellSurfaceFound = false;
146 + if (session.process_id() == 0 && !mainShellSurfaceFound) {
147 + Q_EMIT shellSurfaceCreated(surface);
148 + mainShellSurfaceFound = true;

There's no guarantee that all surfaces are created on the same thread. There should be a mutex/logk_guard around these mainShellSurfaceFound variables.

review: Needs Fixing

lp://qastaging/~gerboland/unity-mir/fix-compatibility-with-mir-0.1.8 updated on 2014-04-14

212. By Gerry Boland on 2014-04-14: place() can be called by several threads, so use locking around mainShellSurfaceFound. Standardize code style.

Revision history for this message

Gerry Boland (gerboland) wrote on 2014-04-14:

> Wouldn't a class member be better?

> There's no guarantee that all surfaces are created on the same thread. There
> should be a mutex/logk_guard around these mainShellSurfaceFound variables.

Good to know. Updated MR to suit.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-14:

FAILED: Continuous integration, rev:212
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/318/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/181/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/182/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/181/console