Page MenuHomePhabricator

elput: add/remove/get Ecore_Device
AbandonedPublic

Authored by ohduna on Jul 20 2016, 10:37 PM.

Details

Summary

When devices are changed, elput add/remove ecore_devices and send Ecore_Event_Device_Info events. Then ecore_input_evas add/remove evas_devices from those events.
When elput gets input events, elput finds ecore_device and store it in Ecore_Events. Then ecore_input_evas finds evas_device and store it in Evas_Events.
Now enlightenment(server) can get ecore_device/evas_device from events.

Test Plan
  1. Enlightenment gets Evas_Event_Mouse_Down.
  2. You can get source device information from ev->dev. evas_device_class_get(), evas_device_name_get(), evas_device_description_get() etc.

Diff Detail

Repository
rEFL core/efl
Branch
devs/jpeg/ecore_device
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2363
Build 2428: arc lint + arc unit
ohduna updated this revision to Diff 9618.Jul 20 2016, 10:37 PM
ohduna retitled this revision from to elput: add/remove/get Ecore_Device.
ohduna updated this object.
ohduna edited the test plan for this revision. (Show Details)
ohduna added reviewers: gwanglim, raster, jpeg, ManMower, zmike.
ohduna added subscribers: input.hacker, JHyun.
devilhorns requested changes to this revision.Jul 21 2016, 6:59 AM
devilhorns edited edge metadata.
devilhorns added inline comments.
src/lib/elput/elput_input.c
184

This flag is useless .. just check if em->window is set

198

This flag is useless .. just check if em->window is set

420

What happens to 'dev' here after it is added ?? I don't see this Ecore_Device getting used anywhere, or added to a list, or anything ??

422

It would make more sense to just fetch this Once

437

Why are we checking the return of ecore_device_iterate() Here, but not in the above _elput_input_ecore_device_add function ??

511

Why are we not checking the return value here for iterator ?

src/lib/elput/elput_private.h
258

This flag is completely useless .. we set it Once to always TRUE .. it is never set to anything else, so why have it ??

This revision now requires changes to proceed.Jul 21 2016, 6:59 AM
ManMower edited edge metadata.Jul 21 2016, 7:34 AM

Due to the way libinput works, this can't possibly work as intended if there are multiple devices in a seat.

Put two keyboards in a single seat.
Hold "a" on keyboard 1. event with device 1 is emitted.
Hold "a" on keyboard 2. NO EVENT
Release "a" on keyboard 1. NO EVENT
Release "a" on keyboard 2. event with device 2 is emitted.

What good did that do? We now think keyboard 1 is still repeating a key and keyboard 2 is sending spurious events?

Or this:
Hold left shift on keyboard 1. event with device 1 is emitted. modifiers show "shift"
Hold right shift on keyboard 2. event with device 2 is emitted. modifiers show "shift"
Release shift on keyboard 1. event with device 1 is emitted. modifiers show "shift"
<keyboard 1 will be typing capital letters even though it has no shift down>
Release shift on keyboard 2. event with device 2 is emitted. modifiers empty.

If left shift is used on both keyboards then the modifiers act the same, but there is no keyboard 2 press and no keyboard 1 release.

Input seats are abstracted for us by libinput, and it combines modifiers and press/release state to make the seat act as a single device. Any attempt to separate the devices will see failures like the above.

Mice are combined in exactly the same way. if mouse 1 holds button 1, mouse 2 button 1 presses are discarded.

Why does enlightenment need the source of an event anyway? It already knows what seat they come from, which is enough to send wayland protocol events to clients.

If we want independent events from devices we must use seats - they can be physical seats (configured by udev) or logical seats (elput API will be needed to configure this - too late for efl 1.18 I think)

But it's impossible to break up seat input into originating devices - information has already been lost by libinput.

ohduna updated this revision to Diff 9626.Jul 21 2016, 5:56 PM
ohduna edited edge metadata.
ohduna marked 7 inline comments as done.

I revised according to devilhorns comment.
Thanks,

src/lib/elput/elput_input.c
184

We already discussed the need of this flag.
em->window could be zero in different kernels since this value is set from drm, so we cannot determine the value is set or not if the value is zero.
please refer to the following diff..
https://phab.enlightenment.org/D3858

198

explained above.

511

EINA_ITERATOR_FOREACH handles NULL list, don't need to check. Just skip the for loop and return.

src/lib/elput/elput_private.h
258

explained above

Dear ManMower,

We understand the way libinput works.
The device info is additional infomation getting from libinput_device_get_sysname() directly.
If libiput emits PRESS of keyboard 1 and RELEASE of keyboard 2, we'd like to send device info(keyboard 1 and 2) with PRESS/RELEASE events.
If you worry that device info breaks up the consistency of event pair, please share your idea.

We also consider a seat that should be a group of device. One seat can have more than one keyboard. So we cannot distinguish devices using seats.

Thanks,

Sorry, I wasn't sure you understood libinput's seats well.

I would like to help, but I don't know what the device information will be used for. My first guess was to allow multi-player games or multi-user input, but this can't be done within a single seat due to locking out each others input and sharing modifiers.

What is the intended use case?

A seat can contain multiple devices, but (as you know) libinput can assign the devices to different logical seats. Adding api to elput to let enlightenment separate devices into their own logical seats would let us assign a single device to each seat. Wayland protocol for listing the devices in a seat would then let us know what the name of the device is for an event, and we wouldn't lose coherency.

EFL apps already listen for all seats, so this would likely not cause any problems for EFL. I've run EFL apps with two seats configured and not seen any obvious issues.

jpeg edited edge metadata.Jul 25 2016, 1:35 AM

Sorry, I wasn't sure you understood libinput's seats well.

I did not so thanks for explaining :-)

I would like to help, but I don't know what the device information will be used for. My first guess was to allow multi-player games or multi-user input, but this can't be done within a single seat due to locking out each others input and sharing modifiers.

As an external observer, I would say device information is useful mostly for multi player games, or potentially if a single player has multiple input devices. I see no reason why splitting devices into different seats should be a problem. Unless a seat conceptually means exactly one (human) user.

Crazy situations with multiple keyboard and multiple mice will lead to crazy inputs. That seems pretty obvious as long as they are all grouped under a common seat/user.

In D4179#69802, @jpeg wrote:

As an external observer, I would say device information is useful mostly for multi player games, or potentially if a single player has multiple input devices. I see no reason why splitting devices into different seats should be a problem. Unless a seat conceptually means exactly one (human) user.

Well, seat is defined as a group of input that goes together. I think that if we ever meet alien, we should allow them to use a seat like any human user. Therefor I am not sure we should enforce the link with a human user :-)

devilhorns added inline comments.Jul 26 2016, 11:50 AM
src/lib/elput/elput_input.c
184

We could easily make the default value be -1 .. then if it is anything other than that you could know if it is set.

198

See reply above

ohduna updated this revision to Diff 9653.Jul 27 2016, 12:47 AM
ohduna edited edge metadata.

revised and rebased.
Thanks,

ohduna updated this revision to Diff 9654.Jul 27 2016, 1:07 AM

rebased again.. sorry..

ohduna updated this revision to Diff 9655.Jul 27 2016, 1:47 AM

I had trouble with cute arcanist.. :(

ohduna abandoned this revision.Jul 27 2016, 7:36 PM

Thanks for all your comments.

I had a trouble in rebasing on master
I made new diff (https://phab.enlightenment.org/D4194)