Page MenuHomePhabricator

rewrite efl cnp and dnd handling
Changes PlannedPublic

Authored by bu5hm4n on Sun, Jan 26, 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
Branch
devs/bu5hm4n/work_cnp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16083
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n updated this revision to Diff 28564.Mon, Jan 27, 11:34 PM

fix leaks, fix wrongly annotated drop,drop event

bu5hm4n updated this revision to Diff 28570.Tue, Jan 28, 1:33 AM

fix leaks & adjust to ecore evas emitting changed events for self caused buffer changes

bu5hm4n updated this revision to Diff 28977.Wed, Feb 12, 5:52 AM
bu5hm4n edited the summary of this revision. (Show Details)

More fixes and progress

bu5hm4n updated this revision to Diff 28978.Wed, Feb 12, 5:57 AM
bu5hm4n edited the summary of this revision. (Show Details)

More fixes and progress

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

fixes for efl.ui.textbox

segfaultxavi requested changes to this revision.Thu, Feb 13, 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.Thu, Feb 13, 12:27 AM
bu5hm4n updated this revision to Diff 29077.Sat, Feb 15, 12:53 PM
bu5hm4n edited the summary of this revision. (Show Details)

fixes & rebase

zmike requested changes to this revision.Tue, Feb 18, 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
9210

See above comment for this enum type

9239

EINA_ARRAY_ITER_NEXT?

9319

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.Tue, Feb 18, 10:27 AM
bu5hm4n added inline comments.Wed, Feb 19, 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
9210

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

9239

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.Wed, Feb 19, 8:44 AM
bu5hm4n updated this revision to Diff 29141.Wed, Feb 19, 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.Thu, Feb 20, 12:29 AM
zmike requested changes to this revision.Thu, Feb 20, 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
9210

Then change the return type of the function to int.

9239

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.Thu, Feb 20, 6:58 AM
bu5hm4n planned changes to this revision.Thu, Feb 20, 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
9239

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.Thu, Feb 20, 9:09 AM
src/lib/elementary/efl_ui_textbox.c
541–542

Oh, that's true.