Page MenuHomePhabricator

CnP and Dnd Compatibility
Open, Incoming QueuePublic

Description

Hello bu5hm4n,

I found some CnP and DnD issues during the test. I think we need to discuss this more.
Could you check and give me your opinion?

Drag and Drop

  1. Ensuring the order of drag enter and drag leave callbacks for drop targets.

I think we should ensure the EFL_UI_DND_EVENT_DROP_ENTERED, EFL_UI_DND_EVENT_DROP_LEFT event call orders
even though we moves the drag object very quickly.

Sometimes, In case of using drop targets more than two.
If we moves the drag object very fast, the Enter and Left events of drop targets come with the unexpected orders.

As-Is)
Enter -> Enter -> Left -> Left
To-Be)
Enter -> Left -> Enter -> Left (sequentially)

This ordering problem can be fixed by modifying the following codes.
In the codes, the event call order is depends on drop_targets orders.
we should make the event call depends on the pointer position.

Code Location : efl_ui_win.c : _motion_cb()

  1. ECORE_WL2_EVENT_DATA_SOURCE_END event handler is changed to ECORE_WL2_EVENT_DATA_SOURCE_DROP

After refactoring the DnD codes, the ECORE_WL2_EVENT_DATA_SOURCE_END event is changed to
ECORE_WL2_EVENT_DATA_SOURCE_DROP event and this make a problem backword compatibility issue.
This makes the problem that the drag done callback is not called after dropping the drag object.

Code Location : ecore_evas_wayland_common.c : _ecore_evas_wl_selection_init()

  1. After dropping the object, target->currently_inside should be reset in _drop_cb()

When the drag and drop is finished (In case of dropping the object), the target->currently_inside value should be false.
Otherwise, When we try to drag new object, the left event of the last dropped target will be called.

Code Location: efl_ui_win.c : _drop_cb()

  1. Why we use ecore_evas_callback_drop_state_changed_set() in _ee_backbone_init and shutdown?

Could you tell me the function of ecore_evas_callback_drop_state_changed_set(),
I think we don't need to use the function.

Code Location: efl_ui_win.c : _ee_backbone_init and _ee_backbone_shutdown

Copy and Paste

  1. When we try to copy twice by code, sending data is rejected by the ECORE_EVAS_SELECTION_BUFFER_LAST condition.

In case of trying to copy twice, the ev->serial value comes 0 in _wl_interaction_send()
and the sending data is rejected by the condition of ECORE_EVAS_SELECTION_BUFFER_LAST.

This is because when trying to copy twice by code twice, the input->display->serial is not updated.
and this makes the second copy failed.

I think we allow the sending data request even tough the input->display->serial is not updated.
(To allow the multiple copy by code)

Code Location : ecore_evas_wayland_common.c : _wl_interaction_send()

herb created this task.May 11 2020, 4:43 AM

Hi,

  1. I am not sure if i totally understand what you mean, the order of events here is defined by the order that the objects are added events to. I am not sure if it makes much sense to order the events in a way that you *first* get the entered and then the left. Depending on the gfx elements on the screen you could have two overlapping objects, which get entered called on, so if there is an assertion that the event flow will always be enter -> left -> enter -> left, then at least 2 overlapping objects do break this assertion. Can you explain to me for which usecase this would be important ? I am feeling like we would want to have a seperated API for that.
  1. Yep, can you provide a patch for that ? I am kind of short in time due to a meson regression, and just want to answer fastly here.
  1. I dont think so, a _drop_cb call should be followed something by a call to _enter_state_change_cb, which resets all these flags.
  1. The drop_state of a window describes if there is currently a drop inside the window or not. Moving in, or starting a drag within the window will result in that call. The correct flags are then set on the needed objects. A moving out, abort or drop of the drop operation should also result in a call to the callback set there, which then cleansup the flag accordingly.

CNP: The serial gets set when the clipboard is claimed. And that serial should be sent there all the time, or we are heading towards a protocol error. Can you find out where that serial gets set to 0 ? I just tested here with weston and ewl, and i can paste as often as i like when i selected something in elm_test.

herb added a comment.EditedMay 18 2020, 12:56 AM

@ bu5hm4n
I created the patch for resolving DnD-2 problem.
please review this :)
https://phab.enlightenment.org/D11846

herb added a comment.EditedMay 18 2020, 2:00 AM

Case: DnD-1
I think the drop left and drop entered event order are important.

There is a legacy dnd application.
and suppose that there are two drop target objects in the same window, one is left side and another one is right side.
the drop target objects are sharing the _dnd_enter_cb and _dnd_leave_cb callback function.
in addition, _dnd_enter_cb and _dnd_leave_cb use the same count variable(int count = 0) for counting movement of drag object.
in _dnd_enter_cb, increase count variable and in _dnd_leave_cb, decrease count variable.
so the count variable is can be always 0 or 1.

but after refactoring codes, sometimes If we drag the object very fast over the two drop targets.
There are moments when two events occur in _motion_cb(), one is drop left event and another one is drop enter event.
so the count variable can be 2 when the events comes like belows.
_dnd_enter_cb -> _dnd_enter_cb -> _dnd_leave_cb -> _dnd_leave_cb

count: 1        count: 2         count: 1        count: 0

I think this means there are compatibility problems.
and There can be unexpected behavior in the legacy dnd application.

This compatibility issue can be resolve the following pseudo code.
Ex) divide for loop and handle EFL_UI_DND_EVENT_DROP_LEFT first
_motion_cb() {

...
for ( ; ; )
  efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_LEFT, &ev);
for ( ; ; )
 efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_ENTERED, &ev);
...

}

As I said in the original reply, that is nothing you should do or rely on, as the positioning could overlap, and then you have another issue.
Further more, I just checked with efl-1.19, and the code looks like that:

if (dropable) // leave last obj and enter new one
  {
     cnp_debug("leave %p\n", last_dropable->obj);
     cnp_debug("enter %p\n", dropable->obj);
     last_dropable->last.in = EINA_FALSE;
     last_dropable->last.type = NULL;
     dropable->last.in = EINA_TRUE;
     EINA_INLIST_FOREACH_SAFE(dropable->cbs_list, itr, cbs)
        if ((cbs->types & dropable->last.format) && cbs->entercb)
          cbs->entercb(cbs->enterdata, dropable->obj);
     EINA_INLIST_FOREACH_SAFE(last_dropable->cbs_list, itr, cbs)
        if ((cbs->types & last_dropable->last.format) && cbs->leavecb)
          cbs->leavecb(cbs->leavedata, last_dropable->obj);
  }
else // leave last obj

So this was always *first* enter and then leave, which even looks quite intentional here.
However, when selection manager was introduced this behaviour was broken, into first leaving, then entering. But the behavior is absolutely not consistent, as the assertion, that only 1 element is going to be under the drop is simply wrong, and elements could overlap, which is breaking exactly that.

How about that: I am explcitly breaking the for loop into 3 loops, but keep the ordering (enter, leave, pos), and you can on tizen restore the behavior that was in the meantime (leave, enter, pos) ? The reason i would rather stick to first enter, then leave is simply about consistancy, as it keeps the same event flow for overlapping as well as none overlapping items. *and* it restores the event flow that was 3 versions ago. @stefan_schmidt @raster what do you think about that?

herb added a comment.EditedMay 18 2020, 5:19 AM

@bu5hm4n
Case: DnD-1 (more)

I think this seems to be bug, there is a case that drop_target is added twice.
how about remove _drop_event_register() in elm_drop_target_add()?

elm_dnd.c
elm_drop_target_add()
{
...
// this function will call _efl_ui_dnd_efl_object_event_callback_array_priority_add then _drop_event_register() is registered twice
efl_event_callback_array_add(obj, drop_target_cb(), target);
_drop_event_register(obj);
...
}

efl_ui_dnd.c
_efl_ui_dnd_efl_object_event_callback_array_priority_add()
{
...
_drop_event_register(obj);
...
}

herb added a comment.May 18 2020, 5:53 AM

@bu5hm4n
Case: DnD-4

I have question about _enter_state_change_cb(), I think in _motion_cb() we emit EFL_UI_DND_EVENT_DROP_LEFT, EFL_UI_DND_EVENT_DROP_ENTER already.
should we emit the event in _enter_state_change_cb()?
(before the drag enter and leave event are triggered by the drag object moves)

static void
_enter_state_change_cb(Ecore_Evas *ee, unsigned int seat EINA_UNUSED, Eina_Position2D p, Eina_Bool move_inside)
{

Efl_Ui_Win_Data *pd = _elm_win_associate_get(ee);
for (unsigned int i = 0; i < eina_inarray_count(pd->drop_target); ++i)
  {
     Ui_Dnd_Target *target = eina_inarray_nth(pd->drop_target, i);
     Eina_Rect rect = efl_gfx_entity_geometry_get(target->obj);
     Eina_Bool inside = eina_rectangle_coords_inside(&rect.rect, p.x, p.y);
     Efl_Ui_Drop_Event ev = {p, seat, ecore_evas_drop_available_types_get(ee, seat)};

     if (inside && move_inside)
       {
          target->currently_inside = EINA_TRUE;

// efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_ENTERED, &ev);

  }
else if (!move_inside && !target->currently_inside)
  {
     target->currently_inside = EINA_FALSE;

// efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_LEFT, &ev);

     }
}

}

bu5hm4n added a comment.EditedMay 18 2020, 10:13 AM

for Dnd-1: Yeah, issue. I have a fix here, will push when I find time.
for Dnd-4: Simply said: yes. _enter_state_change_cb is called when the drop somehow exists the window, or enters a window, that is not a motion event, and will not sent one (As we are not having the next position). All we know is, that the Drop has Entered or Left,. (The enter part is probebly only for consitancy). Due to the fact that these events are protected by target->currently_inside they are only emitted once. So this should not be any problem ?

For 1: can you check that: (I hope it fixes the issue :))

commit 736e700efd0c187b72371cdbd24521653e12ef9c (HEAD -> master)
Author: Marcel Hollerbach <mail@marcel-hollerbach.de>
Date:   Mon May 18 19:04:43 2020 +0200

    elm_dnd: do not register widgets twice

    we have to support none widgets, but we should not register widgets
    twice because of that.

diff --git a/src/lib/elementary/elm_dnd.c b/src/lib/elementary/elm_dnd.c
index 18ae659bdf..f0d1824aca 100644
--- a/src/lib/elementary/elm_dnd.c
+++ b/src/lib/elementary/elm_dnd.c
@@ -182,7 +182,8 @@ elm_drop_target_add(Evas_Object *obj, Elm_Sel_Format format,
    target->format = format;

    efl_event_callback_array_add(obj, drop_target_cb(), target);
-   _drop_event_register(obj); //this is ensuring that we are also supporting none widgets
+   if (!efl_isa(obj, EFL_UI_WIDGET_CLASS))
+     _drop_event_register(obj); //this is ensuring that we are also supporting none widgets
    if (!target_register)
      target_register = eina_hash_pointer_new(NULL);
    eina_hash_list_append(target_register, &obj, target);
herb added a comment.EditedMay 18 2020, 10:00 PM

hello, @bu5hm4n
for DnD-1: drop event register twice is not reproduced with the patch in my environment.

if (!efl_isa(obj, EFL_UI_WIDGET_CLASS))
  _drop_event_register(obj); //this is ensuring that we are also supporting none widgets

you can merge this.
Thanks :)

herb added a comment.EditedMay 18 2020, 10:50 PM

hello, @bu5hm4n
for Dnd-4
before EFL_UI_DND_EVENT_DROP_ENTERED and EFL_UI_DND_EVENT_DROP_LEFT event are triggered by only motion events.
but the events in _enter_state_change_cb() will emit additional event to the application.

so the events should be changed to other types(I think the events property is different) or removed.
because when we use elm_drop_target_add API

Ex)

elm_drop_target_add(Evas_Object *obj, Elm_Sel_Format format,
                Elm_Drag_State enter_cb, void *enter_data,
                Elm_Drag_State leave_cb, void *leave_data,
                Elm_Drag_Pos pos_cb, void *pos_data,
                Elm_Drop_Cb drop_cb, void *drop_data)

App user expects that the enter_cb and leave_cb are triggered by the motion events of drag object.

however we apply the following codes more, there are no dnd issues anymore.

  1. remove additional enter and leave event, also we can change this event to other type.

_enter_state_change_cb(Ecore_Evas *ee, unsigned int seat EINA_UNUSED, Eina_Position2D p, Eina_Bool move_inside)
{

Efl_Ui_Win_Data *pd = _elm_win_associate_get(ee);
for (unsigned int i = 0; i < eina_inarray_count(pd->drop_target); ++i)
  {
     Ui_Dnd_Target *target = eina_inarray_nth(pd->drop_target, i);
     Eina_Rect rect = efl_gfx_entity_geometry_get(target->obj);
     Eina_Bool inside = eina_rectangle_coords_inside(&rect.rect, p.x, p.y);
     Efl_Ui_Drop_Event ev = {p, seat, ecore_evas_drop_available_types_get(ee, seat)};

     if (inside && move_inside)
       {
          target->currently_inside = EINA_TRUE;
         //efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_ENTERED, &ev);
       }
     else if (!move_inside && !target->currently_inside)
       {
          target->currently_inside = EINA_FALSE;
         //efl_event_callback_call(target->obj, EFL_UI_DND_EVENT_DROP_LEFT, &ev);
       }
  }

}

  1. add the inside reset flag in _drop_cb() to prevent to trigger more in motion_cb() when drag is finished this is because when drag is finished, the target->currently_inside = EINA_FALSE will not called for drop target.

_drop_cb()
{

 ...
 for (unsigned int i = 0; i < eina_inarray_count(pd->drop_target); ++i)
  {
     Ui_Dnd_Target *target = eina_inarray_nth(pd->drop_target, i);
     Eina_Rect rect = efl_gfx_entity_geometry_get(target->obj);
     Eina_Bool inside = eina_rectangle_coords_inside(&rect.rect, p.x, p.y);

     if (inside)
       {
          EINA_SAFETY_ON_FALSE_GOTO(target->currently_inside, end);
          eina_array_push(tmp, target->obj);
          target->currently_inside = EINA_FALSE; // reset inside flag when drag is finished (dropped)
       }
  }
...

}

jykeon added a subscriber: jykeon.May 18 2020, 11:50 PM
herb added a comment.May 19 2020, 12:18 AM

hello, @bu5hm4n
for CnP-1
The serial is updated by only input codes, so when we try to copy and paste using button(the serial is updated and copy and paste work well)
ex)
src/lib/ecore_wl2/ecore_wl2_input.c:697: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:721: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:768: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:942: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:969: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:1062: input->display->serial = serial;
src/lib/ecore_wl2/ecore_wl2_input.c:1241: input->display->serial = serial;

The problem occurs when I copy and paste multiple times with code (not using button)
This case, the serial is updated once at the first elm_cnp_selection_set call.
In case of other selection set (copy text2, copy text3"), the serial is not updated (serial is 0) then copy will be failed.
ex)
elm_cnp_selection_set(obj, "copy text 1" ....);
elm_cnp_selection_set(obj, "copy text 2" ....);
elm_cnp_selection_set(obj, "copy text 3" ....);

To resolve this issue, we can modify the following code.
The reason that data is not sent is there is a condition code to reject no sent_serial in _wl_interaction_send()

static Eina_Bool
_wl_interaction_send(void *data, int type EINA_UNUSED, void *event)
{

...
 // if sent_serial is 0, the data send will not be proceed, before we allowed to send data even if sent_serial is 0
 if (ev->serial == wdata->selection_data[ECORE_EVAS_SELECTION_BUFFER_COPY_AND_PASTE_BUFFER].sent_serial)
 ...

}

herb added a comment.EditedMay 19 2020, 12:36 AM

hello, @bu5hm4n
for CnP-2 (new case)
I have a question about elm_cnp_loss_callback_set() function.
I think the loss_callback behavior is changed compared to before.

before, the loss callback_function is called when another elm_cnp_selection_set() is called.
but now we call loss callback when the selection is changed (EFL_UI_SELECTION_EVENT_WM_SELECTION_CHANGED)
so there are cases the loss callback is not called correctly.

I suggest the following code to keep back word compatibility.

when we try to call elm_cnp_selection_set(A, ...);
the A should be a selection owner.
and then we call elm_cnp_selection_set(B, ...);
now the B is a selection owner and the A is not a selection owner anymore.
this time if the A has a loss callback, the callback should be triggered.

The following is the prototype code.

static Evas_Object *selection_owner = NULL;
EAPI Eina_Bool
elm_cnp_selection_set(Evas_Object *obj, Elm_Sel_Type selection, Elm_Sel_Format format, const void *buf, size_t buflen)
{
...
selection_owner = obj;
}

static void
_selection_changed_cb(void *data, const Efl_Event *ev)
{

if (selection_owner != ev->object)
  {
     ldata->loss_cb(ldata->udata, ldata->type);
     efl_event_callback_del(ev->object, ev->desc, _selection_changed_cb, data);
     free(data);
  }

}

Dnd-4: You have to differenciate between a few things: 1. you have motion, enter & leave for the window. Which is driven directly by the protocol stuff from x / wl / win32 / cocoa. There motion, enter, leave are then via code broken down to the per widget position, enter & leave events. The per widget enter / leave events can only be called ONCE before the opposite happens. The position happens inbetween as often as possible. The fact that widget-enter and widget-leave are sometimes called from motion and sometimes from window-enter or window-leave cannot be changed, or we will miss enter and leave events in certain situations (dnd started, entered, not moved, and then dropped). So the behaviour you see there is correct, and still after writing all this, i dont get what the issue with this is. It is not opaque to the app from where the event is coming, and the app does not have access to window-motion or window-enter, or window-leave, so the app *cannot* know that.
To your second point: No that is wrong, dropping terminates the drop action, which will result in a call to _enter_state_change_cb which does that. Resetting the flags in _drop will not emit the needed DROP_LEFT event, which we probebly want to have for usecases like: Changing the theme based on that state etc. and the LEFT simple resets it to the default theme

Cnp-1: Well, that issue does not seem in any way related to the cnp work then ? As its somewhere in the internals of ecore_wl2 ? Also, if you remove the condition that you have added to the comment, every window will paste its current selection_data (if there are more than 1), which is simply wrong. the event there *has* to carry the correct serial, or this will simply break.

Cnp-2: I can right now only test Xorg, but _selection_changed_cb is changed when we have the CNP buffer, and claim it again. If this is not happening in wl, then i guess we should look there into the backend to make that there happening as well. However, after quickly looking at the code in ecore_wl2, that should just happen the same way, the wayland compositor should sent that. (weston does)

herb added a comment.EditedMay 19 2020, 5:23 AM

for DnD-4(1): I think the event enter and left in _motion_cb, the event enter and left in _enter_state_change_cb are different. the events in motion_cb are triggered by motion.
but the events in _enter_state_change_cb are triggered by wayland enter and left events.
I have a doubt to serve same events about both cases.
for DnD-4(2): when the drag object moved to the target object and the drag is finished, the target->currently_inside flag should be false. but _enter_state_changed_cb dose not change the target->currently_inside flag to false when drag is done.

for CnP-1: I think this is the policy problem, we decide to allow the copy and paste without the input changes.
for CnP-2: The problem is that the call of loss_callback can be different depends on the server. I think it's better to give same result regardless the server.

Dnd-4:(1) For the API user we only want to say when a element has Entered or Left a widget, the background why or how this happens is not a subject to the API user. The more important part is that the behaviour is consistent, and the way it is right now is IMO consistant, the alternative not.
Dnd-4(2): I just tested that here, when i am starting a drag by a external client or the client itself, _enter_state_change_cb gets called, flag is set correctly, enter is emitted, i am dropping in that widget, dropping event is called, _enter_state_change_cb gets called, left gets called, flag is resetted.

CnP-1: Hum ? I am not sure how i should read that sentence. Can you bring some
CnP-2: Just tested, works here, EFL_UI_SELECTION_EVENT_WM_SELECTION_CHANGED is emitted every single time the selection of CNP is changed.

I have tested all this in weston, and the signal for WM_SELECTION_CHANGED and enter as well as leave are emitted by the compositor through a data device, are you 100% sure that the compositor is acting in the same way for you ?