Mir

Merge lp://qastaging/~andreas-pokorny/mir/input-registrar-observer into lp://qastaging/mir

input-registrar-observer
Merge into development-branch

Proposed by Andreas Pokorny on 2014-04-23

Status:	Rejected
Rejected by:	Andreas Pokorny on 2014-05-05
Proposed branch:	lp://qastaging/~andreas-pokorny/mir/input-registrar-observer
Merge into:	lp://qastaging/mir
Prerequisite:	lp://qastaging/~andreas-pokorny/mir/custom_input_dispatcher
Diff against target:	1057 lines (+401/-124) 29 files modified include/server/mir/input/input_dispatcher_configuration.h (+0/-1) include/server/mir/scene/input_registrar.h (+7/-16) include/server/mir/scene/input_registrar_observer.h (+60/-0) include/test/mir_test/fake_event_hub_input_configuration.h (+6/-1) include/test/mir_test_doubles/mock_input_registrar_observer.h (+3/-3) include/test/mir_test_doubles/stub_input_registrar.h (+53/-0) include/test/mir_test_doubles/stub_input_registrar_observer.h (+5/-5) src/server/input/android/android_input_registrar.h (+2/-2) src/server/input/android/input_dispatcher_configuration.cpp (+10/-13) src/server/input/android/input_dispatcher_configuration.h (+9/-3) src/server/input/default_configuration.cpp (+2/-10) src/server/input/null_input_configuration.cpp (+0/-21) src/server/scene/CMakeLists.txt (+1/-0) src/server/scene/default_configuration.cpp (+12/-0) src/server/scene/default_input_registrar.cpp (+63/-0) src/server/scene/default_input_registrar.h (+56/-0) src/server/scene/surface_stack.cpp (+2/-2) src/server/scene/surface_stack.h (+3/-3) tests/acceptance-tests/test_custom_input_dispatcher.cpp (+1/-5) tests/acceptance-tests/test_server_shutdown.cpp (+2/-1) tests/integration-tests/input/android/test_android_cursor_listener.cpp (+3/-1) tests/integration-tests/input/android/test_android_input_manager.cpp (+11/-8) tests/integration-tests/input/test_nested_input.cpp (+7/-18) tests/integration-tests/session_management.cpp (+1/-0) tests/mir_test_doubles/fake_event_hub_input_configuration.cpp (+8/-5) tests/mir_test_framework/input_testing_server_options.cpp (+2/-1) tests/unit-tests/scene/CMakeLists.txt (+1/-0) tests/unit-tests/scene/test_default_input_registrar.cpp (+66/-0) tests/unit-tests/scene/test_surface_stack.cpp (+5/-5)
To merge this branch:	bzr merge lp://qastaging/~andreas-pokorny/mir/input-registrar-observer
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Needs Fixing on 2014-05-02
Robert Carr (community)		2014-04-23	Needs Information on 2014-05-01
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-29
Review via email: mp+216858@code.qastaging.launchpad.net

Commit message

Allow multiple InputRegistrar

This change is allows to split out the the input send parts from droidinput::InputDispatcher. InputRegistrar is copied to InputRegistrarObserver, and two methods are added to InputRegistrar to register further InputRegistrarObservers. The default implementation is no longer related to android integration code.

Description of the change

This the first of the follow up branches of https://code.launchpad.net/~andreas-pokorny/mir/custom_input_dispatcher/+merge/215174 It moves the InputRegistrar back to the DefaultServerConfiguration.

The interface InputRegistrar is used by the scene to indicate that surfaces enter and leave the scene. The android-input related code uses that information to create WindowHandles and pass those to the InputDispatcher as possible input targets. With this change InputRegistrar is renamed to InputRegistrarObserver, and another InputRegistrar is provided that maintains a list of observers. So the android bridging code can simply register itself, and the ominous InputDispatcherConfiguration can be reduced by one.

The new input sender implementation will need that information too, hence the other motivation for that change.

This change will become unnecessary if a racarrs SceneObserver comes in sooner - and if we decide that the InputReceptionMode is not a necessary configuration option or can be obtained in a different way.

lp://qastaging/~andreas-pokorny/mir/input-registrar-observer updated on 2014-04-23

1547. By Andreas Pokorny on 2014-04-23: * New upstream release 0.1.9 (https://launchpad.net/mir/+milestone/0.1.9)
  - mirclient ABI unchanged, still at 7. Clients do not need rebuilding.
  - mirserver ABI bumped to 19. Shells need rebuilding.
  - More libmirserver class changes and reorganization, including;
    . Moving things from shell:: to scene::
    . Rewriting/refactoring surface factories.
  - Added an id() to Renderable.
  - Scene/Renderer interfaces:
    . Scene is no longer responsible for its own iteration (no for_each
      any more). Instead you should iterate over the list returned by
      Scene::generate_renderable_list().
  - Bugs fixed:
    . TODO: fill in when proposing to Ubuntu
[ Daniel van Vugt ]
  - mirserver ABI bumped to 18. Shells need rebuilding.
    . graphics::Platform::create_display() has a new parameter allowing you
      to customize the compositor's (E)GL configutation.
    . Renderable::buffer(unsigned long frameno) is now:
      Renderable::buffer(void const* user_id). See below.
    . Renderable::should_be_rendered_in() is replaced by a more natural:
      Renderable::visble()
    . input::Surface::name() returns by value instead of reference now,
      to ensure future thread safety.
  - Switched EventHub device enumeration and hotplug to Udev. NOTE! This
    means mir_test_* can't run natively on touch devices any more without
    some setup first:
      umockdev-run -- bin/mir_unit_tests
  - Introduced "RenderableList" as the way to sample the Scene contents,
    and started using that in the default compositor.
  - Introduced physical length units and conversion (geometry::Length) in
    preparation for arbitrary DPI rendering.
    anti-aliased and high-DPI scalable.
  - Multi-monitor frame sync has been redesigned to eliminate the need for
    frame number tracking.
  - Bugs (and enhancements) resolved:
    . [enhancement] Please move input detection to libudev (LP: #1237784)
    . [enhancement] Add a clamping resize mode to GLRenderer (LP: #1259887)
    . [regression] Intermittent loss of multimonitor frame sync
      (LP: #1290306)
    . [enhancement] Make GL config options configurable (LP: #1290780)
    . memcheck-test doesn't test anything when DISABLED_GTEST_DISCOVERY is
      enabled (LP: #1291876)
    . "Error opening DRM device" is always followed by "Unknown error -(some
      negative number)" (LP: #1292384)
    . Rendering/composition gets stopped early (LP: #1293896)
    . Ubuntu Touch Settings and terminal apps are not rendering correctly on
      rotate. (LP: #1294048)
    . [regression] Apps are much slower to open (LP: #1294051)
    . Settings app opens to a blank screen unless given enough time to render
      or the app is touched (LP: #1294053)
    . TestClientInput/DemoPrivateProtobuf memory leak is causing regular CI
      test failures (LP: #1295231)
    . OSK touch events "fall through" and hit surface behind them
      (LP: #1297878)
    . [enhancement] add a test for composite of last client post
      (LP: #1298596)
    . [regression] Surfaces vanish as soon as their edges touch the edge of
      screen (LP: #1301115)
* Cherry-picked from future release 0.1.9:
  - Bug fix: mirplatformgraphics does not have boost program options in its
    symbol table (LP: #1301040)
  - Bug fix: unity8 crashed with SIGSEGV in glDeleteTextures() from
    mir::scene::GLPixelBuffer::~GLPixelBuffer() from
    mir::scene::ThreadedSnapshotStrategy::~ThreadedSnapshotStrategy()
    (LP: #1256360)
[ Ubuntu daily release ]
* New rebuild forced

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-23:

FAILED: Continuous integration, rev:1546
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1381/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1678/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1676/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1251/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1113/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1118/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1253/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1381/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1385/rebuild

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-24:

We do need ReceptionMode for the shell surface. It could become a shell attribute though and queried from the relevant scene observer. Maybe this isnt required?

review: Needs Information

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-04-25:

> We do need ReceptionMode for the shell surface. It could become a shell
> attribute though and queried from the relevant scene observer. Maybe this isnt
> required?

you mean a surface attribute?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-28:

111 +/// An interface used to register input targets and take care of input assosciation (i.e.
112 +/// create input channels).
113 +class InputRegistrarObserver
114 +{
115 +public:
116 + virtual ~InputRegistrarObserver() = default;
117 +
118 + virtual void input_channel_opened(std::shared_ptr<input::InputChannel> const& opened_channel,
119 + std::shared_ptr<input::Surface> const& info,
120 + input::InputReceptionMode input_mode) = 0;
121 + virtual void input_channel_closed(std::shared_ptr<input::InputChannel> const& closed_channel) = 0;

From the point of view of code invoking it an "Observer" should not have any role beyond being notified of changes. Specifically, it shouldn't have responsibility to "take care of input assosciation" (although an implementation of the interface could) nor should it be given ownership of anything by being passed shared_ptr (nor unique_ptr).

The comment and function prototypes are at odds with the "Observer" name. But I don't see enough of the picture here to decide which is wrong. Maybe "Observer" is wrong and this is, e.g. a "Strategy" or "State" object?

~~~~

548 + for( auto const& observer : current_observers )

Project style is

for (auto const& observer : current_observers)

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-28:

>> you mean a surface attribute?

Yes sorry, words are hard.

lp://qastaging/~andreas-pokorny/mir/input-registrar-observer updated on 2014-04-29

1548. By Andreas Pokorny on 2014-04-29: merged devel

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-29:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1430/rebuild

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-01:

Now that introduce-scene-observer is landed is this still needed?

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-02:

Merge conflicts

~~~~

813 +#include "mir/scene/input_registrar.h"
...
828 +namespace ms= mir::scene;

I don't see these used in the changes to this file.

review: Needs Fixing

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-05-05:

> Now that introduce-scene-observer is landed is this still needed?

no, so I reject this and rework it.

Unmerged revisions

1548. By Andreas Pokorny on 2014-04-29

merged devel

1547. By Andreas Pokorny on 2014-04-23

* New upstream release 0.1.9 (https://launchpad.net/mir/+milestone/0.1.9)
  - mirclient ABI unchanged, still at 7. Clients do not need rebuilding.
  - mirserver ABI bumped to 19. Shells need rebuilding.
  - More libmirserver class changes and reorganization, including;
    . Moving things from shell:: to scene::
    . Rewriting/refactoring surface factories.
  - Added an id() to Renderable.
  - Scene/Renderer interfaces:
    . Scene is no longer responsible for its own iteration (no for_each
      any more). Instead you should iterate over the list returned by
      Scene::generate_renderable_list().
  - Bugs fixed:
    . TODO: fill in when proposing to Ubuntu
[ Daniel van Vugt ]
  - mirserver ABI bumped to 18. Shells need rebuilding.
    . graphics::Platform::create_display() has a new parameter allowing you
      to customize the compositor's (E)GL configutation.
    . Renderable::buffer(unsigned long frameno) is now:
      Renderable::buffer(void const* user_id). See below.
    . Renderable::should_be_rendered_in() is replaced by a more natural:
      Renderable::visble()
    . input::Surface::name() returns by value instead of reference now,
      to ensure future thread safety.
  - Switched EventHub device enumeration and hotplug to Udev. NOTE! This
    means mir_test_* can't run natively on touch devices any more without
    some setup first:
      umockdev-run -- bin/mir_unit_tests
  - Introduced "RenderableList" as the way to sample the Scene contents,
    and started using that in the default compositor.
  - Introduced physical length units and conversion (geometry::Length) in
    preparation for arbitrary DPI rendering.
    anti-aliased and high-DPI scalable.
  - Multi-monitor frame sync has been redesigned to eliminate the need for
    frame number tracking.
  - Bugs (and enhancements) resolved:
    . [enhancement] Please move input detection to libudev (LP: #1237784)
    . [enhancement] Add a clamping resize mode to GLRenderer (LP: #1259887)
    . [regression] Intermittent loss of multimonitor frame sync
      (LP: #1290306)
    . [enhancement] Make GL config options configurable (LP: #1290780)
    . memcheck-test doesn't test anything when DISABLED_GTEST_DISCOVERY is
      enabled (LP: #1291876)
    . "Error opening DRM device" is always followed by "Unknown error -(some
      negative number)" (LP: #1292384)
    . Rendering/composition gets stopped early (LP: #1293896)
    . Ubuntu Touch Settings and terminal apps are not rendering correctly on
      rotate. (LP: #1294048)
    . [regression] Apps are much slower to open (LP: #1294051)
    . Settings app opens to a blank screen unless given enough time to render
      or the app is touched (LP: #1294053)
    . TestClientInput/DemoPrivateProtobuf memory leak is causing regular CI
      test failures (LP: #1295231)
    . OSK touch events "fall through" and hit surface behind them
      (LP: #1297878)
    . [enhancement] add a test for composite of last client post
      (LP: #1298596)
    . [regression] Surfaces vanish as soon as their edges touch the edge of
      screen (LP: #1301115)
* Cherry-picked from future release 0.1.9:
  - Bug fix: mirplatformgraphics does not have boost program options in its
    symbol table (LP: #1301040)
  - Bug fix: unity8 crashed with SIGSEGV in glDeleteTextures() from
    mir::scene::GLPixelBuffer::~GLPixelBuffer() from
    mir::scene::ThreadedSnapshotStrategy::~ThreadedSnapshotStrategy()
    (LP: #1256360)
[ Ubuntu daily release ]
* New rebuild forced

1546. By Andreas Pokorny on 2014-04-22

Allow multiple InputRegistrar

This change is necessary to split out the the input send parts from droidinput::InputDispatcher. InputRegistrar is copied to InputRegistrarObserver, and two methods are added to InputRegistrar to register further InputRegistrarObservers. The default implementation is no longer related to android integration code.