Mir

Merge lp://qastaging/~robertcarr/mir/receive-input-in-client into lp://qastaging/~mir-team/mir/trunk

  1. receive-input-in-client
  2. Merge into trunk
Proposed by Robert Carr
Status: Merged
Approved by: Thomas Voß
Approved revision: no longer in the source branch.
Merged at revision: 562
Proposed branch: lp://qastaging/~robertcarr/mir/receive-input-in-client
Merge into: lp://qastaging/~mir-team/mir/trunk
Diff against target: 3169 lines (+1583/-233)
69 files modified
3rd_party/CMakeLists.txt (+2/-6)
3rd_party/android-deps/std/SortedVector.h (+1/-1)
3rd_party/android-input/android/frameworks/base/include/androidfw/InputTransport.h (+2/-0)
3rd_party/android-input/android/frameworks/base/services/input/InputTransport.cpp (+5/-0)
CMakeLists.txt (+0/-7)
cross-compile-chroot.sh (+0/-1)
debian/rules (+0/-1)
doc/building_source_for_android.md (+2/-2)
examples/demo_client.c (+1/-1)
examples/demo_client_accelerated.cpp (+1/-1)
examples/demo_client_unaccelerated.c (+1/-1)
examples/eglapp.c (+14/-1)
examples/render_surfaces.cpp (+2/-16)
include/client/mir_toolkit/mir_client_library.h (+3/-1)
include/server/mir/input/null_input_manager.h (+56/-0)
include/shared/mir/input/android/android_input_lexicon.h (+7/-0)
include/shared/mir_toolkit/c_types.h (+21/-0)
include/shared/mir_toolkit/input/event.h (+2/-2)
include/test/mir_test/wait_condition.h (+5/-3)
src/client/CMakeLists.txt (+10/-3)
src/client/input/CMakeLists.txt (+12/-0)
src/client/input/android_input_platform.cpp (+44/-0)
src/client/input/android_input_platform.h (+52/-0)
src/client/input/android_input_receiver.cpp (+91/-0)
src/client/input/android_input_receiver.h (+84/-0)
src/client/input/android_input_receiver_thread.cpp (+68/-0)
src/client/input/android_input_receiver_thread.h (+71/-0)
src/client/input/input_platform.h (+55/-0)
src/client/input/input_receiver_thread.h (+48/-0)
src/client/mir_client_library.cpp (+10/-6)
src/client/mir_client_surface.h (+1/-0)
src/client/mir_connection.cpp (+8/-2)
src/client/mir_connection.h (+7/-0)
src/client/mir_surface.cpp (+19/-1)
src/client/mir_surface.h (+13/-0)
src/server/CMakeLists.txt (+1/-0)
src/server/default_server_configuration.cpp (+7/-2)
src/server/frontend/protobuf_message_processor.cpp (+4/-2)
src/server/input/CMakeLists.txt (+0/-7)
src/server/input/android/CMakeLists.txt (+1/-1)
src/server/input/android/android_input_application_handle.cpp (+6/-1)
src/server/input/android/android_input_application_handle.h (+1/-1)
src/server/input/android/event_filter_dispatcher_policy.cpp (+3/-2)
src/server/input/android/transport/CMakeLists.txt (+31/-0)
src/server/input/android/transport/android_input_lexicon.cpp (+7/-7)
src/server/input/dummy_input_manager.cpp (+0/-42)
tests/acceptance-tests/CMakeLists.txt (+4/-0)
tests/acceptance-tests/test_client_input.cpp (+268/-0)
tests/acceptance-tests/test_client_library.cpp (+7/-7)
tests/acceptance-tests/test_focus_management_api.cpp (+1/-1)
tests/acceptance-tests/test_focus_selection.cpp (+1/-1)
tests/acceptance-tests/test_surfaceloop.cpp (+5/-5)
tests/integration-tests/CMakeLists.txt (+0/-2)
tests/integration-tests/client/test_client_render.cpp (+8/-8)
tests/integration-tests/input/android/test_android_cursor_listener.cpp (+2/-3)
tests/integration-tests/input/android/test_android_input_manager.cpp (+8/-9)
tests/integration-tests/input/android/test_fake_event_hub_to_event_filter.cpp (+2/-3)
tests/integration-tests/test_error_reporting.cpp (+1/-1)
tests/integration-tests/test_surfaceloop.cpp (+4/-4)
tests/mir_test_doubles/CMakeLists.txt (+0/-2)
tests/unit-tests/CMakeLists.txt (+0/-3)
tests/unit-tests/client/CMakeLists.txt (+4/-0)
tests/unit-tests/client/input/CMakeLists.txt (+9/-0)
tests/unit-tests/client/input/test_android_input_receiver.cpp (+198/-0)
tests/unit-tests/client/input/test_android_input_receiver_thread.cpp (+152/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+106/-51)
tests/unit-tests/input/android/test_android_input_application_handle.cpp (+18/-6)
tests/unit-tests/input/android/test_android_input_lexicon.cpp (+5/-5)
tests/unit-tests/input/android/test_android_input_window_handle.cpp (+1/-1)
To merge this branch: bzr merge lp://qastaging/~robertcarr/mir/receive-input-in-client
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Chris Halse Rogers Approve
Review via email: mp+155368@code.qastaging.launchpad.net

Commit message

Enable end-to-end keyboard input and test through tests/acceptance-tests/test_client_input.cpp

Description of the change

This branch contains the client side of input and the finishing touches required to make keyboard input delivery work end to end!

The driving acceptance test is shown in tests/acceptance-tests/test_client_input.cpp

See test_client_mir_surface.cpp for explanation of how client input thread is started and utilized. (this test has an incidental bug! in failing to set fd_items). Implementation revolves around MirSurface::created.

The input machinery on the client side is mclia::InputReceiver and mclia::InputReceiverThread (tests/unit-tests/client/input/...). (some of you may recognize android_input_receiver.cpp from November ;))

Some changes to protobuf_message_processor.cpp were required

Currently this is failing to work with MIR_INPUT_USE_ANDROID_TYPES=false (as is the default). The acceptance test can be observed to fail (the client will only receive one event). This is due to the server failing to receive handling responses from the client and believing the client has gone nonresponsive. The failing code path is around InputDispatcher.cpp:3132, handleReceiveCallback is never invoked.

On the other hand with MIR_INPUT_USE_ANDROID_TYPES=true, handleReceiveCallback is invoked. With this working, the acceptance test passes. You can observe input working in manual testing too! eglapp.c is modified to respond to "q" to quit. I have a messy branch of Qtubuntu (will cleanup) which enables key input as well, and have succesfully run and navigated the qt5/examples/qtdeclarative/quick/keynavigation example!

Some notes from my own look at the diff

* Should the event handler type take MirSurface as the first argument? So far this hasn't been useful. If so, perhaps the EventDelegate should be passed to the Connection, it could be for other kinds of callbacks (such as?...application focus maybe?) as well. ubuntu_platform_api uses no Surface as the first argument and delegate attached to surface (or rather two argument callback, context). Maybe we should just work this way? Seems a shame to not use a delegate struct though as we will just be continually adding arguments (focus_changed_cb, proximity_cb (strawman ;)) etc...)

* The continual writing of std::function<void(MirEvent*)> is unpleasant maybe it should be typedeffed somewhere?

To post a comment you must log in.
Revision history for this message
Robert Carr (robertcarr) wrote :

Jenkins will fail due to MIR_INPUT_USE_ANDROID_TYPES=false

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

FAILED: Continuous integration, rev:582
http://jenkins.qa.ubuntu.com/job/mir-ci/199/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/130/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/200/console

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

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

FAILED: Continuous integration, rev:585
http://jenkins.qa.ubuntu.com/job/mir-ci/201/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/132/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/202/console

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

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

FAILED: Continuous integration, rev:586
http://jenkins.qa.ubuntu.com/job/mir-ci/208/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/142/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/209/console

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

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

Trunk merged. There were some tricky bits around test_client_mir_surface.cpp as trunk was missing:

        server_package.fd_items = num_fd;
and so the test appeared to pass when really it should have failed.

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

FAILED: Continuous integration, rev:587
http://jenkins.qa.ubuntu.com/job/mir-ci/210/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/144/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/211/console

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

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

In general: at least a little bit of Doxygen comments for the classes you add, please! ☺

------------------------

716 + droidinput::status_t status;
717 + if((status = input_consumer->consume(&event_factory, consume_batches,
718 + default_frame_time, &event_sequence_id, &android_event)) != droidinput::WOULD_BLOCK)

Status is set but never used.

------------------------

807 + int get_fd() const;

I think we'd generally expect this to be called just ‘fd()’?

------------------------

822 + static const bool consume_batches = true;
823 + static const bool do_not_consume_batches = false;
...
825 + static const bool handle_event = true;
826 + static const bool do_not_handle_event = false;

As far as I'm aware we've not been using this pattern elsewhere in the code? I'm not particularly against it, though.

-------------------------

739 + auto status = ::poll(&pfd, 1, timeout.count());
740 +
741 + if (status > 0)
742 + return true;
743 + if (status == 0)
744 + return false;
745 +
746 + // TODO: What to do in case of error? ~racarr
747 + return false;
748 +}

This should probably try polling(!) again while status == EINTR.

Other errors should probably be runtime exceptions; none of the other errors poll can return (EFAULT, EINVAL, ENOMEM) are going to go away without explicit action.

I'm not comfortable letting this in while we're polling every 10msec for input.

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

>>> In general: at least a little bit of Doxygen comments for the classes you add, please! ☺

I tried to add some helpful comments!

>> 716 + droidinput::status_t status;
>> 717 + if((status = input_consumer->consume(&event_factory, consume_batches,
>> 718 + default_frame_time, &event_sequence_id, &android_event)) != droidinput::WOULD_BLOCK)

>> Status is set but never used.

Fixed

>> 807 + int get_fd() const;

Fixed

------------------------

>> 822 + static const bool consume_batches = true;
>> 823 + static const bool do_not_consume_batches = false;
...
>> 825 + static const bool handle_event = true;
>> 826 + static const bool do_not_handle_event = false;

>> As far as I'm aware we've not been using this pattern elsewhere in the code? I'm not particularly against >> it, though.

These come from a long series of reviews on an original version of AndroidInputReceiver back in november...I do not have strong opinions though I do think they make the implementation read in a rather literate style.

-------------------------

>> I'm not comfortable letting this in while we're polling every 10msec for input.

The custom poll implementation is replaced with a droidinput::Looper, this way the poll is indefinite, but may still be woken by a wake pipe from another thread.

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

l725 is a mystery to me. It's easy to observe: Moving it to the constructor causes the acceptance test to fail intermittently.

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

I'd like to add an exception here:
+ else if (result == ALOOPER_POLL_ERROR) // TODO: Exception?

It's difficult though without a mockable looper to test this exception so I would suggest we target it for a follow-up mp

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

FAILED: Continuous integration, rev:590
http://jenkins.qa.ubuntu.com/job/mir-ci/225/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/161/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/44/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/226/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/29

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

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

FAILED: Continuous integration, rev:592
http://jenkins.qa.ubuntu.com/job/mir-ci/227/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/163/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/46/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/228/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/31

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

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

FAILED: Continuous integration, rev:595
http://jenkins.qa.ubuntu.com/job/mir-ci/236/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/179/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/62/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/237/console

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

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

FAILED: Continuous integration, rev:596
http://jenkins.qa.ubuntu.com/job/mir-ci/237/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/180/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/63/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/238/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/44

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

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

Why is "MirEventDelegate const *event_handler" a separate parameter and not a member of MirSurfaceParameters?

~~~~

563 + virtual ~TestingClientConfiguration() {};

Is this necessary? And shouldn't it be "virtual ~TestingClientConfiguration() = default;"

~~~~

550 +#include <gmock/gmock.h>

There are worse things wrong with this file:

  o WaitCondition doesn't belong in namespace mir
  o anonymous namespaces make things unique to each translation units, and should rarely appear in headers.

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

FAILED: Continuous integration, rev:599
http://jenkins.qa.ubuntu.com/job/mir-ci/240/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/184/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/67/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/241/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/49

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

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

FAILED: Continuous integration, rev:601
http://jenkins.qa.ubuntu.com/job/mir-ci/241/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/185/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/68/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/242/console

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

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

FAILED: Continuous integration, rev:603
http://jenkins.qa.ubuntu.com/job/mir-ci/242/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/186/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/69
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/243/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/51

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

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

FAILED: Continuous integration, rev:604
http://jenkins.qa.ubuntu.com/job/mir-ci/244/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/71
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/245/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/52/console

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

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

FAILED: Continuous integration, rev:605
http://jenkins.qa.ubuntu.com/job/mir-ci/246/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/73
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/247/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/54

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

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

> >> 822 + static const bool consume_batches = true;
> >> 823 + static const bool do_not_consume_batches = false;
> ...
> >> 825 + static const bool handle_event = true;
> >> 826 + static const bool do_not_handle_event = false;
>
> >> As far as I'm aware we've not been using this pattern elsewhere in the
> code? I'm not particularly against >> it, though.
>
> These come from a long series of reviews on an original version of
> AndroidInputReceiver back in november...I do not have strong opinions though I
> do think they make the implementation read in a rather literate style.

Where are we using it here? I find no use of "do_not_handle_event" in the MP.

It is better to use "enum class BatchMode { do_not_consume_batches, consume_batches };" - which allows type checking on parameters.

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

FAILED: Continuous integration, rev:609
http://jenkins.qa.ubuntu.com/job/mir-ci/266/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/214
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/97
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/267/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/74

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

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

* General cleanup and fix valgrind complaints r606-610
* Android input application handle should take a weak reference to prevent circular ownership. (r608)
* Remove strange constants
* Cleanup wait_condition.h

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

PASSED: Continuous integration, rev:610
http://jenkins.qa.ubuntu.com/job/mir-ci/267/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/216
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/99
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/268
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/268/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/75

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

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

PASSED: Continuous integration, rev:612
http://jenkins.qa.ubuntu.com/job/mir-ci/268/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/217
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/269
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/269/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/77

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

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

FAILED: Continuous integration, rev:615
http://jenkins.qa.ubuntu.com/job/mir-ci/270/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/219
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/102
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/271/console

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

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

PASSED: Continuous integration, rev:616
http://jenkins.qa.ubuntu.com/job/mir-ci/274/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/224
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/107
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/275
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/275/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/83

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

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

321 InputMessage msg;
322 + memset(&msg, 0, sizeof(InputMessage));
...
330 InputMessage msg;
331 + memset(&msg, 0, sizeof(InputMessage));
...
339 InputMessage msg;
340 + memset(&msg, 0, sizeof(InputMessage));

Wouldn't a default constructor be less repetitive?

~~~~

Why is "MirEventDelegate const *event_handler" a separate parameter and not a member of MirSurfaceParameters?

~~~~

"Should the event handler type take MirSurface as the first argument? ... ubuntu_platform_api uses no Surface as the first argument and delegate attached to surface (or rather two argument callback, context). Maybe we should just work this way?"

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

>> 321 InputMessage msg;
>> 322 + memset(&msg, 0, sizeof(InputMessage));
>> ...
>> 330 InputMessage msg;
>> 331 + memset(&msg, 0, sizeof(InputMessage));
>> ...
>> 339 InputMessage msg;
>> 340 + memset(&msg, 0, sizeof(InputMessage));

Yes it would says r618.

>> Why is "MirEventDelegate const *event_handler" a separate parameter and not a member of
>> MirSurfaceParameters?

I think it's a very different kind of parameter (not sent over the wire to the server, not used for construction, not serializable). Maybe the API user doesn't care?

>> "Should the event handler type take MirSurface as the first argument? ... ubuntu_platform_api uses no
>> Surface as the first argument and delegate attached to surface (or rather two argument callback, context). >> Maybe we should just work this way?"

This was just kind of a thought I threw out. I like the current signature (surface, ev, ctx) and it is in keeping with C style used in GLib, etc...

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

PASSED: Continuous integration, rev:618
http://jenkins.qa.ubuntu.com/job/mir-ci/277/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/229
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/112
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/278
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/278/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/87

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

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

Spurious whitespace:

1302 -
1303 +

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

Probably OK once the conflict is fixed

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

FAILED: Continuous integration, rev:619
http://jenkins.qa.ubuntu.com/job/mir-ci/283/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/237/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/120/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/287/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/93

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

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

FAILED: Continuous integration, rev:620
http://jenkins.qa.ubuntu.com/job/mir-ci/286/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/242/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/125/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/291/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/98

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

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

PASSED: Continuous integration, rev:621
http://jenkins.qa.ubuntu.com/job/mir-ci/289/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/245
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/128
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/294
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/294/artifact/work/output/*zip*/output.zip

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

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

1236 -
1237 +

Spurious whitespace

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

2667 - auto surface = std::make_shared<MirSurface> (connection.get(),
2668 - *client_comm_channel,
2669 - logger,
2670 - mock_buffer_factory,
2671 - params,
2672 - &empty_callback,
2673 - nullptr);
2674 + auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, mock_buffer_factory, input_platform, params, nullptr, &empty_callback, (void*) NULL);

Using "(void*) NULL" in place of "nullptr" isn't an improvement.

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

333 +

Spurious whitespace

There's also some NULL usage.

But I'll abstain as there's nothing obvious to block on and I'm at EOD.

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

PASSED: Continuous integration, rev:623
http://jenkins.qa.ubuntu.com/job/mir-ci/300/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/258
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/141
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/305
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/305/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/113

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

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

Many whitespace and style fixes

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

PASSED: Continuous integration, rev:639
http://jenkins.qa.ubuntu.com/job/mir-ci/302/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/145
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/307
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/307/artifact/work/output/*zip*/output.zip

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

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

So, we can probably land this, and do some followup cleaning:

155 + surface = mir_surface_create_sync(connection, &request_params, NULL);

Might as well use nullptr here (and change the surrounding NULL checks).

src/client/input/android_input_platform.{cpp,h} is GPlv3+; should be LGPLv3+
src/client/input/android_input_receiver.{cpp,h} is GPlv3+; should be LGPLv3+
src/client/input/android_input_receiver_thread.{cpp,h} is GPlv3+; should be LGPLv3+
src/client/input/input_platform.h is GPlv3+; should be LGPLv3+
…and basically all the added files, it seems.

A bunch of cases of ~Foo() {}, which I think would more idiomatically be ~Foo() = default;

1356 + ("enable-input,i", po::value<bool>(), "Enable input. [bool:default=false]")
1357 (log_display, po::value<bool>(), "Log the Display report. [bool:default=false]")
1358 (log_app_mediator, po::value<bool>(), "Log the ApplicationMediator report. [bool:default=false]")
1359 (log_msg_processor, po::value<bool>(), "log the MessageProcessor report")
1360 @@ -316,9 +318,12 @@
1361 mir::DefaultServerConfiguration::the_input_manager()
1362 {
1363 return input_manager(
1364 - [&, this]()
1365 + [&, this]() -> std::shared_ptr<mi::InputManager>
1366 {
1367 - return mi::create_input_manager(the_event_filters(), the_display());
1368 + if (the_options()->get("enable-input", false))
1369 + return mi::create_input_manager(the_event_filters(), the_display());
1370 + else
1371 + return std::make_shared<mi::NullInputManager>();

Is this the right way around? It looks like input is turned off if ‘enable-input’ is true?

2641 +// thread->join();

Looks like you've got some temporary testing code still hanging around?

2883 - auto surface = std::make_shared<MirSurface> (connection.get(),
2884 - *client_comm_channel,
2885 - logger,
2886 - mock_buffer_factory,
2887 - params,
2888 - &empty_callback,
2889 - nullptr);
2890 + auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, mock_buffer_factory, input_platform, params, nullptr, &empty_callback, nullptr);
(and further examples in that file)
I think we prefer the former formatting, with shorter lines?

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

> 1368 + if (the_options()->get("enable-input", false))
> 1369 + return mi::create_input_manager(the_event_filters(),
> the_display());
> 1370 + else
> 1371 + return std::make_shared<mi::NullInputManager>();
>
> Is this the right way around? It looks like input is turned off if ‘enable-
> input’ is true?

It is the right way "false" is the default value for get. Maybe we should add a helper function so that we can write "the_options()->get("enable-input", default_to(false))"?

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

>So, we can probably land this, and do some followup cleaning:

+1

...
> src/client/input/android_input_platform.{cpp,h} is GPlv3+; should be LGPLv3+
> src/client/input/android_input_receiver.{cpp,h} is GPlv3+; should be LGPLv3+
> src/client/input/android_input_receiver_thread.{cpp,h} is GPlv3+; should be
> LGPLv3+
> src/client/input/input_platform.h is GPlv3+; should be LGPLv3+
> …and basically all the added files, it seems.
>
> A bunch of cases of ~Foo() {}, which I think would more idiomatically be
> ~Foo() = default;

We can probably fix this when we clean up a whole bunch of potentially throwing destructors.

> 2641 +// thread->join();
>
> Looks like you've got some temporary testing code still hanging around?
>
> 2883 - auto surface = std::make_shared<MirSurface> (connection.get(),
> 2884 -
> *client_comm_channel,
> 2885 - logger,
> 2886 - mock_buffer_factory,
> 2887 - params,
> 2888 - &empty_callback,
> 2889 - nullptr);
> 2890 + auto surface = std::make_shared<MirSurface> (connection.get(),
> *client_comm_channel, logger, mock_buffer_factory, input_platform, params,
> nullptr, &empty_callback, nullptr);
> (and further examples in that file)
> I think we prefer the former formatting, with shorter lines?

I think we should prefer a third style - breaking before the first parameter when necessary. (The original is fragile when renaming anything that appears before that point - and we do global renames often enough for it to matter.)

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

FAILED: Autolanding.
Unapproved changes made after approval.
http://jenkins.qa.ubuntu.com/job/mir-autolanding/115/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/155
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-autolanding/116
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-autolanding/116/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-autolanding-build/./distribution=precise,flavor=amd64/12

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
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