Merge lp:~raof/mir/input-methods-can-specify-foreign-parents into lp:mir
| Status: | Merged |
|---|---|
| Approved by: | Daniel van Vugt on 2015-07-01 |
| Approved revision: | 2527 |
| 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 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Alberto Aguirre | Approve on 2015-06-29 | ||
| PS Jenkins bot | continuous-integration | Approve on 2015-06-29 | |
| Alan Griffiths | Needs Fixing on 2015-06-16 | ||
| Kevin DuBois (community) | Approve on 2015-06-09 | ||
| Daniel van Vugt | Abstain on 2015-06-05 | ||
| Robert Carr (community) | 2015-05-01 | Approve on 2015-06-03 | |
|
Review via email:
|
|||
Commit Message
Allow input method surfaces to specify another client's surface as its parent
Description of the Change
Make persistent-
| 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?
| Robert Carr (robertcarr) wrote : | # |
Beyond that looking good
| 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'
| 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.
| Daniel van Vugt (vanvugt) wrote : | # |
I recall mpt (or someone) designed this in some detail already. Need to find the design...
| Robert Carr (robertcarr) wrote : | # |
I can't find where surfaces which are not input methods are prevented from using this.
| 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.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2520
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2521
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Robert Carr (robertcarr) wrote : | # |
I think the window type validation should happen on the server side not the client
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2524
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| 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...
| 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.
| 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.
| 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.
| 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_
| 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.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2524
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2525
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2526
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Kevin DuBois (kdub) wrote : | # |
https:/
| Alberto Aguirre (albaguirre) wrote : | # |
^--Results in stack smashing because MirSurface:
I think this warrants a libmirprotobuf ABI bump?
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2527
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2527
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| 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 mirclientplatfo
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 mirclientplatfo
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2531
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2532
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2534
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
*Needs Fixing*
You've tested and implemented the mir_surface_
*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?
| Chris Halse Rogers (raof) wrote : | # |
I actually have implemented mir_surface_
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.
| 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.
| 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:/
which will fix the original issue this branch had. Then no workarounds should be needed.
| Alberto Aguirre (albaguirre) wrote : | # |
mir 0.13.3 has landed, so you should be able to go back to r2526
| 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?
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2527
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel van Vugt (vanvugt) wrote : | # |
I abstain, but see we're ready for top approval now.

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