Merge lp:~raof/mir/the-least-dirty-thing into lp:mir
Status: | Merged |
---|---|
Approved by: | Alberto Aguirre on 2014-10-27 |
Approved revision: | 1870 |
Merged at revision: | 2005 |
Proposed branch: | lp:~raof/mir/the-least-dirty-thing |
Merge into: | lp:mir |
Diff against target: |
1865 lines (+1026/-46) 53 files modified
client-ABI-sha1sums (+1/-1) debian/control (+46/-0) debian/libmirclient-debug-extension-dev.install (+3/-0) debian/libmirclient-debug-extension1.install (+1/-0) debian/libmirclient-dev.install (+1/-1) include/client/mir_toolkit/debug/surface.h (+20/-0) include/platform/mir/options/configuration.h (+1/-0) include/server/mir/default_server_configuration.h (+3/-0) include/server/mir/scene/coordinate_translator.h (+66/-0) platform-ABI-sha1sums (+1/-1) server-ABI-sha1sums (+3/-2) src/client/CMakeLists.txt (+52/-2) src/client/mir_connection.cpp (+3/-1) src/client/mir_connection.h (+1/-0) src/client/mir_debug_api.cpp (+39/-0) src/client/mir_surface.cpp (+63/-0) src/client/mir_surface.h (+16/-1) src/client/mir_surface_api.cpp (+2/-3) src/client/mirclient-debug-extension.pc.in (+11/-0) src/client/mirclient.pc.in (+5/-3) src/client/symbols-debug.map (+5/-0) src/common/protobuf/mir_protobuf.proto (+17/-0) src/common/symbols.map (+54/-0) src/include/server/mir/frontend/unsupported_feature_exception.h (+41/-0) src/platform/options/default_configuration.cpp (+4/-1) src/platform/symbols.map (+1/-0) src/server/frontend/CMakeLists.txt (+1/-0) src/server/frontend/default_configuration.cpp (+21/-8) src/server/frontend/default_ipc_factory.cpp (+6/-3) src/server/frontend/default_ipc_factory.h (+8/-1) src/server/frontend/protobuf_message_processor.cpp (+37/-0) src/server/frontend/session_mediator.cpp (+30/-2) src/server/frontend/session_mediator.h (+16/-2) src/server/frontend/unsupported_coordinate_translator.cpp (+32/-0) src/server/frontend/unsupported_coordinate_translator.h (+37/-0) src/server/scene/CMakeLists.txt (+1/-0) src/server/scene/default_configuration.cpp (+11/-0) src/server/scene/default_coordinate_translator.cpp (+32/-0) src/server/scene/default_coordinate_translator.h (+37/-0) src/server/symbols.map (+6/-0) tests/acceptance-tests/CMakeLists.txt (+2/-0) tests/acceptance-tests/test_client_library.cpp (+0/-1) tests/acceptance-tests/test_client_surface_events.cpp (+1/-1) tests/acceptance-tests/test_client_surfaces.cpp (+1/-1) tests/acceptance-tests/test_custom_input_dispatcher.cpp (+2/-2) tests/acceptance-tests/test_debug_api.cpp (+231/-0) tests/include/mir_test_doubles/mock_coordinate_translator.h (+42/-0) tests/include/mir_test_doubles/mock_surface.h (+2/-1) tests/integration-tests/CMakeLists.txt (+1/-0) tests/integration-tests/test_stale_frames.cpp (+1/-1) tests/unit-tests/frontend/test_session_mediator.cpp (+5/-5) tests/unit-tests/frontend/test_session_mediator_android.cpp (+1/-1) tests/unit-tests/frontend/test_session_mediator_mesa.cpp (+2/-1) |
To merge this branch: | bzr merge lp:~raof/mir/the-least-dirty-thing |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve on 2014-10-27 | |
Alberto Aguirre (community) | Approve on 2014-10-27 | ||
Alan Griffiths | Approve on 2014-10-14 | ||
Daniel van Vugt | 2014-08-21 | Abstain on 2014-10-14 | |
Andreas Pokorny (community) | Approve on 2014-09-24 | ||
Alexandros Frantzis (community) | Approve on 2014-09-19 | ||
Kevin DuBois (community) | Abstain on 2014-09-08 | ||
Review via email:
|
Commit message
Add a debug interface to translate from surface to screen coordinates.
This is the Mir part of the infrastructure for Autopilot to determine the screen location of widgets, for full-stack testing.
This is hidden behind a --debug server option, to make it absolutely clear that applications cannot depend on this functionality outside of a constrained environment.
(See also https:/
Description of the change
Add a translate_
This can be somewhat cleaned up when we've got real support for extensions, but should do for now.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1849
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://
Daniel van Vugt (vanvugt) wrote : | # |
Minor observations so far:
(1) 18 +Package: libmirclient-debug
is potentially confusing with the existing convention: packagename-dbg
so I think the dash needs removing. Something like "libmirclientte
(2) It seems we're going backwards in code bloat faster than I can keep up with fixing it. Can we avoid this part?
176 +
177 +# TODO: Extension apparatus so that mirclient-debug can be a MODULE and not
178 +# duplicate (almost) the entirity of mirclient
179 +add_library(
180 + mirclient-debug SHARED
181 +
182 + mir_debug_api.cpp
183 + $<TARGET_
184 +)
(3) The amount of coupling we have is a bit excessive. Or is that an existing issue imposed by the existing class design?; "make_mediator()"
Kevin DuBois (kdub) wrote : | # |
36 + Interfaces will come and go; end-user applications SHOULD NOT be linked
37 + against this library.
"interfaces come and go" is true of all the public interfaces... We should say something stronger like:
"Not all mir servers run with the capability to service this api; end-user applications SHOULD NOT depend on the functionality of this api."
Chris Halse Rogers (raof) wrote : | # |
> Minor observations so far:
>
> (1) 18 +Package: libmirclient-debug
> is potentially confusing with the existing convention: packagename-dbg
> so I think the dash needs removing. Something like "libmirclientte
> since it's really not about "debugging" Mir at all.
It actually is a bit about debugging Mir; at least, I've used the symbols in there to debug Mir and low-level client bits like XMir.
>
> (2) It seems we're going backwards in code bloat faster than I can keep up
> with fixing it. Can we avoid this part?
Not without either (a) exposing all the libmirclient internal symbols, or (b) adding in a real extension mechanism.
Since this library isn't expected to be installed on anything but development machines I wouldn't care if it was 100MB. I don't think we should spend any significant effort caring about its size.
We'll want an actual extension mechanism at some point in the future. At that point this can be switched to using that and libmirclient-
>
> (3) The amount of coupling we have is a bit excessive. Or is that an existing
> issue imposed by the existing class design?; "make_mediator()"
Yeah, that's the existing class design.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1852
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://
Alexandros Frantzis (afrantzis) wrote : | # |
> "Not all mir servers run with the capability to service this api; end-user applications SHOULD NOT depend on the functionality of this api."
I agree with Kevin. I don't think it's worth having a different debugging version of the client library. Until we get an extension mechanism, I would prefer just exposing the debug APIs in our normal client library builds (like we have been doing up to now).
Chris Halse Rogers (raof) wrote : | # |
From the client perspective it *is* an extension; libmirclient-
I can, of course, fold those symbols back into libmirclient. I think it's useful to have the non-core things split out a bit, though.
Daniel van Vugt (vanvugt) wrote : | # |
OK, I still have the same concerns as mentioned above. But nothing strictly blocking. That's not to say this is a proper review.
Chris Halse Rogers (raof) wrote : | # |
So, I'm not sure what needs to be done here in order to make this acceptable.
Do I need to fold the three debugging APIs back into libmirclient rather than their own DSO? Do I need to reduce the on-disc size of libmirclient-
Alexandros Frantzis (afrantzis) wrote : | # |
> Do I need to fold the three debugging APIs back into libmirclient rather than their own DSO?
> Do I need to reduce the on-disc size of libmirclient-
> infrastructure for extension loading in libmirclient?
I am fine with folding the debugging APIs back into libmirclient until we get a proper extension mechanism.
Kevin DuBois (kdub) wrote : | # |
So, the stronger wording seems better, so that 'needs fixing' is resolved. As for having two libraries vs the one, I'm on the fence, so I won't object. I somewhat like that there's less temptation to roll these functions into the public api.
As for the DebuggingSessio
OTOH, SessionMediator already has a pretty bloated constructor... but I'd rather have one bloated constructor on SessionMediator than a bloated constructor on both SessionMediator and DebuggingSessio
Chris Halse Rogers (raof) wrote : | # |
Yeah, I could make an extra interface for SessionMediator to depend on. I'll ask the list first about the general “how many DSOs should this be” question and then see what that version of SessionMediator would look like.
A bit ugly, because we're adding even more special-purpose code to a general interface, but until it gains an extension mechanism... :)
Andreas Pokorny (andreas-pokorny) wrote : | # |
In src/server/
743 + response-
744 + response-
You have to transform x and y according to the surface orientation here (and in theory surface transformation but i doubt we want to be super correct - makes me wonder whether unity8 overrides that information stored at the surface? ). Furthermore in the nested case you have to forward that request to the system compositor.
So dl-load the debug extension in mirserver, and fail unlikely if not available.
Regarding the library vs internal topic. I prefer the separate library approach already implemented in this MP
kevin gunn (kgunn72) wrote : | # |
As long as it's a dbug module that's not going to be part of a release image & you have to install when you want. Then I prefer the bloat-on-debug hit to keep it separated from regular ol' libmirclient.
however, is there a plan or mechanism to help resynch this with libmirclient where the code is duplicated ?
iirc, we're adding primarily for AP testing... so we'd want to keep the the functionality the same in the debug client, as to ensure the testing/config is valid and we trust the results.
Chris Halse Rogers (raof) wrote : | # |
Clients wanting to use the debug extensions link against libmirclient as normal, and *also* libmirclient-
Andreas: Urgh, yeah, I guess so. I _think_ we have enough information in the nested case to do the translation without bouncing up to the host server, but it'll be safer to do so, yeah.
Kevin DuBois (kdub) wrote : | # |
Its looking like we're coalescing around 2 DSO's then, everyone? (a second debug library is good by me)
Chris Halse Rogers (raof) wrote : | # |
Andreas: Actually, I don't think we *do* need to forward to the system compositor - the nested compositor ensures it has a logical coordinate space that's identical to the system compositor's by means of the display configuration.
And we don't actually use the surface orientation anywhere, so I don't think we need to transform according to that either?
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1855
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1857
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 : | # |
PASSED: Continuous integration, rev:1858
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://
Andreas Pokorny (andreas-pokorny) wrote : | # |
> Andreas: Actually, I don't think we *do* need to forward to the system
> compositor - the nested compositor ensures it has a logical coordinate space
> that's identical to the system compositor's by means of the display
> configuration.
>
> And we don't actually use the surface orientation anywhere, so I don't think
> we need to transform according to that either
I agree for a 'we' as in u-s-c + unity8 - since usc does not change the properties of unity8 surface. hmm.. it could still decide that unity8 and thus the client surface is not visible. But that is probably not a relevant result of the transformation? So I agree if you count demo shell out.
> And we don't actually use the surface orientation anywhere, so I don't think
> we need to transform according to that either
I thought for that calculation coordinates are given as client surface coordinates. Rotating the device turns a client surface of 400x900 pixels into a 900x400 grid. Whatever is now on 800x10, must be touched by the user at 390x800 or 10x100...
Alexandros Frantzis (afrantzis) wrote : | # |
OK. Hopefully we will get a proper extension mechanism in the near/medium term.
509 +TEST_F(
"Unavailable"
> I thought for that calculation coordinates are given as client surface coordinates. Rotating the
> device turns a client surface of 400x900 pixels into a 900x400 grid. Whatever is now on 800x10,
> must be touched by the user at 390x800 or 10x100...
Not sure about this aspect of the code, so a weak approval for me pending further information on this.
Andreas Pokorny (andreas-pokorny) wrote : | # |
As far as I can see qtmir/unity8 is not using mirs orientation values(, yet?). But it will need a way to influence the calculation instead.
Chris Halse Rogers (raof) wrote : | # |
Yup. CoordinateTrans
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1859
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1860
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://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1861
http://
Executed test runs:
SUCCESS: 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:1862
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://
Andreas Pokorny (andreas-pokorny) wrote : | # |
lgtm.
one little detail .. why int32_t in the protobuf messages/int in the client api - but unsigned in the CoordinateTrans
(negative positions could be possible too, if decorations are in negative coordinate space of the surface)
Chris Halse Rogers (raof) wrote : | # |
Probably best to use int32_t everywhere. Good catch.
Daniel van Vugt (vanvugt) wrote : | # |
We do indeed have negative coordinates. So make sure your x's and y's are signed.
Although I recommend against int32_t for a few reasons. "int" is better because:
* On any modern platform "int" is already >= 32 bits
* On some 64-bit platforms "int" is 64-bit and it's more efficient to use that native word size than forcing 32-bits.
* int32_t increases coupling to yet another header.
Alberto Aguirre (albaguirre) wrote : | # |
> Although I recommend against int32_t for a few reasons. "int" is better
> because:
> * On any modern platform "int" is already >= 32 bits
> * On some 64-bit platforms "int" is 64-bit and it's more efficient to use
> that native word size than forcing 32-bits.
> * int32_t increases coupling to yet another header.
No since it involves protobuf (https:/
The type should match the type assigned in protobuf or in the very least specifically converted from int32_t to whatever internal type you use. The last thing we need is a subtle type conversion issue.
Daniel van Vugt (vanvugt) wrote : | # |
Alright, still needs conversion to signed ints though. Like in:
232 + virtual geometry::Point surface_
233 + uint32_t x, uint32_t y) = 0;
Even local coordinates can go negative -- Consider what happens when you drag something like a scroll bar or image. The scrolling keeps responding even when the pointer itself is out of the window (so could be in negative coordinates).
Daniel van Vugt (vanvugt) wrote : | # |
Actually I still disagree with int32_t. If you make the decision to use that type just because Protobuf does, then you leak that information throughout your APIs in a very ugly way. "int" should be used instead, so that we at least hide the protocol implementation details from our client API design.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1865
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Chris Halse Rogers (raof) wrote : | # |
Hah. I think we've found another area in which our stylistic preferences disagree :)
The way I see it, we currently hide the fact from our client API that Mir's coordinates system has exactly 32bits of significant data. “int” isn't hiding irrelevant implementation details, it's hiding a potentially relevant aspect of our API.
Indeed, I think we should be explicitly sizing *all* our types where they correspond to coordinates or other data types that are a part of our API that happen to be integral.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1866
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : | # |
Remember we only support client and server on the same host, and only plan on supporting that. As such the client and server can safely use the same type with an unspecified width, like "int".
Only if you're supporting a networked environment with heterogeneous word sizes is it necessary to specify the size. But we have no plans to ever make Mir a network protocol.
Chris Halse Rogers (raof) wrote : | # |
I run armhf code on my amd64 machine all the time. Local machine only does *not* imply same-word-size, or even same-endianness.
Chris Halse Rogers (raof) wrote : | # |
And, obviously, i386/amd64 is an even more common usecase.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1867
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://
Andreas Pokorny (andreas-pokorny) wrote : | # |
> And, obviously, i386/amd64 is an even more common usecase.
hm maybe we could add something to the ci script that takes the x86 build and tries to run it with an amd64 server
Daniel van Vugt (vanvugt) wrote : | # |
You have an armhf client/server talking to an amd64 server/client? That sounds interesting. QEMU?
Also interesting is the idea of mixing and matching amd64 and i386 on the same host. That does sound like a plausible albeit uncommon scenario.
Still, even if there are reasonable use cases for mixing and matching I think it's much cleaner design to choose the client API and then modify the protocol to fit rather than vice-versa.
Chris Halse Rogers (raof) wrote : | # |
On Tue, Oct 14, 2014 at 5:06 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Abstain
>
> You have an armhf client/server talking to an amd64 server/client?
> That sounds interesting. QEMU?
Indeed. With dpkg multiarch the only limitation on the architecture
combinations are the qemu system hosts.
>
> Also interesting is the idea of mixing and matching amd64 and i386 on
> the same host. That does sound like a plausible albeit uncommon
> scenario.
It's actually *super* common; does Steam ship an amd64 binary, for
example? The majority of games are still i386 binaries.
If Intel has its way we'll end up with lots of x32 binaries on amd64
systems as the norm :).
>
> Still, even if there are reasonable use cases for mixing and matching
> I think it's much cleaner design to choose the client API and then
> modify the protocol to fit rather than vice-versa.
Indeed, but the client API should guarantee appropriately sized data
types :)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1868
http://
Executed test runs:
SUCCESS: 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:1869
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Chris Halse Rogers (raof) wrote : | # |
Dear protobuf: Your behaviour in registering an init thing that aborts on double-load is really annoying.
Alan Griffiths (alan-griffiths) wrote : | # |
On 20/10/14 22:04, Chris Halse Rogers wrote:
> Dear protobuf: Your behaviour in registering an init thing that aborts on double-load is really annoying.
Yes, maybe it was a mistake to roll libmirprotobuf into libmircommon and
we should split it out again.
--
Alan Griffiths +44 (0)798 9938 758
Octopull Ltd http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1869
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1870
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://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Alberto Aguirre (albaguirre) wrote : | # |
Looks like a CI hiccup unrelated to MP let's try again...
FAILED: Continuous integration, rev:1848 jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- ci/2455/ jenkins. qa.ubuntu. com/job/ mir-android- utopic- i386-build/ 1407/console jenkins. qa.ubuntu. com/job/ mir-clang- utopic- amd64-build/ 1413/console jenkins. qa.ubuntu. com/job/ mir-mediumtests -utopic- touch/1390/ console jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- utopic- amd64-ci/ 977 jenkins. qa.ubuntu. com/job/ mir-team- mir-development -branch- utopic- amd64-ci/ 977/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- utopic- armhf/327/ console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/mir- team-mir- development- branch- ci/2455/ rebuild
http://