Mir

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

0.20-fix-1550050
Merge into 0.20

Proposed by Andreas Pokorny on 2016-03-02

Status:

Merged

Merged at revision:

3331

Proposed branch:

lp://qastaging/~andreas-pokorny/mir/0.20-fix-1550050

Merge into:

lp://qastaging/mir/0.20

Diff against target:

423 lines (+219/-53)

6 files modified

include/test/mir/test/doubles/mock_input_device_hub.h (+43/-0)
src/server/input/default_configuration.cpp (+37/-24)
src/server/input/default_input_device_hub.cpp (+14/-8)
src/server/input/key_repeat_dispatcher.cpp (+41/-0)
src/server/input/key_repeat_dispatcher.h (+7/-4)
tests/unit-tests/input/test_key_repeat_dispatcher.cpp (+77/-17)

To merge this branch:

bzr merge lp://qastaging/~andreas-pokorny/mir/0.20-fix-1550050

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Needs Fixing on 2016-03-07
Kevin DuBois (community)		2016-03-02	Approve on 2016-03-04
Review via email: mp+287854@code.qastaging.launchpad.net

Commit message

Fixes the potential endless repeat on unplugged devices

The key repeat dispatcher now reacts on the removal of input devices by resetting potentially still active repeat alarms. The integration here is a bit clumsy, since the input device hub needs the input dispatcher to forward device events. While the input dispatcher needs it to register at the input device hub as an observer.

Description of the change

This is the small version of the fix. It makes the KeyRepeatDispatcher observe the input devices attached and resets the repeat handling alarms on device removal.

The changes in src/server/input/default_configuration.cpp were necessary since there is already a dependency path from InputDeviceHub to InputDispatcher. I plan to make a different fix on lp:mir that moves the repeat handling directly to the device/platform itself.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-02:

The effect of the branch is sometimes hidden by the fact that some bluetooth drivers keep track for input states and release keys when unplugging the device. The problem is easier to reproduce with USB devices.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-03:

src/server/input/default_configuration.cpp

- [this]()
+ [this]() -> std::shared_ptr<mi::InputDispatcher>

- the_event_filter_chain_dispatcher(), the_main_loop(), the_cookie_authority(),
- enable_repeat, key_repeat_timeout, key_repeat_delay);
+ the_event_filter_chain_dispatcher(), the_main_loop(), the_cookie_authority(),
+ enable_repeat, key_repeat_timeout, key_repeat_delay);

unneeded changes?

+ void set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub);
two step initialization.

review: Needs Fixing

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-03:

> src/server/input/default_configuration.cpp
>
> - [this]()
> + [this]() -> std::shared_ptr<mi::InputDispatcher>
>
> - the_event_filter_chain_dispatcher(), the_main_loop(),
> the_cookie_authority(),
> - enable_repeat, key_repeat_timeout, key_repeat_delay);
> + the_event_filter_chain_dispatcher(), the_main_loop(),
> the_cookie_authority(),
> + enable_repeat, key_repeat_timeout, key_repeat_delay);
>
> unneeded changes?

yes reverting that..

>
> + void set_input_device_hub(std::shared_ptr<InputDeviceHub> const& hub);
> two step initialization.

This is one of the reasons why I make a better solution into lp:mir - it will turn into a bigger change and somewhat turn into something that no longer looks like simple fix.
Two step init is necessary to avoid a dependency cycle between 'the intput dispatchers' and InputDeviceHub.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-04:

I suppose I don't mind as much if the two step initialization will be improved before going to trunk.

It would be good (necessary?) to see a CI pass on the MP before merging as the release. I suppose all that adds up to a +1 from me then.

review: Approve

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-07:

> I suppose I don't mind as much if the two step initialization will be improved
> before going to trunk.
>
> It would be good (necessary?) to see a CI pass on the MP before merging as the
> release. I suppose all that adds up to a +1 from me then.

Yeah our ci has yet no way of running against a release branch.. Maybe a manually triggered run could work

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-07: