Page MenuHomePhabricator

eo: enable priority with event forwarder.
ClosedPublic

Authored by cedric on Dec 19 2018, 2:05 PM.

Details

Summary

Note: Their isn't any ability to do something like a static array of
events at the moment. It might lead to large memory being used when it
wouldn't be necessary. If that was the case, we could fix it, but it
would require a lot of dynamic hash operation I think.

Depends on D7496

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.
cedric created this revision.Dec 19 2018, 2:05 PM
cedric updated this revision to Diff 17994.Dec 19 2018, 2:51 PM
cedric edited the summary of this revision. (Show Details)
bu5hm4n requested changes to this revision.Dec 20 2018, 11:41 AM
bu5hm4n added inline comments.
src/lib/eo/efl_object.eo
236–237

Format :)

244

Maybe we want to have a priority here too ?

This revision now requires changes to proceed.Dec 20 2018, 11:41 AM

Will fix.

src/lib/eo/efl_object.eo
236–237

Arf, I really need an .eo description for emacs.

244

Priority is useless when removing a callback as the previously registered callbacks could have been moved due to the insertion of other callback. So you can only walk the entire thing to try to find the one you are looking for.

cedric updated this revision to Diff 18046.Dec 20 2018, 3:09 PM

Rebase.

cedric updated this revision to Diff 18077.Dec 21 2018, 12:08 PM

Rebase and fix indentation.

bu5hm4n added inline comments.Dec 26 2018, 5:21 AM
src/lib/eo/efl_object.eo
244

well not really IMO. If you subscribe twice once before once after, and want to remove the after, so the before is still called, you need to unsubscribe twice, to subscribe again, maybe we should add the priority to all callbacks in eo ?

cedric added inline comments.Dec 28 2018, 10:10 AM
src/lib/eo/efl_object.eo
244

So what happen if you do insert twice with before and now you want to remove the first one you inserted? The first one is not anymore the before one as it has been superseded by the second before insertion. As a caller, you have no clue what the state of the stack of event handler on a object look like at any point in time. So I don't see how you could rely on that information to find something. Basically if you where to implement it, you would realize that you need to ignore the order parameter to find the event handler in all case.

bu5hm4n added inline comments.Dec 29 2018, 4:20 AM
src/lib/eo/efl_object.eo
244

Yeah but the difference is if i have two callbacks registered, with the identical parameters, then it does not matter which one is actaully removed. (Before just maps to a priority that is sorted.)

However, if i have a "before" and a "after" registered, it is possible for the user to want to keep the "before" registration, while removing the "after" which *is* a different paramtered call...

Also, we could use the priority as sort of entry into the callbacks array, so we *start* to search at the priority after, and iterate the full array by rolling over, this would serve the best usage for the user, while keeping the possibility of specifying which one to remove. :)

cedric added inline comments.Jan 2 2019, 11:27 AM
src/lib/eo/efl_object.eo
244

You really can't use the priority as a sort of entry into the callbacks array as we have no clue what happened. Let see: BEFORE (A), BEFORE (B), AFTER (C), DEFAULT (D), AFTER (E), BEFORE (F) would result in FBACDE, now you use the priority to remove the AFTER (C). What do we do ? DEFAULT (D) is in between the two AFTER. You have to linearly walk the entire think. I don't see a way around that.

As for making use of the PRIORITY as a key for all callback in our infra, I think this is a larger discussion than this patch as this is requesting a large change in our core object. Could you open a ticket to follow up on that discussion?

zmike added a subscriber: zmike.Jan 2 2019, 11:29 AM

Maybe this is already existing elsewhere and not in the diff, but can a little more doc be added regarding the purpose of an event callback forwarder?

In D7482#132194, @zmike wrote:

Maybe this is already existing elsewhere and not in the diff, but can a little more doc be added regarding the purpose of an event callback forwarder?

What would you like to see in the doc? I have mostly copy&pasted the existing doc when doing this API, so there isn't much anything different around.

We just should do it before we declare eo stable.

src/lib/eo/efl_object.eo
244

Yeah you are right, lets move this to some other ticket :)

Yeah, I looked at the existing doc and it doesn't really say anything either. Maybe outside the scope of this patch, but it would be good to get a bit more detail here I think?

Add an event callback forwarder for an event and an object.

Is the description, but it could maybe be improved to something like 'Add an event callback forwarder for an event and an object which propagates the event to the passed new_obj' ?

@segfaultxavi may have ideas here since he docs...

zmike added a comment.Jan 2 2019, 11:55 AM

To be clear, this is just discussion for another diff and my comments are not blocking the acceptance or merge-ability of this patch.

bu5hm4n accepted this revision.Jan 2 2019, 1:03 PM

Same for me - the formatting thing is fixed :)

This revision is now accepted and ready to land.Jan 2 2019, 1:03 PM
This revision was automatically updated to reflect the committed changes.

I'm too late for the party, let's just copy my comments to a new task.

src/lib/eo/Eo.h
2072

It wouldn't hurt to specify the type of this parameter, since this is a macro (see the macro above, for example).

src/lib/eo/efl_object.eo
233

I understand this was written like this before and it's not @cedric's responsibility to fix it... but this might be a good time anyway :)
So, what the heck is an event callback forwarder? I can add 1 + 1 and deduce what it is, but, as a user, I do not want to be doing that.
"Event Callback Forwarder" should be formally defined somewhere, and at the very least it needs: what it does and why (that is, an example of why a user would use it).

236–237

Haha, yeah, now this definitely needs some explanation. What is the priority? Why is it needed? What's the valid range?
The current comments talks about "inserting", so the priority is actually an index into a list?