Page MenuHomePhabricator

ecore_evas: Introduce cnp / dnd API for ecore evas
ClosedPublic

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

Details

Summary

The idea of copy and paste here is:

  • The user specifies the content he wants to have in the selection buffer with a Eina_Content, these content pointer ownerships are passed to the called. Internally ecore_evas code will memorieze the pointer, and pass on function callbacks to the modules, which then do not have to deal with the ownership.
  • In case the module does not specify these APIs, the callback implementation will be called, which only works for cnp *not* dnd.
  • Action and mime types are handled as strings, which allows way better custom organisations.

(The docs needs improvement)

Depends on D11018

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

fix eina api names

bu5hm4n updated this revision to Diff 28569.Jan 28 2020, 1:31 AM

fixes + also call changed event for self-caused events

bu5hm4n updated this revision to Diff 28580.Jan 28 2020, 5:54 AM

change tests

bu5hm4n updated this revision to Diff 28972.Feb 12 2020, 5:51 AM

More fixes and progress

devilhorns requested changes to this revision.Feb 14 2020, 7:34 AM
devilhorns added a subscriber: devilhorns.
devilhorns added inline comments.
src/lib/ecore_evas/ecore_evas.c
5462

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5650

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5656

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5662

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5682

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5706

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5719

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5738

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

5750

As this is a forward facing API, we should probably check for a valid Ecore_Evas here before we try to set the function pointer.

src/lib/elementary/meson.build
950

This looks like an erroneous comma here...

This revision now requires changes to proceed.Feb 14 2020, 7:34 AM
bu5hm4n updated this revision to Diff 29072.Feb 15 2020, 12:52 PM

fixes & rebase

bu5hm4n marked 9 inline comments as done.Feb 15 2020, 12:56 PM
zmike requested changes to this revision.Feb 18 2020, 7:54 AM
zmike added inline comments.
src/lib/ecore_evas/Ecore_Evas.h
3716

Missing return value docs.

3738

Why isn't this just an array?

3803

This is missing proper Drag namespacing.

3827

This is missing proper Drag namespacing.

3866

Seems like this should be part of doxygen?

src/lib/ecore_evas/ecore_evas.c
5511

Can you remove the ret = part from this macro? It makes the below code harder to read since there's no obvious setting of the returned value.

5736

Is there really a point to freeing this every time vs just leaving it? A hash like this isn't going to use any noticeable amount of memory, and the additional maintainability burden from having multiple deallocation paths doesn't seem worthwhile to me.

src/lib/ecore_evas/ecore_evas_fallback_selection.c
62

Can you use EINA_ARRAY_ITER_NEXT here?

src/lib/ecore_evas/ecore_evas_private.h
250

What happens if multiple selection requests are in flight simultaneously? I guess it doesn't matter since we never actually use this pointer for anything other than setting and unsetting an unused pointer?

src/lib/elementary/meson.build
950

Trailing commas are fine in meson, but that's a keen eye for detail you've put to use here.

src/tests/elementary/efl_ui_window_cnp_dnd_slave.c
123

Pretty sure this can be more normally accessed using e.g., Efl_Loop_Arguments *arge = ev->info; which has argc and argv members.

This revision now requires changes to proceed.Feb 18 2020, 7:54 AM
bu5hm4n planned changes to this revision.Feb 18 2020, 8:21 AM

@segfaultxavi takes care of the documentation, how should we handle it we do not conflict that hard with the revisions on top ?

src/lib/ecore_evas/Ecore_Evas.h
3738

Because a iterator is enough ... the user of this API can now provide whatever data type serves best for him... (And the higher elm layer can just pass through the iterator)

src/lib/ecore_evas/ecore_evas.c
5511

Then i would handeval this a few times. Additionally, this file already contains the IFC IFE magic which is simular...

5736

One hash is not using that much. However, you have one has per window, which, (in case you have many windows) does bloat up the memory usage quite a bit in apps like elm_test.
Also the hash has a allocated bucket with the tree in it, as the first item was in it.
(Btw. you dont have multiple deallocation paths, every drag that is in the window, gets a leave event when the client quits, so this here is the only deallocation path.

src/lib/ecore_evas/ecore_evas_fallback_selection.c
62

Mhm, i did not want to use that here as the usage is somewhat clumsy. I would need an additional field just to store array->data, which is just bloating up the code, basically for nothing.

src/lib/ecore_evas/ecore_evas_private.h
250

if multiple selections are requested, the previos request is canceld with a error message indicating that there was an error. This is the same behaviour we had in the old elm implementation, and it seems we never hit this limitation over the years.

(in terms of values, requested_selection will first be set to NULL, then to the new request)

src/tests/elementary/efl_ui_window_cnp_dnd_slave.c
123

True - did not know about that array there. nice!

zmike added inline comments.Feb 18 2020, 9:29 AM
src/lib/ecore_evas/ecore_evas.c
5511

Yes, and I've been hating that for years as well, so it would be great if we could avoid extending the problem further here.

src/lib/ecore_evas/ecore_evas_private.h
250

I don't think this is correct behavior. Consider:

  • Requesting a primary selection and clipboard selection simultaneously, as in the potential case of a clipboard manager
  • Requesting the same selection across multiple seats

Both of these cases seem realistic and valid to me.

bu5hm4n added inline comments.Feb 18 2020, 9:38 AM
src/lib/ecore_evas/ecore_evas_private.h
250

Mhm, okay, I thought you were talking about requesting twice the same buffer.

The case you described should just work I think. And the selection ptr will be overwritten.
I think it should just be removed. Anything using it will likely do faulty things.

Requesting the same from multiple seats will not work. As the implementations are simply not supporting that, and never did that, that's just something that needs to be added at some point. This stack here is already overloaded, I would rather prefer doing that a point where this landed, and we actually have a testable case.

zmike added a comment.Feb 18 2020, 9:47 AM

Also I think for completeness there should be an OTHER enum value for the selection buffers for e.g., covering X11 secondary selection, even though this is not a commonly used functionality.

zmike added inline comments.Feb 19 2020, 6:18 AM
src/lib/ecore_evas/ecore_evas_private.h
250

Missed this comment somehow.

I'd be okay with removing the pointer since we're not using it I suppose?

bu5hm4n updated this revision to Diff 29139.Feb 19 2020, 11:05 AM

work on proposed changes

zmike requested changes to this revision.Feb 20 2020, 6:41 AM

My comments in Ecore_Evas.h still haven't been addressed.

This revision now requires changes to proceed.Feb 20 2020, 6:41 AM
bu5hm4n requested review of this revision.Feb 20 2020, 6:59 AM

I asked @segfaultxavi about that, as he handles all the config stuff in his revision, that is ontop of that.

vtorri added a subscriber: vtorri.Feb 24 2020, 4:23 AM
vtorri added inline comments.
src/tests/elementary/efl_ui_window_cnp_dnd_slave.c
26

use eina_str_has_prefix() instead ?

vtorri requested changes to this revision.Feb 24 2020, 5:36 AM
vtorri added inline comments.
src/lib/ecore_evas/ecore_evas.c
5511

if content is NULL, then segfault with eina_content_type_get()

This revision now requires changes to proceed.Feb 24 2020, 5:36 AM
vtorri added inline comments.Feb 24 2020, 7:49 AM
src/lib/ecore_evas/ecore_evas.c
5512

converted might be uninitialized

bu5hm4n updated this revision to Diff 29193.Feb 24 2020, 9:22 AM

Add log-domain, fix _deliver_cb

zmike requested changes to this revision.Feb 25 2020, 7:30 AM

I asked @segfaultxavi about that, as he handles all the config stuff in his revision, that is ontop of that.

I checked his diff, and the typedef changes aren't done there either, so my comments are still unresolved. All new types must be namespaced.

This revision now requires changes to proceed.Feb 25 2020, 7:30 AM

Even after reading my own diffs I don't know what are you talking about. Is there something I have forgotten to do?

zmike added a comment.Feb 26 2020, 5:13 AM

My comments are regarding the lack of dnd namespacing on the new API in Ecore_Evas.h. I don't see any fixes for this later in the series (which I'd be fine with) either.

OOOKKKK, understood. There's definitely some methods that lack the Drag namespace, which @zmike has already marked with inline comments ("This is missing proper Drag namespacing.")
The rest of methods seem to be correctly using the selection or drag namespaces.

Now fixed in D11426, at the top of this patch stack.

zmike accepted this revision.Feb 26 2020, 8:47 AM
bu5hm4n updated this revision to Diff 29274.Tue, Mar 3, 4:27 AM

rebase & update the complete stack to work arround issues

bu5hm4n updated this revision to Diff 29291.Tue, Mar 3, 5:00 AM

Better error handling for the case of a not existing seat

bu5hm4n updated this revision to Diff 29313.Tue, Mar 3, 7:01 AM

fix seat handling in fallback

zmike accepted this revision.Tue, Mar 3, 9:15 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sun, Mar 8, 3:02 AM
Closed by commit rEFL39f3ce42dcde: ecore_evas: Introduce cnp / dnd API for ecore evas (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.