Page MenuHomePhabricator

Ecore Evas Wayland: Create the devices during Ecore_Evas setup.
ClosedPublic

Authored by iscaro on Nov 10 2016, 10:13 AM.

Details

Summary

When launching an Elementary App using Wayland the elm_config will
automatically connect to the Wayland's display server and all events
regarding seats are lost, since by the time that Ecore_Evas is created
the global events were already dispatched. To fix this problem,
everytime an Ecore_Evas is created, the code must check if there
are any seat capabilities available, if so, the devices will be created.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
iscaro updated this revision to Diff 10078.Nov 10 2016, 10:13 AM
iscaro retitled this revision from to Ecore Evas Wayland: Create the devices during Ecore_Evas setup..
iscaro updated this object.
iscaro edited the test plan for this revision. (Show Details)

@jpeg This fixes the bug that you reported.

iscaro updated this revision to Diff 10079.Nov 10 2016, 1:04 PM

Changes since last version:

  • Do not create a seat if it already exists.
jpeg edited edge metadata.Nov 13 2016, 6:09 PM

Do we really need the distinction between NO_SEAT and NONE for capabilities? It looks confusing to me as cap != 0 does not mean the device is useful. If the device has only NONE capabilities, it's pretty damn useless (even if it's a 'valid' device). I would say merge NO_SEAT and NONE and the patch is good to go.

This fixes the issue indeed.

jpeg requested changes to this revision.Nov 13 2016, 6:10 PM
jpeg edited edge metadata.
This revision now requires changes to proceed.Nov 13 2016, 6:10 PM
In D4390#73706, @jpeg wrote:

Do we really need the distinction between NO_SEAT and NONE for capabilities? It looks confusing to me as cap != 0 does not mean the device is useful. If the device has only NONE capabilities, it's pretty damn useless (even if it's a 'valid' device). I would say merge NO_SEAT and NONE and the patch is good to go.

This fixes the issue indeed.

I'm glad that this patch fixed the issue for you.
Now, talking about NO_SEAT and NONE capabilities. I created them, because sometimes when I fetch the inputs (using ecore_wl2_display_inputs_get()) an input may have a seat but with no capabilities yet. These capabilities will be advertised later by the Ecore event ECORE_WL2_EVENT_SEAT_CAPABILITIES_CHANGED. If I merge the seat capabilities NO_SEAT and NONE I will need to create a new seat if its capabilities changes at _ecore_evas_wl_common_cb_seat_capabilities_changed().
What do you prefer?

jpeg added a comment.Nov 14 2016, 5:57 PM
In D4390#73737, @iscaro wrote:
In D4390#73706, @jpeg wrote:

Do we really need the distinction between NO_SEAT and NONE for capabilities? It looks confusing to me as cap != 0 does not mean the device is useful. If the device has only NONE capabilities, it's pretty damn useless (even if it's a 'valid' device). I would say merge NO_SEAT and NONE and the patch is good to go.

This fixes the issue indeed.

I'm glad that this patch fixed the issue for you.
Now, talking about NO_SEAT and NONE capabilities. I created them, because sometimes when I fetch the inputs (using ecore_wl2_display_inputs_get()) an input may have a seat but with no capabilities yet. These capabilities will be advertised later by the Ecore event ECORE_WL2_EVENT_SEAT_CAPABILITIES_CHANGED. If I merge the seat capabilities NO_SEAT and NONE I will need to create a new seat if its capabilities changes at _ecore_evas_wl_common_cb_seat_capabilities_changed().
What do you prefer?

Honestly I can't understand everything and can't figure out why that would be a problem. Can't there be a device with NONE capabilities like you would have one with NO_SEAT?

jpeg added a comment.Nov 15 2016, 12:19 AM

Due to time zone differences, I'll let you and your friends decide on the issue of NO_SEAT vs. NONE. The patch is otherwise good to go, and I believe should be merged in master.

jpeg accepted this revision.Nov 15 2016, 12:19 AM
jpeg edited edge metadata.

Due to time zone differences, I'll let you and your friends decide on the issue of NO_SEAT vs. NONE. The patch is otherwise good to go, and I believe should be merged in master.

This revision is now accepted and ready to land.Nov 15 2016, 12:19 AM
Closed by commit rEFL9ba11c5cd020: Ecore Evas Wayland: Create the devices during Ecore_Evas setup. (authored by iscaro, committed by Jean-Philippe Andre <jp.andre@samsung.com>). · Explain WhyNov 16 2016, 2:55 AM
This revision was automatically updated to reflect the committed changes.
jpeg added a comment.Nov 16 2016, 2:58 AM

So I uh... pushed the patch. Including NO_SEAT and NONE.

bdilly edited edge metadata.Nov 16 2016, 3:59 AM

It seems good overall. Just a few inline comments.
Indeed I failed to preview such a scenario.

src/lib/ecore_wl2/ecore_wl2_input.c
1580

The way you're handling CAPABILITIES_NONE seems wrong to me.
The way you've done CAPABILITIES_NONE flag always will be on.

Actually you could add a check after

if (input->wl.touch)

cap |= ECORE_WL2_SEAT_CAPABILITIES_TOUCH;

if (!cap) cap = ECORE_WL2_SEAT_CAPABILITIES_NONE

Or even better, just switch NONE and NO_SEAT values:

NONE = 0
NO_SEAT = 1

1584

Is this a possible condition?
An input without an associated seat?

src/modules/ecore_evas/engines/wayland/ecore_evas_wayland_common.c
576

Are you sure it's a possible scenario?
The same seat advertised twice by wayland?
Were you able to test it somehow?

582

It seems all you need here is a "break" statement. No need for err_add label

2055

This snprintf should be inside function common_seat_add()

In D4390#73851, @jpeg wrote:

So I uh... pushed the patch. Including NO_SEAT and NONE.

Thanks @jpeg . I was out of office. Returning today.
I'm suggesting a few changes that @iscaro may commit as an extra PR.

In D4390#73706, @jpeg wrote:

Do we really need the distinction between NO_SEAT and NONE for capabilities? It looks confusing to me as cap != 0 does not mean the device is useful. If the device has only NONE capabilities, it's pretty damn useless (even if it's a 'valid' device). I would say merge NO_SEAT and NONE and the patch is good to go.

This fixes the issue indeed.

Actually I can see a difference between NO_SEAT and NONE.
NONE always happens in a seat lifetime, btw. At first a seat is announced by wayland. Then you register callbacks for capabilities changes. And when the capabilities changes is called you'll have more capabilities (pointer / keyboard / touch). So in the meantime, during this interval, you'll have a seat with no capabilities. If a ecore_evas is created at that moment, a new seat evas device should be added, but not keyboard or mouse.

On the other hand, if it's capabilities is set to NO_SEAT, not even a seat evas device should be created.
But is it possible to have such scenario? An input without seat? If it is, we should have both NO_SEAT and NONE.
If not, just drop NO_SEAT.

In D4390#73890, @bdilly wrote:
In D4390#73706, @jpeg wrote:

Do we really need the distinction between NO_SEAT and NONE for capabilities? It looks confusing to me as cap != 0 does not mean the device is useful. If the device has only NONE capabilities, it's pretty damn useless (even if it's a 'valid' device). I would say merge NO_SEAT and NONE and the patch is good to go.

This fixes the issue indeed.

Actually I can see a difference between NO_SEAT and NONE.
NONE always happens in a seat lifetime, btw. At first a seat is announced by wayland. Then you register callbacks for capabilities changes. And when the capabilities changes is called you'll have more capabilities (pointer / keyboard / touch). So in the meantime, during this interval, you'll have a seat with no capabilities. If a ecore_evas is created at that moment, a new seat evas device should be added, but not keyboard or mouse.

On the other hand, if it's capabilities is set to NO_SEAT, not even a seat evas device should be created.
But is it possible to have such scenario? An input without seat? If it is, we should have both NO_SEAT and NONE.
If not, just drop NO_SEAT.

This is exactly what i tried to express by creating NO_SEAT and NONE, however when an input is created a seat is also created. So every input will be attached to a seat. I will just drop the NO_SEAT value.

src/lib/ecore_wl2/ecore_wl2_input.c
1580

Indeed.

1584

Indeed. This is invalid.

src/modules/ecore_evas/engines/wayland/ecore_evas_wayland_common.c
576

God. I made a confusion, because I had two Ecore Evas at the same time, so I thought they were duplicated.

I'm really sorry about this.

582

ok

2055

ok..