Page MenuHomePhabricator

rewrite efl cnp and dnd handling
ClosedPublic

Authored by bu5hm4n on Jan 26 2020, 3:30 AM.

Details

Summary

the previous commits introduced a abstraction for drag in drop which can
be now used for this here. With this commit all the direct protocol
handling in efl.ui is removed, and only the ecore evas API is used.

Additionally, this lead to a giant refactor of how APIs do work. All
Efl.Ui. interfaces have been removed except Efl.Ui.Selection and
Efl.Ui.Dnd, these two have been restructored.
A small list of what is new:

  • In general no function pointers are used anymore. They feel very uncompftable in bindings and in C. For us its a lot easier to just

listen to a event when a drop enters or leaves, there is no need to
register custom functions for that.

  • Asynchronous data transphere is handled via futures, which proved to be more error safe.
  • Formats and actions are handled as mime types / strings.
  • 0 is the default seat if you do not know what else to take.
  • Content is in general passes as a content container from eina, this also allows applications to pass custom types

The legacy dnd and cnp API is implemented based on that.
All cnp related things are in elm_cnp.c the dnd parts are in elm_dnd.c

Depends on D11329

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n updated this revision to Diff 28978.Feb 12 2020, 5:57 AM
bu5hm4n edited the summary of this revision. (Show Details)

More fixes and progress

bu5hm4n updated this revision to Diff 28990.Feb 12 2020, 11:30 AM

fixes for efl.ui.textbox

segfaultxavi requested changes to this revision.Feb 13 2020, 12:27 AM

I am afraid this does not apply anymore on top of current master due to changes to efl_ui_textbox.c.

This revision now requires changes to proceed.Feb 13 2020, 12:27 AM
bu5hm4n updated this revision to Diff 29077.Feb 15 2020, 12:53 PM
bu5hm4n edited the summary of this revision. (Show Details)

fixes & rebase

zmike requested changes to this revision.Feb 18 2020, 10:27 AM

I tried

src/lib/elementary/efl_ui_dnd.c
74

Is there a point to having this?

src/lib/elementary/efl_ui_dnd.eo
12

Is there a reason why this is no longer an enum?

34

Same as above

45–46

For consistency shouldn't this be more like drop_selection_get? data seems to not quite fit here since it's a selection buffer.

59

This should probably at least provide seat info.

60

Seat info.

src/lib/elementary/efl_ui_selection.c
52

Do we really need this?

src/lib/elementary/efl_ui_selection.eo
3–21

This needs a -1 value if you're going to be returning an error ever.

16

Can we standardize parameter ordering here to always do buffer first, seat second, and then whatever else

src/lib/elementary/efl_ui_textbox.c
446

Strong function name.

541–542

This leaks the iterator?

1960–1962

iterator leak

src/lib/elementary/efl_ui_win.c
9213

See above comment for this enum type

9242

EINA_ARRAY_ITER_NEXT?

9322

tmp is leaked

src/lib/elementary/elm_cnp.c
34

Shouldn't these be stringshared like the rest?

138

selection

src/lib/elementary/elm_dnd.c
41

private

136

stringshare?

217

This leaks all the stringshares

src/lib/evas/canvas/evas_main.c
197

These should probably be in elm_init...

This revision now requires changes to proceed.Feb 18 2020, 10:27 AM
bu5hm4n added inline comments.Feb 19 2020, 8:44 AM
src/lib/elementary/efl_ui_dnd.c
74

wad?

src/lib/elementary/efl_ui_dnd.eo
12
  • In X you can pass arbitary names (in theory)
  • In WL you can pass arbitary ints as action (in theory)

With this here we can simply enable that, and let a app decide its custom action it wants to have.

If we have a enum, we again run into the issue that we have well defined keys, which we should support on any platform, but we cannot be sure to support them everywhere.

The action things are anyways very wobbly, as we have basically not a single user for it.

src/lib/elementary/efl_ui_selection.c
52

?

src/lib/elementary/efl_ui_selection.eo
3–21

This is soley ever used for indicating which buffer to use.

src/lib/elementary/efl_ui_textbox.c
541–542

The ownership of the iterator goes to the callee.

1960–1962

Same as above.

src/lib/elementary/efl_ui_win.c
9213

This is just internal, we have that quite a few times in efl like that ...

9242

the same as in the other revision, this is super clumsy to use. I am planning to write a Coccinelle patch when all this is landed, and use a new macro. Other code will also benefit from it.

src/lib/elementary/elm_cnp.c
34

Nothing in here should be stringshared... I removed them above.

src/lib/elementary/elm_dnd.c
136

no - not needed.

217

Not filled with stringshares.

src/lib/evas/canvas/evas_main.c
197

The elm functions for that are just redirecting to evas API, so this seems fine to go into evas?

bu5hm4n planned changes to this revision.Feb 19 2020, 8:44 AM
bu5hm4n updated this revision to Diff 29141.Feb 19 2020, 11:09 AM

update due to ee changes & work on review comments

I should probebly say that the default handling here and in the wayland impl. absolutly flawned. If 0 is ever a valid seat id in wayland, it will just be overwritten with whatever the default is. On the other side, with the old API and this here we are teaching a user that if you are not interested in seats, just pass 0 it will be fine. Which is absolutly wrong. Maybe we should think about providing a "seatless" version of this API which will just always default to the default seat ? On the other side, i do not want to change too much in these revisions, they are already quite unmanagable, and just gigantic...

segfaultxavi resigned from this revision.Feb 20 2020, 12:29 AM
zmike requested changes to this revision.Feb 20 2020, 6:58 AM
zmike added inline comments.
src/lib/elementary/efl_ui_dnd.c
74

It doesn't do anything besides store ee, which isn't actually necessary for anything.

src/lib/elementary/efl_ui_dnd.eo
12

Good explanation

src/lib/elementary/efl_ui_selection.c
52

See above

src/lib/elementary/efl_ui_selection.eo
3–21

Still an enum, so if you're returning an enum value in functions then you need one to indicate errors. Otherwise you need to always return an int where you might have an error.

src/lib/elementary/efl_ui_textbox.c
541–542

Having read the code, it seems more like the iterator is passed in, then immediately converted to an array and the pointer is instantly lost.

1960–1962

.

src/lib/elementary/efl_ui_win.c
9213

Then change the return type of the function to int.

9242

I'm not sure how it's clumsy here? This is exactly the case the macro exists for.

src/lib/elementary/elm_cnp.c
34

But not below?

src/lib/evas/canvas/evas_main.c
197

The mime type implies that we're using elm, and we don't want to be handling this if we aren't actually using elm.

This revision now requires changes to proceed.Feb 20 2020, 6:58 AM
bu5hm4n planned changes to this revision.Feb 20 2020, 7:16 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_ui_dnd.c
74

the ee is usefull to cache, since i do not have to query the evas and the ecore evas later on.

src/lib/elementary/efl_ui_selection.eo
3–21

Enum is never returned. Only used as an indication in the parameters, as others have complained using a bool for something like that.

src/lib/elementary/efl_ui_textbox.c
541–542

what ? efl_ui_dnd_drop_data_get calls ecore_evas_selection_get which calls _iterator_to_array_stringshared which calls eina_iterator_free.

src/lib/elementary/efl_ui_win.c
9242

You need an additional field Array Iterator, which is annoying, and nothing i wanted to add.

src/lib/elementary/elm_cnp.c
34

Damn - i forgot them. They simply shounld not be stringshares.

src/lib/evas/canvas/evas_main.c
197

But its evas textblock markup, whatever it was called, it is defined in this entity, where this is added.

zmike added inline comments.Feb 20 2020, 9:09 AM
src/lib/elementary/efl_ui_textbox.c
541–542

Oh, that's true.

bu5hm4n updated this revision to Diff 29196.Feb 24 2020, 9:27 AM

fix wrong seat value in selection_get

bu5hm4n updated this revision to Diff 29197.Feb 24 2020, 10:43 AM

remove unneeded eina_stringshare

zmike accepted this revision.Feb 25 2020, 7:32 AM

Probably good enough

This revision is now accepted and ready to land.Feb 25 2020, 7:32 AM
bu5hm4n updated this revision to Diff 29279.Tue, Mar 3, 4:29 AM

rebase & update the complete stack to work arround issues

bu5hm4n updated this revision to Diff 29292.Tue, Mar 3, 5:02 AM

add default seat handling into legacy handling

Closed by commit rEFL165f6f0ae285: rewrite efl cnp and dnd handling (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhySun, Mar 8, 3:03 AM
This revision was automatically updated to reflect the committed changes.