Mir

Merge lp:~raof/mir/input-methods-can-specify-foreign-parents into lp:mir

  1. input-methods-can-specify-foreign-parents
  2. Merge into development-branch
Proposed by Chris Halse Rogers
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 2721
Proposed branch: lp:~raof/mir/input-methods-can-specify-foreign-parents
Merge into: lp:mir
Prerequisite: lp:~raof/mir/persistent-surface-id
Diff against target: 628 lines (+331/-3)
19 files modified
include/client/mir_toolkit/mir_surface.h (+41/-0)
src/client/mir_surface.cpp (+12/-0)
src/client/mir_surface.h (+1/-0)
src/client/mir_surface_api.cpp (+30/-0)
src/client/symbols.map (+3/-0)
src/include/server/mir/frontend/shell.h (+3/-0)
src/include/server/mir/frontend/template_protobuf_message_processor.h (+4/-1)
src/protobuf/mir_protobuf.proto (+6/-0)
src/server/frontend/protobuf_message_processor.cpp (+3/-1)
src/server/frontend/session_mediator.cpp (+7/-0)
src/server/frontend/shell_wrapper.cpp (+6/-0)
src/server/frontend/shell_wrapper.h (+2/-0)
src/server/shell/default_persistent_surface_store.cpp (+10/-1)
src/server/shell/frontend_shell.cpp (+17/-0)
src/server/shell/frontend_shell.h (+2/-0)
tests/acceptance-tests/test_client_library.cpp (+45/-0)
tests/acceptance-tests/test_client_library_errors.cpp (+63/-0)
tests/acceptance-tests/test_client_surfaces.cpp (+74/-0)
tests/include/mir/test/doubles/mock_shell.h (+2/-0)
To merge this branch: bzr merge lp:~raof/mir/input-methods-can-specify-foreign-parents
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Needs Fixing
Kevin DuBois (community) Approve
Daniel van Vugt Abstain
Robert Carr (community) Approve
Review via email: mp+258001@code.launchpad.net

Commit message

Allow input method surfaces to specify another client's surface as its parent

Description of the change

Make persistent-surface-id actually useful by using it to let IM surfaces specify another client's surface as their parent.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2517
http://jenkins.qa.ubuntu.com/job/mir-ci/3672/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2261/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2260
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2211
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1669/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2211
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2211/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5118
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20122

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

>> + // XXX: This is ugly, but there doesn't seem to be a better way

Why not just let the exception propagate to the frontend to produce the error like we normally do? To control the error message?

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote :

Beyond that looking good

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Looks good.

review: Approve
Revision history for this message
Chris Halse Rogers (raof) wrote :

> >> + // XXX: This is ugly, but there doesn't seem to be a better way
>
> Why not just let the exception propagate to the frontend to produce the error
> like we normally do? To control the error message?

It's my understanding that if the frontend catches the error then the client is killed. That doesn't want to happen here - an ID no longer referring to an active surface is not a programming error, it'

Revision history for this message
Chris Halse Rogers (raof) wrote :

> >> + // XXX: This is ugly, but there doesn't seem to be a better way
>
> Why not just let the exception propagate to the frontend to produce the error
> like we normally do? To control the error message?

It's my understanding that if the frontend catches the error then the client is killed. That doesn't want to happen here - an ID no longer referring to an active surface is not a programming error, it's an unavoidable race condition. The IM will need to catch it and respond by not popping up a surface, but it shouldn't be fatal.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I recall mpt (or someone) designed this in some detail already. Need to find the design...

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote :

I can't find where surfaces which are not input methods are prevented from using this.

review: Needs Information
Revision history for this message
Chris Halse Rogers (raof) wrote :

> I can't find where surfaces which are not input methods are prevented from
> using this.

That's because there's currently nothing which enforces that. I'll update.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2520
http://jenkins.qa.ubuntu.com/job/mir-ci/3955/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2640
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/151
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2588
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/111/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2588
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2588/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5438
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20855

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2521
http://jenkins.qa.ubuntu.com/job/mir-ci/3957/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2642
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/153
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2590
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/113
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/113/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2590
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2590/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5440
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20857

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

review: Approve (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

I think the window type validation should happen on the server side not the client

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2524
http://jenkins.qa.ubuntu.com/job/mir-ci/3985/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2679/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/191
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2627/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/141/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2627/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

LGTM

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I remember in sprint discussions with tvoss/mpt at one point it was said that an input method does not operate on its parent, but some other "target" surface instead. Can't remember why.

Still need to check with mpt/tvoss if they have written down a design for it...

review: Needs Information
Revision history for this message
Chris Halse Rogers (raof) wrote :

Perhaps you're thinking of the fact that the input methods are out-of-process daemons with their own connection to Mir? They don't act on their own parents, because they don't have parents. They're parented on whatever client has the active text-box.

Which is precisely why we need this foreign-parent interface for them.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I don't mind. I prefer having one parent concept than two. Just keep in mind there's a risk mpt/tvoss might have specified it differently.

review: Abstain
Revision history for this message
Kevin DuBois (kdub) wrote :

It doesnt seem quite like a parent is involved, we're just attaching the input surface to the edge of another surface. Aside from that, lgtm.

Revision history for this message
Kevin DuBois (kdub) wrote :

Meh, I guess given the current way edge attachment works, it doesn't flummox me too much to call the method "mir_surface_spec_attach_to_foreign_parent". In a vacuum (apart from the spec document) mir_surface_spec_attach_to_surface_edge would make more sense I guess. Anyways +1

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> It doesnt seem quite like a parent is involved, we're just attaching the input
> surface to the edge of another surface. Aside from that, lgtm.

That's how menu's work too. And I assume they have similar properties, like if the parent is not visible anymore the foreign child should go away too.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2524
http://jenkins.qa.ubuntu.com/job/mir-ci/4060/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2791/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/304/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2739/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/216/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2739/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2525
http://jenkins.qa.ubuntu.com/job/mir-ci/4061/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2792/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/305
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2740/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/217/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2740/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2526
http://jenkins.qa.ubuntu.com/job/mir-ci/4062/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2793
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/306
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2741/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/218
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/218/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2741
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2741/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5577/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21082

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

https://code.launchpad.net/~kdub/mir/multiple-bufferstream-api/+merge/261123 is having the same problem with glmark.... both that MP and this one add a new api function...

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

^--Results in stack smashing because MirSurface::MirSurface allocates a mir::protobuf::SurfaceParameters object on the stack using the previous definition (as defined in mir 0.13.2, since glmark2-es2-mir links against libmirclient8 - i.e. no parent_persistent_id field) but the destructor invoked and the end of MirSurface::MirSurface constructor scope comes from the CI build of libmirprotobuf.so.0, which attempts to delete the parent_persistent_id_ field smashing the stack.

I think this warrants a libmirprotobuf ABI bump?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2527
http://jenkins.qa.ubuntu.com/job/mir-ci/4075/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2813
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/326
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2761/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/231
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/231/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2761
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2761/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5596/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21129

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2527
http://jenkins.qa.ubuntu.com/job/mir-ci/4076/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2814
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/327
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2762/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/232
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/232/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2762
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2762/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5597/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21131

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Gah, I guess loading libmirprotobuf.so.0 and libmirprotobuf.so.1 in the same process is more common than I thought since the mirclientplatformmesa module links against libmirclient.

The mako test runner will install the mesa platform modules from the CI build; when glmark2-es2-mir starts up (which links against libmirclient8), it probes mirclientplatformmesa which loads libmirclient9 (and subsequently libmirprotobuf.so.1) into the same process where libmirclient8 and libmirprotobuf.so.0 are already loaded....gah.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2531
http://jenkins.qa.ubuntu.com/job/mir-ci/4086/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2827/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/340/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2775/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/242/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2775/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2532
http://jenkins.qa.ubuntu.com/job/mir-ci/4087/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2828
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/341/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2776/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/243/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2776
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2776/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5609/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21158

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2534
http://jenkins.qa.ubuntu.com/job/mir-ci/4089/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2830
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/343
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2778
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/245
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/245/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2778
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2778/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5611
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21165

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*Needs Fixing*

You've tested and implemented the mir_surface_create() case, but not the mir_surface_apply_spec() case.

*Needs Discussion*

If we land 0.14 first (which means rebuilding all the downstreams to use libmirclient9) then we wouldn't need the large ugly hack. Right?

review: Needs Fixing
Revision history for this message
Chris Halse Rogers (raof) wrote :

I actually have implemented mir_surface_apply_spec, albeit negatively - any morph request into mir_surface_type_inputmethod will be denied (in FrontendShell), and you can't set a foreign parent for !mir_surface_type_inputmethod.

Now, as for the ‘can we get away without this’ question... I don't know. Once 0.14 has landed and we've rebuilt everything we own against libmirclient9 then this will pass CI without the RPC hack. I don't know whether it should or not, though.

Will *everything* definitely be rebuilt against libmirclient9? Will we need to backport any of this to vivid-overlay without rebuiling everything? Are there any third parties using libmirclient8?

Basically, do we have a requirement to not break things using libmirclient8? If yes, then we need the hack. If no, then we need to mark it as such by adding Breaks: libmirclient8 to libmirprotobuf0.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

692 + // TODO: Fish out a policy verification object that enforces the various invariants
693 + // in the surface spec requirements (eg: regular surface has no parent,
694 + // dialog may have a parent, gloss must have a parent).

That's the WindowManager policy as already "fished out" of AbstractShell. FrontendShell is only responsible for mapping "frontend" concepts to "shell" concepts.

review: Needs Fixing
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

We had a brief discussion on this - I'll make a 0.13.3 release that will include some version of this: https://code.launchpad.net/~albaguirre/mir/fix-1465883/+merge/262160
which will fix the original issue this branch had. Then no workarounds should be needed.

review: Needs Fixing
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

mir 0.13.3 has landed, so you should be able to go back to r2526

Revision history for this message
Chris Halse Rogers (raof) wrote :

> 692 + // TODO: Fish out a policy verification object that enforces the
> various invariants
> 693 + // in the surface spec requirements (eg: regular surface
> has no parent,
> 694 + // dialog may have a parent, gloss must have a parent).
>
> That's the WindowManager policy as already "fished out" of AbstractShell.
> FrontendShell is only responsible for mapping "frontend" concepts to "shell"
> concepts.

Hm. I think either I'm misunderstanding the current state of the code, or you're misunderstanding my intent.

My understanding is that we want to provide the shell authors with surfaces that have some guaranteed properties - regular surfaces have no parent, gloss surfaces definitely have a parent, tooltip surfaces are not fullscreen, etc. Likewise we claim in the client library that various surface types must have certain properties.

My understanding is that the WindowManager policy is expected to be provided *by* the shell; it's therefore too late for us to guarantee that shells only see well-formed requests.

We used to provide these guarantees by making malformed requests inexpressible in the client API, but that's no longer the case. Is there anywhere else where we guarantee these invariants to the shell?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2527
http://jenkins.qa.ubuntu.com/job/mir-ci/4226/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3046
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/566
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2994
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/382/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2994
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2994/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5807
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21553

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

review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

OK.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I abstain, but see we're ready for top approval now.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/client/mir_toolkit/mir_surface.h'
2--- include/client/mir_toolkit/mir_surface.h 2015-06-17 05:20:42 +0000
3+++ include/client/mir_toolkit/mir_surface.h 2015-06-29 06:21:38 +0000
4@@ -416,6 +416,30 @@
5 void mir_surface_spec_set_preferred_orientation(MirSurfaceSpec* spec, MirOrientationMode mode);
6
7 /**
8+ * Request that the created surface be attached to a surface of a different client.
9+ *
10+ * This is restricted to input methods, which need to attach their suggestion surface
11+ * to text entry widgets of other processes.
12+ *
13+ * \param [in] spec Specification to mutate
14+ * \param [in] parent A MirPersistentId reference to the parent surface
15+ * \param [in] attachment_rect A rectangle specifying the region (in parent surface coordinates)
16+ * that the created surface should be attached to.
17+ * \param [in] edge The preferred edge direction to attach to. Use
18+ * mir_edge_attachment_any for no preference.
19+ * \return False if the operation is invalid for this surface type.
20+ *
21+ * \note If the parent surface becomes invalid before mir_surface_create() is processed,
22+ * it will return an invalid surface. If the parent surface is valid at the time
23+ * mir_surface_create() is called but is later closed, this surface will also receive
24+ * a close event.
25+ */
26+bool mir_surface_spec_attach_to_foreign_parent(MirSurfaceSpec* spec,
27+ MirPersistentId* parent,
28+ MirRectangle* attachment_rect,
29+ MirEdgeAttachment edge);
30+
31+/**
32 * Set the requested state.
33 * \param [in] spec Specification to mutate
34 * \param [in] mode Requested state
35@@ -694,6 +718,23 @@
36 */
37 void mir_persistent_id_release(MirPersistentId* id);
38
39+/**
40+ * \brief Get a string representation of a MirSurfaceId
41+ * \param [in] id The ID to serialise
42+ * \return A string representation of id. This string is owned by the MirSurfaceId,
43+ * and must not be freed by the caller.
44+ *
45+ * \see mir_surface_id_from_string
46+ */
47+char const* mir_persistent_id_as_string(MirPersistentId* id);
48+
49+/**
50+ * \brief Deserialise a string representation of a MirSurfaceId
51+ * \param [in] string_representation Serialised representation of the ID
52+ * \return The deserialised MirSurfaceId
53+ */
54+MirPersistentId* mir_persistent_id_from_string(char const* string_representation);
55+
56 #ifdef __cplusplus
57 }
58 /**@}*/
59
60=== modified file 'src/client/mir_surface.cpp'
61--- src/client/mir_surface.cpp 2015-06-17 18:24:42 +0000
62+++ src/client/mir_surface.cpp 2015-06-29 06:21:38 +0000
63@@ -107,6 +107,12 @@
64 if (parent.is_set() && parent.value() != nullptr)
65 message->set_parent_id(parent.value()->id());
66
67+ if (parent_id)
68+ {
69+ auto id = message->mutable_parent_persistent_id();
70+ id->set_value(parent_id->as_string());
71+ }
72+
73 if (aux_rect.is_set())
74 {
75 message->mutable_aux_rect()->set_left(aux_rect.value().left);
76@@ -648,6 +654,12 @@
77 if (spec.parent.is_set() && spec.parent.value())
78 surface_specification->set_parent_id(spec.parent.value()->id());
79
80+ if (spec.parent_id)
81+ {
82+ auto id = surface_specification->mutable_parent_persistent_id();
83+ id->set_value(spec.parent_id->as_string());
84+ }
85+
86 if (spec.aux_rect.is_set())
87 {
88 auto const rect = surface_specification->mutable_aux_rect();
89
90=== modified file 'src/client/mir_surface.h'
91--- src/client/mir_surface.h 2015-06-17 18:24:42 +0000
92+++ src/client/mir_surface.h 2015-06-29 06:21:38 +0000
93@@ -86,6 +86,7 @@
94 mir::optional_value<MirOrientationMode> pref_orientation;
95
96 mir::optional_value<MirSurface*> parent;
97+ std::unique_ptr<MirPersistentId> parent_id;
98 mir::optional_value<MirRectangle> aux_rect;
99 mir::optional_value<MirEdgeAttachment> edge_attachment;
100
101
102=== modified file 'src/client/mir_surface_api.cpp'
103--- src/client/mir_surface_api.cpp 2015-06-17 05:20:42 +0000
104+++ src/client/mir_surface_api.cpp 2015-06-29 06:21:38 +0000
105@@ -632,3 +632,33 @@
106 {
107 delete id;
108 }
109+
110+bool mir_surface_spec_attach_to_foreign_parent(MirSurfaceSpec* spec,
111+ MirPersistentId* parent,
112+ MirRectangle* attachment_rect,
113+ MirEdgeAttachment edge)
114+{
115+ mir::require(mir_persistent_id_is_valid(parent));
116+ mir::require(attachment_rect != nullptr);
117+
118+ if (!spec->type.is_set() ||
119+ spec->type.value() != mir_surface_type_inputmethod)
120+ {
121+ return false;
122+ }
123+
124+ spec->parent_id = std::make_unique<MirPersistentId>(*parent);
125+ spec->aux_rect = *attachment_rect;
126+ spec->edge_attachment = edge;
127+ return true;
128+}
129+
130+char const* mir_persistent_id_as_string(MirPersistentId *id)
131+{
132+ return id->as_string().c_str();
133+}
134+
135+MirPersistentId* mir_persistent_id_from_string(char const* id_string)
136+{
137+ return new MirPersistentId{id_string};
138+}
139
140=== modified file 'src/client/symbols.map'
141--- src/client/symbols.map 2015-06-25 11:26:34 +0000
142+++ src/client/symbols.map 2015-06-29 06:21:38 +0000
143@@ -78,6 +78,8 @@
144 mir_omnidirectional_resize_cursor_name;
145 mir_open_hand_cursor_name;
146 mir_orientation_event_get_direction;
147+ mir_persistent_id_as_string;
148+ mir_persistent_id_from_string;
149 mir_persistent_id_is_valid;
150 mir_persistent_id_release;
151 mir_platform_message_create;
152@@ -130,6 +132,7 @@
153 mir_surface_set_state;
154 mir_surface_set_swapinterval;
155 mir_surface_spec_release;
156+ mir_surface_spec_attach_to_foreign_parent;
157 mir_surface_spec_set_buffer_usage;
158 mir_surface_spec_set_fullscreen_on_output;
159 mir_surface_spec_set_height;
160
161=== modified file 'src/include/server/mir/frontend/shell.h'
162--- src/include/server/mir/frontend/shell.h 2015-06-17 05:20:42 +0000
163+++ src/include/server/mir/frontend/shell.h 2015-06-29 06:21:38 +0000
164@@ -33,6 +33,7 @@
165 {
166 struct SurfaceCreationParameters;
167 struct PromptSessionCreationParameters;
168+class Surface;
169 }
170 namespace shell { class SurfaceSpecification; }
171
172@@ -63,7 +64,9 @@
173 virtual SurfaceId create_surface(std::shared_ptr<Session> const& session, scene::SurfaceCreationParameters const& params) = 0;
174 virtual void modify_surface(std::shared_ptr<Session> const& session, SurfaceId surface, shell::SurfaceSpecification const& modifications) = 0;
175 virtual void destroy_surface(std::shared_ptr<Session> const& session, SurfaceId surface) = 0;
176+
177 virtual std::string persistent_id_for(std::shared_ptr<Session> const& session, SurfaceId surface) = 0;
178+ virtual std::shared_ptr<scene::Surface> surface_for_id(std::string const& serialised_id) = 0;
179
180 virtual int set_surface_attribute(
181 std::shared_ptr<Session> const& session,
182
183=== modified file 'src/include/server/mir/frontend/template_protobuf_message_processor.h'
184--- src/include/server/mir/frontend/template_protobuf_message_processor.h 2015-06-17 05:20:42 +0000
185+++ src/include/server/mir/frontend/template_protobuf_message_processor.h 2015-06-29 06:21:38 +0000
186@@ -27,6 +27,7 @@
187 #include <boost/exception/diagnostic_information.hpp>
188
189 #include <memory>
190+#include <string>
191
192 namespace mir
193 {
194@@ -79,7 +80,9 @@
195 }
196 catch (std::exception const& x)
197 {
198- result_message.set_error(boost::diagnostic_information(x));
199+ using namespace std::literals::string_literals;
200+ result_message.set_error("Error processing request: "s +
201+ x.what() + "\nInternal error details: " + boost::diagnostic_information(x));
202 self->send_response(invocation.id(), &result_message);
203 }
204 }
205
206=== modified file 'src/protobuf/mir_protobuf.proto'
207--- src/protobuf/mir_protobuf.proto 2015-06-25 19:29:44 +0000
208+++ src/protobuf/mir_protobuf.proto 2015-06-29 06:21:38 +0000
209@@ -35,6 +35,8 @@
210 optional int32 height_inc = 18;
211 optional SurfaceAspectRatio min_aspect = 19;
212 optional SurfaceAspectRatio max_aspect = 20;
213+
214+ optional PersistentSurfaceId parent_persistent_id = 21;
215 }
216
217 message SurfaceAspectRatio
218@@ -63,11 +65,15 @@
219 optional int32 min_height = 14;
220 optional int32 max_width = 15;
221 optional int32 max_height = 16;
222+
223 optional int32 width_inc = 17;
224 optional int32 height_inc = 18;
225 optional SurfaceAspectRatio min_aspect = 19;
226 optional SurfaceAspectRatio max_aspect = 20;
227+
228 repeated StreamConfiguration stream = 21;
229+
230+ optional PersistentSurfaceId parent_persistent_id = 22;
231 }
232
233 message StreamConfiguration {
234
235=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
236--- src/server/frontend/protobuf_message_processor.cpp 2015-06-17 19:16:03 +0000
237+++ src/server/frontend/protobuf_message_processor.cpp 2015-06-29 06:21:38 +0000
238@@ -138,7 +138,9 @@
239 }
240 catch (std::exception const& x)
241 {
242- result_message->set_error(boost::diagnostic_information(x));
243+ using namespace std::literals;
244+ result_message->set_error("Error processing request: "s +
245+ x.what() + "\nInternal error details: " + boost::diagnostic_information(x));
246 callback->Run();
247 }
248 }
249
250=== modified file 'src/server/frontend/session_mediator.cpp'
251--- src/server/frontend/session_mediator.cpp 2015-06-19 12:35:35 +0000
252+++ src/server/frontend/session_mediator.cpp 2015-06-29 06:21:38 +0000
253@@ -203,6 +203,13 @@
254 if (request->has_parent_id())
255 params.with_parent_id(SurfaceId{request->parent_id()});
256
257+
258+ if (request->has_parent_persistent_id())
259+ {
260+ auto persistent_id = request->parent_persistent_id().value();
261+ params.parent = shell->surface_for_id(persistent_id);
262+ }
263+
264 if (request->has_aux_rect())
265 {
266 params.with_aux_rect(geom::Rectangle{
267
268=== modified file 'src/server/frontend/shell_wrapper.cpp'
269--- src/server/frontend/shell_wrapper.cpp 2015-06-17 05:20:42 +0000
270+++ src/server/frontend/shell_wrapper.cpp 2015-06-29 06:21:38 +0000
271@@ -19,6 +19,7 @@
272 #include "shell_wrapper.h"
273
274 namespace mf = mir::frontend;
275+namespace ms = mir::scene;
276
277 std::shared_ptr<mf::Session> mf::ShellWrapper::open_session(
278 pid_t client_pid,
279@@ -73,6 +74,11 @@
280 return wrapped->persistent_id_for(session, surface);
281 }
282
283+std::shared_ptr<ms::Surface> mf::ShellWrapper::surface_for_id(std::string const& serialised_id)
284+{
285+ return wrapped->surface_for_id(serialised_id);
286+}
287+
288 int mf::ShellWrapper::set_surface_attribute(
289 std::shared_ptr<Session> const& session,
290 SurfaceId surface_id,
291
292=== modified file 'src/server/frontend/shell_wrapper.h'
293--- src/server/frontend/shell_wrapper.h 2015-06-17 05:20:42 +0000
294+++ src/server/frontend/shell_wrapper.h 2015-06-29 06:21:38 +0000
295@@ -59,6 +59,8 @@
296
297 std::string persistent_id_for(std::shared_ptr<Session> const& session, SurfaceId surface) override;
298
299+ std::shared_ptr<scene::Surface> surface_for_id(std::string const& serialised_id) override;
300+
301 int set_surface_attribute(
302 std::shared_ptr<Session> const& session,
303 SurfaceId surface_id,
304
305=== modified file 'src/server/shell/default_persistent_surface_store.cpp'
306--- src/server/shell/default_persistent_surface_store.cpp 2015-05-29 03:30:25 +0000
307+++ src/server/shell/default_persistent_surface_store.cpp 2015-06-29 06:21:38 +0000
308@@ -87,5 +87,14 @@
309
310 std::shared_ptr<ms::Surface> msh::DefaultPersistentSurfaceStore::surface_for_id(Id const& id) const
311 {
312- return (*store)[id];
313+ try
314+ {
315+ return (*store)[id];
316+ }
317+ catch (std::out_of_range& err)
318+ {
319+ using namespace std::literals;
320+ BOOST_THROW_EXCEPTION(std::out_of_range(
321+ "Lookup for surface with ID: "s + id.serialize_to_string() + " failed."));
322+ }
323 }
324
325=== modified file 'src/server/shell/frontend_shell.cpp'
326--- src/server/shell/frontend_shell.cpp 2015-06-17 05:20:42 +0000
327+++ src/server/shell/frontend_shell.cpp 2015-06-29 06:21:38 +0000
328@@ -25,6 +25,8 @@
329 #include "mir/scene/surface_creation_parameters.h"
330 #include "mir/scene/prompt_session.h"
331
332+#include <boost/throw_exception.hpp>
333+
334 namespace mf = mir::frontend;
335 namespace ms = mir::scene;
336 namespace msh = mir::shell::detail;
337@@ -74,6 +76,15 @@
338
339 auto populated_params = params;
340
341+ // TODO: Fish out a policy verification object that enforces the various invariants
342+ // in the surface spec requirements (eg: regular surface has no parent,
343+ // dialog may have a parent, gloss must have a parent).
344+ if (populated_params.parent.lock() &&
345+ populated_params.type.value() != mir_surface_type_inputmethod)
346+ {
347+ BOOST_THROW_EXCEPTION(std::invalid_argument("Foreign parents may only be set on surfaces of type mir_surface_type_inputmethod"));
348+ }
349+
350 if (populated_params.parent_id.is_set())
351 populated_params.parent = scene_session->surface(populated_params.parent_id.value());
352
353@@ -107,6 +118,12 @@
354 return surface_store->id_for_surface(surface).serialize_to_string();
355 }
356
357+std::shared_ptr<ms::Surface> msh::FrontendShell::surface_for_id(std::string const& serialized_id)
358+{
359+ PersistentSurfaceStore::Id const id{serialized_id};
360+ return surface_store->surface_for_id(id);
361+}
362+
363 int msh::FrontendShell::set_surface_attribute(
364 std::shared_ptr<mf::Session> const& session,
365 mf::SurfaceId surface_id,
366
367=== modified file 'src/server/shell/frontend_shell.h'
368--- src/server/shell/frontend_shell.h 2015-06-17 05:20:42 +0000
369+++ src/server/shell/frontend_shell.h 2015-06-29 06:21:38 +0000
370@@ -71,6 +71,8 @@
371
372 std::string persistent_id_for(std::shared_ptr<mf::Session> const& session, mf::SurfaceId surface) override;
373
374+ std::shared_ptr<ms::Surface> surface_for_id(std::string const& serialized_id) override;
375+
376 int set_surface_attribute(
377 std::shared_ptr<mf::Session> const& session,
378 mf::SurfaceId surface_id,
379
380=== modified file 'tests/acceptance-tests/test_client_library.cpp'
381--- tests/acceptance-tests/test_client_library.cpp 2015-06-25 21:34:27 +0000
382+++ tests/acceptance-tests/test_client_library.cpp 2015-06-29 06:21:38 +0000
383@@ -913,3 +913,48 @@
384 mir_persistent_id_release(surface_id);
385 mir_connection_release(connection);
386 }
387+
388+TEST_F(ClientLibrary, input_method_can_specify_foreign_surface_id)
389+{
390+ auto first_client = mir_connect_sync(new_connection().c_str(), "Regular Client");
391+
392+ auto surface_spec = mir_connection_create_spec_for_normal_surface(first_client,
393+ 800, 600,
394+ mir_pixel_format_argb_8888);
395+ auto main_surface = mir_surface_create_sync(surface_spec);
396+ mir_surface_spec_release(surface_spec);
397+
398+ ASSERT_THAT(main_surface, IsValid());
399+
400+ auto main_surface_id = mir_surface_request_persistent_id_sync(main_surface);
401+ ASSERT_TRUE(mir_persistent_id_is_valid(main_surface_id));
402+
403+ // Serialise & deserialise the ID
404+ auto im_parent_id = mir_persistent_id_from_string(mir_persistent_id_as_string(main_surface_id));
405+
406+ auto im_client = mir_connect_sync(new_connection().c_str(), "IM Client");
407+ surface_spec = mir_connection_create_spec_for_input_method(im_client,
408+ 200, 20,
409+ mir_pixel_format_argb_8888);
410+ MirRectangle attachment_rect {
411+ 200,
412+ 200,
413+ 10,
414+ 10
415+ };
416+ mir_surface_spec_attach_to_foreign_parent(surface_spec,
417+ im_parent_id,
418+ &attachment_rect,
419+ mir_edge_attachment_any);
420+ auto im_surface = mir_surface_create_sync(surface_spec);
421+
422+ EXPECT_THAT(im_surface, IsValid());
423+
424+ mir_surface_spec_release(surface_spec);
425+ mir_persistent_id_release(main_surface_id);
426+ mir_persistent_id_release(im_parent_id);
427+ mir_surface_release_sync(main_surface);
428+ mir_surface_release_sync(im_surface);
429+ mir_connection_release(first_client);
430+ mir_connection_release(im_client);
431+}
432
433=== modified file 'tests/acceptance-tests/test_client_library_errors.cpp'
434--- tests/acceptance-tests/test_client_library_errors.cpp 2015-06-25 03:00:08 +0000
435+++ tests/acceptance-tests/test_client_library_errors.cpp 2015-06-29 06:21:38 +0000
436@@ -247,6 +247,38 @@
437 mir_connection_release(connection);
438 }
439
440+TEST_F(ClientLibraryErrors, passing_invalid_parent_id_to_surface_create)
441+{
442+ using namespace testing;
443+
444+ auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
445+
446+ ASSERT_THAT(connection, IsValid());
447+
448+ // An ID that parses as valid, but doesn't correspond to any
449+ auto invalid_id = mir_persistent_id_from_string("05f223a2-39e5-48b9-9416-b0ce837351b6");
450+
451+ auto spec = mir_connection_create_spec_for_input_method(connection,
452+ 200, 200,
453+ mir_pixel_format_argb_8888);
454+ MirRectangle rect{
455+ 100,
456+ 100,
457+ 10,
458+ 10
459+ };
460+ mir_surface_spec_attach_to_foreign_parent(spec, invalid_id, &rect, mir_edge_attachment_any);
461+
462+ auto surface = mir_surface_create_sync(spec);
463+ EXPECT_THAT(surface, Not(IsValid()));
464+ EXPECT_THAT(mir_surface_get_error_message(surface), MatchesRegex(".*Lookup.*failed.*"));
465+
466+ mir_persistent_id_release(invalid_id);
467+ mir_surface_spec_release(spec);
468+ mir_surface_release_sync(surface);
469+ mir_connection_release(connection);
470+}
471+
472 using ClientLibraryErrorsDeathTest = ClientLibraryErrors;
473
474
475@@ -287,3 +319,34 @@
476
477 mir_connection_release(connection);
478 }
479+
480+TEST_F(ClientLibraryErrorsDeathTest, surface_spec_attaching_invalid_parent_id)
481+{
482+ auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
483+
484+ auto spec = mir_connection_create_spec_for_input_method(connection, 100, 100, mir_pixel_format_argb_8888);
485+
486+ MirRectangle rect{
487+ 100,
488+ 100,
489+ 10,
490+ 10
491+ };
492+ EXPECT_DEATH(mir_surface_spec_attach_to_foreign_parent(spec, nullptr, &rect, mir_edge_attachment_any), "");
493+
494+ mir_connection_release(connection);
495+}
496+
497+TEST_F(ClientLibraryErrorsDeathTest, surface_spec_attaching_invalid_rectangle)
498+{
499+ auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
500+
501+ auto spec = mir_connection_create_spec_for_input_method(connection, 100, 100, mir_pixel_format_argb_8888);
502+
503+ auto id = mir_persistent_id_from_string("fa69b2e9-d507-4005-be61-5068f40a5aec");
504+
505+ EXPECT_DEATH(mir_surface_spec_attach_to_foreign_parent(spec, id, nullptr, mir_edge_attachment_any), "");
506+
507+ mir_persistent_id_release(id);
508+ mir_connection_release(connection);
509+}
510
511=== modified file 'tests/acceptance-tests/test_client_surfaces.cpp'
512--- tests/acceptance-tests/test_client_surfaces.cpp 2015-06-25 03:00:08 +0000
513+++ tests/acceptance-tests/test_client_surfaces.cpp 2015-06-29 06:21:38 +0000
514@@ -19,9 +19,15 @@
515 #include "mir_toolkit/mir_client_library.h"
516 #include "mir_toolkit/debug/surface.h"
517
518+#include "mir/test/doubles/mock_window_manager.h"
519+
520+#include "mir/scene/session.h"
521+#include "mir/geometry/rectangle.h"
522+
523 #include "mir_test_framework/connected_client_headless_server.h"
524 #include "mir_test_framework/any_surface.h"
525 #include "mir/test/validity_matchers.h"
526+#include "mir/test/fake_shared.h"
527
528 #include <gmock/gmock.h>
529 #include <gtest/gtest.h>
530@@ -30,6 +36,11 @@
531 #include <mutex>
532
533 namespace mtf = mir_test_framework;
534+namespace mtd = mir::test::doubles;
535+namespace msh = mir::shell;
536+namespace ms = mir::scene;
537+namespace mt = mir::test;
538+namespace mg = mir::geometry;
539
540 namespace
541 {
542@@ -79,6 +90,17 @@
543 mir_buffer_usage_hardware,
544 mir_display_output_id_invalid
545 };
546+
547+ void SetUp() override
548+ {
549+ server.override_the_window_manager_builder([this](msh::FocusController*)
550+ {
551+ return mt::fake_shared(window_manager);
552+ });
553+ ConnectedClientHeadlessServer::SetUp();
554+ }
555+
556+ testing::NiceMock<mtd::MockWindowManager> window_manager;
557 };
558
559 extern "C" void create_surface_callback(MirSurface* surface, void * context)
560@@ -328,3 +350,55 @@
561
562 mir_surface_release_sync(surf);
563 }
564+
565+TEST_F(ClientSurfaces, input_methods_get_corret_parent_coordinates)
566+{
567+ using namespace testing;
568+
569+ mg::Rectangle const server_rect{mg::Point{100, 100}, mg::Size{10, 10}};
570+ MirRectangle client_rect{ 100, 100, 10, 10 };
571+
572+ auto const edge_attachment = mir_edge_attachment_vertical;
573+
574+ std::shared_ptr<ms::Surface> parent_surface;
575+ InSequence seq;
576+ EXPECT_CALL(window_manager, add_surface(_,_,_)).WillOnce(Invoke([&parent_surface](auto session, auto params, auto builder)
577+ {
578+ auto id = builder(session, params);
579+ parent_surface = session->surface(id);
580+ return id;
581+ }));
582+ EXPECT_CALL(window_manager, add_surface(_,_,_)).WillOnce(Invoke([&parent_surface, server_rect, edge_attachment](auto session, auto params, auto builder)
583+ {
584+ EXPECT_THAT(params.parent.lock(), Eq(parent_surface));
585+ EXPECT_TRUE(params.aux_rect.is_set());
586+ EXPECT_THAT(params.aux_rect.value(), Eq(server_rect));
587+ EXPECT_TRUE(params.edge_attachment.is_set());
588+ EXPECT_THAT(params.edge_attachment.value(), Eq(edge_attachment));
589+
590+ return builder(session, params);
591+ }));
592+
593+ auto surface = mtf::make_any_surface(connection);
594+
595+ auto parent_id = mir_surface_request_persistent_id_sync(surface);
596+
597+ auto im_connection = mir_connect_sync(new_connection().c_str(), "Mock IM connection");
598+ ASSERT_THAT(im_connection, IsValid());
599+
600+ auto spec = mir_connection_create_spec_for_input_method(im_connection, 100, 20,
601+ mir_pixel_format_abgr_8888);
602+ ASSERT_THAT(spec, NotNull());
603+
604+ mir_surface_spec_attach_to_foreign_parent(spec, parent_id, &client_rect, edge_attachment);
605+
606+ mir_persistent_id_release(parent_id);
607+
608+ auto im = mir_surface_create_sync(spec);
609+ mir_surface_spec_release(spec);
610+
611+ mir_surface_release_sync(im);
612+ mir_surface_release_sync(surface);
613+
614+ mir_connection_release(im_connection);
615+}
616
617=== modified file 'tests/include/mir/test/doubles/mock_shell.h'
618--- tests/include/mir/test/doubles/mock_shell.h 2015-06-17 05:20:42 +0000
619+++ tests/include/mir/test/doubles/mock_shell.h 2015-06-29 06:21:38 +0000
620@@ -55,6 +55,8 @@
621 MOCK_METHOD3(modify_surface, void(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId, shell::SurfaceSpecification const&));
622 MOCK_METHOD2(destroy_surface, void(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId));
623 MOCK_METHOD2(persistent_id_for, std::string(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId));
624+ MOCK_METHOD1(surface_for_id, std::shared_ptr<scene::Surface>(std::string const&));
625+
626
627
628 MOCK_METHOD4(set_surface_attribute, int(

Subscribers

People subscribed via source and target branches