Page MenuHomePhabricator

ecore_evas: Introduce cnp / dnd API for ecore evas
Needs ReviewPublic

Authored by bu5hm4n on Sun, Jan 26, 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
Branch
devs/bu5hm4n/work_cnp
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16081
bu5hm4n created this revision.Sun, Jan 26, 3:35 AM
bu5hm4n requested review of this revision.Sun, Jan 26, 3:35 AM

What docs need improvement? The ones in src/lib/ecore_evas/Ecore_Evas.h are pretty exhaustive!

bu5hm4n updated this revision to Diff 28516.Mon, Jan 27, 1:56 AM
bu5hm4n edited the summary of this revision. (Show Details)

fixes & more docs

bu5hm4n updated this revision to Diff 28526.Mon, Jan 27, 4:35 AM

fix eina api names

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

fixes + also call changed event for self-caused events

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

change tests

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

More fixes and progress

devilhorns requested changes to this revision.Fri, Feb 14, 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.Fri, Feb 14, 7:34 AM
bu5hm4n updated this revision to Diff 29072.Sat, Feb 15, 12:52 PM

fixes & rebase

bu5hm4n marked 9 inline comments as done.Sat, Feb 15, 12:56 PM
zmike requested changes to this revision.Tue, Feb 18, 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.Tue, Feb 18, 7:54 AM
bu5hm4n planned changes to this revision.Tue, Feb 18, 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.Tue, Feb 18, 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.Tue, Feb 18, 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.Tue, Feb 18, 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.Wed, Feb 19, 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.Wed, Feb 19, 11:05 AM

work on proposed changes

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

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

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

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