Mir

Merge lp://qastaging/~andreas-pokorny/mir/fix-1530946 into lp://qastaging/mir

Proposed by Andreas Pokorny
Status: Merged
Approved by: Andreas Pokorny
Approved revision: no longer in the source branch.
Merged at revision: 3266
Proposed branch: lp://qastaging/~andreas-pokorny/mir/fix-1530946
Merge into: lp://qastaging/mir
Diff against target: 1416 lines (+544/-220)
33 files modified
include/client/mir/events/event_builders.h (+2/-1)
include/client/mir_toolkit/events/keymap_event.h (+25/-1)
include/server/mir/scene/null_surface_observer.h (+1/-1)
include/server/mir/scene/surface.h (+2/-1)
include/server/mir/scene/surface_observer.h (+4/-2)
include/test/mir/test/doubles/stub_surface.h (+2/-1)
include/test/mir/test/event_matchers.h (+2/-15)
src/client/event.cpp (+23/-6)
src/client/event_printer.cpp (+1/-8)
src/client/events/CMakeLists.txt (+2/-0)
src/client/events/event_builders.cpp (+41/-46)
src/client/events/make_empty_event.cpp (+42/-0)
src/client/events/make_empty_event.h (+32/-0)
src/client/events/serialization.cpp (+111/-0)
src/client/input/xkb_mapper.cpp (+70/-45)
src/client/mir_surface.cpp (+8/-4)
src/client/rpc/mir_protobuf_rpc_channel.cpp (+45/-46)
src/client/symbols.map (+4/-0)
src/include/client/mir/events/serialization.h (+35/-0)
src/include/common/mir/events/event_private.h (+3/-1)
src/include/common/mir/input/xkb_mapper.h (+22/-7)
src/include/server/mir/scene/surface_event_source.h (+2/-1)
src/include/server/mir/scene/surface_observers.h (+2/-1)
src/server/frontend/event_sender.cpp (+4/-3)
src/server/scene/basic_surface.cpp (+7/-5)
src/server/scene/basic_surface.h (+2/-1)
src/server/scene/legacy_surface_change_notification.cpp (+2/-1)
src/server/scene/legacy_surface_change_notification.h (+2/-1)
src/server/scene/null_surface_observer.cpp (+5/-1)
src/server/scene/surface_event_source.cpp (+6/-2)
tests/acceptance-tests/test_client_input.cpp (+31/-17)
tests/include/mir/test/doubles/stub_scene_surface.h (+2/-1)
tests/mir_test_framework/stub_surface.cpp (+2/-1)
To merge this branch: bzr merge lp://qastaging/~andreas-pokorny/mir/fix-1530946
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Mir CI Bot continuous-integration Approve
Alan Griffiths Approve
Chris Halse Rogers Approve
Alberto Aguirre (community) Approve
Review via email: mp+283634@code.qastaging.launchpad.net

Commit message

Send the keymap instead of not sending the credentials over the wire

Moves the compilation of keymaps to the server, so that a client may never receive an invalid keymap set up. Also adds a MirDeviceId to the event, so that in a future iteration we can implement the per device keymaps. Those cannot be implemented right now, since the nested server has yet no access to the set of available devices that may need a keymap.

In a better world we would use a mem-fd and compile the keymaps only once, and just send the keymap over the wire.

Description of the change

Surprise surprise keymap changing never worked.

This fixes three issues, and prepares a new feature:
 - xkb commons xkb_rule_names is easy to initialize wrongly, the surface interface now uses strings instead
 - xkb_rule_names was never serialized properly so, the client rpc channel only received server side pointers
 - keymap compilation happens on server so illegal combinations are tracked immediately

The new feature is configuring per device keymaps. This cannot be implemented with the current MP yet, since the nested server is still unaware of any attached keyboards, that might need keymaps..

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

There are changes that seem unrelated to this fix. For example: a public make_event() function that creates an uninitialized (aka invalid) event. I don't think any code outside our event creation could need this.

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

+ return std::move(e);

this is a pessimization - it disables RVO

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> There are changes that seem unrelated to this fix. For example: a public
> make_event() function that creates an uninitialized (aka invalid) event. I
> don't think any code outside our event creation could need this.

reworking that rihgt now

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

As mentioned:

mir::EventUPtr make_event() should be private to event_builders.cpp, mev::deserialize_event() should return an EventUPtr rather than populate a previously-allocated uninitialized event, you shouldn't ‘return std::move()’ as it is unnecessary and inhibits RVO, and you've got two spaces after “bool” in a number of places ;).

review: Needs Fixing
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3254
https://mir-jenkins.ubuntu.com/job/mir-ci/129/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/129/console

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

review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

renamed it to make_empty_event to avoid being exposed as symnbol

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3255
https://mir-jenkins.ubuntu.com/job/mir-ci/133/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/133/console

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

review: Approve (continuous-integration)
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3256
https://mir-jenkins.ubuntu.com/job/mir-ci/134/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/134/console

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

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

LGTM

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

Fine for me now.

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

PASSED: Continuous integration, rev:3254
http://jenkins.qa.ubuntu.com/job/mir-ci/6109/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5659
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4566
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5615
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/324
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/433
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/433/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/433
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/433/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5612
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5612/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8050
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26945
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/320
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/320/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/176
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26959

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

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

One nit:

 EventUPtr make_event(
+
frontend::SurfaceId const& surface_id,

Unwanted whitespace

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3257
https://mir-jenkins.ubuntu.com/job/mir-ci/140/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/140/console

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

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

PASSED: Continuous integration, rev:3256
http://jenkins.qa.ubuntu.com/job/mir-ci/6113/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5664
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4571
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5620
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/328
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/437
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/437/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/437
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/437/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5617
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5617/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8055
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26974
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/324
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/324/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/180
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26985

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches