Page MenuHomePhabricator

elm_dnd: clean up the registered target object when elm_drop_target_del API is called
Needs RevisionPublic

Authored by herb on Mon, Sep 14, 4:09 AM.

Details

Summary

Add missing routine, The registered flag is set to EINA_FALSE when elm_drop_target_del is called.

@fix

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17317
Build 11580: arc lint + arc unit
herb created this revision.Mon, Sep 14, 4:09 AM
herb requested review of this revision.Mon, Sep 14, 4:09 AM
bu5hm4n requested changes to this revision.Mon, Sep 14, 4:48 AM
  1. This breaks legacy drop targets on none efl.ui.dnd elements.
  2. This breaks registering 2 events, unregistering 1 -> no event will be delivered anymore.
This revision now requires changes to proceed.Mon, Sep 14, 4:48 AM
herb added a comment.Fri, Sep 18, 4:01 AM
  1. This breaks legacy drop targets on none efl.ui.dnd elements.
  2. This breaks registering 2 events, unregistering 1 -> no event will be delivered anymore.

There is a case that drop target object cannot be registered after following calls

elm_drop_target_add(obj, ...);
elm_drop_target_del(obj, ...);
elm_drop_target_add(obj, ...);

When elm_drop_target_add -> del ->add functions are called like above.
drop target object cannot be re-registered again because pd->registered is already EINA_TURE (pd->registered set to EINA_TRUE at the first function call)
so _drop_event_register() function will not be called and then the target object will not be registered.

Making the unregister in line 231 conditionally for not widgets should fix that.

herb added a comment.Sun, Sep 20, 7:42 PM

Making the unregister in line 231 conditionally for not widgets should fix that.

Here is upstream efl example code.

printf("bg drop target add1 \n");
elm_drop_target_add(bg, ELM_SEL_FORMAT_TARGETS, NULL, NULL, NULL, NULL, NULL, NULL, _drop_bg_change_cb, NULL);
printf("bg drop target del \n");
elm_drop_target_del(bg, ELM_SEL_FORMAT_TARGETS, NULL, NULL, NULL, NULL, NULL, NULL, _drop_bg_change_cb, NULL);
printf("bg drop target add2 \n");
elm_drop_target_add(bg, ELM_SEL_FORMAT_TARGETS, NULL, NULL, NULL, NULL, NULL, NULL, _drop_bg_change_cb, NULL);

And the following result is current behavior, after bg drop target add2 the drop target is not registered.
bg drop target add1
drop event register:0x400000a76129
bg drop target del
drop event unregister:0x400000a76129
bg drop target add2

The following result is with this patch, after drop target add2, drop event is registered again
bg drop target add1
drop event register:0x400000a1f682
bg drop target del
drop event unregister:0x400000a1f682
bg drop target add2
drop event register:0x400000a1f682

I think currently the drop target is not re-registered after drop_target_del API is called and this seems to be wrong behavior.
is this normal behavior?

What you describe is definitly a bug which should be fixed yes.

However:

  1. The addition of _efl_ui_dnd_efl_object_event_callback_array_del causes two intertwined register calls fail (elm_drop_target_add,elm_drop_target_add,elm_drop_target_del) will cause the event to not be delivered, even though it was registered twice and only deleted once.
  2. The removal of _drop_event_unregister is causing that none widgets do not get removed from the array when they are not a target anymore, leading to lifetime issues.

Yes this is a bug which needs to be fixed, but its not a fix if it causes 2 more issues. The correct fix for this is to put a if (!efl_isa(obj, EFL_UI_WIDGET_CLASS) before _drop_event_unregister