Page MenuHomePhabricator

eo: replace loop_get with interface_get
ClosedPublic

Authored by bu5hm4n on Apr 22 2016, 2:09 AM.

Details

Summary

interface_get is more generic, so other mechanisms can also reuse the
code.
The object itself has to support the function, so there is no need for
eo_isa which would have a negative performance impact.
The base class implementation calls interface_get on the parent, so a
override of the function can just call the super function to continue in
the recursion.

Test Plan

just run the eo test suite

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.
bu5hm4n updated this revision to Diff 8966.Apr 22 2016, 2:09 AM
bu5hm4n retitled this revision from to eo: replace loop_get with interface_get.
bu5hm4n updated this object.
bu5hm4n edited the test plan for this revision. (Show Details)
bu5hm4n added reviewers: tasn, raster.
raster requested changes to this revision.Apr 22 2016, 4:32 AM
raster edited edge metadata.

far far far far far more complexity for no gain. what is the gain? you remove a loop_get from base class? what VALUE does that have? we don't care about the loop class. we care about the loop OBJECT. the OWNING object so we can add timers, fd handlers, whatever to that specific loop and thus have callbacks called int he correct owning context (the loop). this has no point. and even worse... WHAT interface? the name is not useful - what interface am i getting?

This revision now requires changes to proceed.Apr 22 2016, 4:32 AM

There is no more complexity its the 1:1 mechanism you implemented, with the only difference that you can check for the class.

The mainpoint of this is to have smth. generic, this can also be usefull for example to find the owning focus manager when implementing focus as a graph, it can be very usefull to find the next widget at the top.

Maybe the documentation is unclear, so: you pass the function EFL_LOOP_CLASS and get the next object up in the object tree which from this type. So 1:1 the thing you implemented before, just in a more generic form.

bu5hm4n updated this revision to Diff 8967.Apr 22 2016, 5:14 AM
bu5hm4n edited edge metadata.

better function name

bu5hm4n abandoned this revision.Apr 22 2016, 8:30 AM

Since promises in eo are pulling in a mainloop to each and every object (which is ... retarded). And @raster preferece to have multiple xyz_get instead of a generic object_find. I am going to close this.

All in all i think its a very very poor way how with the critic on this is dealt:

  • Noone ever openly comunicated how promises should work, infact i think this is the badest way this could have ever been designed.
  • This merged a object system with a core system. which means that we have a cyclic dependency between core->eo->core, which makes the abstraction basically useless, since noone can reuse eolian for small objects which do not need a promise.
  • I really hate the attitude, of ignoring my arguments with "hah well deal with it, there cannot be any other solution because loop > 1". There can be a other soltution which means that other things also would need to be fixed, for example the promise. This hack is just the result of a earlier hack. and should NOT exists in the baseclass of a object-system.

Also some usecase which came up in my mind. I have a object tree with a mainloop at the top. I reference 3-4 objects out of that tree. I delete the mainloop. i will have max 3-4 objecttrees now, without a mainloop at the top. what happens now to the promises if there is still api async running ? or what happens if i now call apis which should return a promise ?

Since promises in eo are pulling in a mainloop to each and every object (which is ... retarded). And @raster preferece to have multiple xyz_get instead of a generic object_find. I am going to close this.

This is absolutely completely utterly false ! Promise do not depend on ecore. They come from eina and that's there only dependency.

This proposal is in my opinion way better than what we currently have. It does cover more possible use case. Focus is a small one, but Canvas lookup is another big one. @raster should we also add a canvas_get lookup, because so many object need it ? I think this API make much more sense.

src/lib/eo/eo_base_class.c
550

Shouldn't interface be a class ?

Also some usecase which came up in my mind. I have a object tree with a mainloop at the top. I reference 3-4 objects out of that tree. I delete the mainloop. i will have max 3-4 objecttrees now, without a mainloop at the top. what happens now to the promises if there is still api async running ? or what happens if i now call apis which should return a promise ?

As said a promise do not depend on the main loop, the object that return it do. If you delete the main loop, the object will get deleted too and all the promise will be canceled if this object did rely on the main loop to get that promise done.

@raster: we can always add inline helper function in C for canvas and loop lookup for example.

bu5hm4n reclaimed this revision.Apr 22 2016, 9:45 AM

Since @cedric also is for this proposal.

From the discussion earlier i thought promises are on the mainloop, sorry.

you cannot DRIVE a promise without a loop. they are non-functional without a loop. the whole point is to queue work for later and when results come back call the success/fail cb's. they thus need a loop to do that unless you just make them all synchronous in which case why do a promise? just return a value.

jpeg added a comment.Apr 22 2016, 8:39 PM
In D3909#64320, @cedric wrote:

This proposal is in my opinion way better than what we currently have. It does cover more possible use case. Focus is a small one, but Canvas lookup is another big one. @raster should we also add a canvas_get lookup, because so many object need it ? I think this API make much more sense.

Yes, totally agree here. The initial naming was poor, object_find makes more sense. Or maybe something like parent_find or whatnot. This API is much better than loop_get as it englobes loop_get, canvas_get, etc...

bu5hm4n added a comment.EditedApr 25 2016, 1:34 AM

Or maybe, add functions in efl.loop or evas.canvas which are typesafe, so you have smth. like a minimalisitc factory function. which just uses this eo.base object_find backend. Then you have your typesafe function, and the get functions are where you would expect them a bit more.

Or maybe, add functions in efl.loop or evas.canvas which are typesafe, so you have smth. like a minimalisitc factory function. which just uses this eo.base object_find backend. Then you have your typesafe function, and the get functions are where you would expect them a bit more.

This is a good point. We can provide an Efl.Loop_User mixin that Efl.Ui. object and all other Ecore object provide. This is definitively cleaner.

I am just with @jpeg , parent_find has a name make more sense in my opinion too (Still I am not a native speaker, so if someone has a better idea).

base_interface

if there are wrappers that are typesafe higher up. then this can work.

bu5hm4n updated this revision to Diff 8985.Apr 26 2016, 1:28 AM
bu5hm4n edited edge metadata.

update name

I really dont have any idea what just happend.

bu5hm4n updated this revision to Diff 8986.Apr 26 2016, 1:33 AM

maybe fix it ?

bu5hm4n updated this revision to Diff 8987.Apr 26 2016, 1:45 AM

remove klass names

bu5hm4n added inline comments.Apr 26 2016, 2:00 AM
src/lib/eo/eo_base_class.c
550

I have to name it klass, since class is going to b0rke ephysics and cpp ...

jpeg added a comment.Apr 26 2016, 3:43 AM

Shouldn't the default implementation (Eo.Base) use eo_isa, like the following:

if (eo_isa(obj, klass)) return obj;
else if (parent) return eo_provider_find(parent, klass);
else return NULL;

I know eo_isa is expensive, so I'm not sure if we should use it. I wonder.

i personally don't like provider_find. provider_get ... maybe but provider... hmm. owner? master?

I would like to go for provider_find since the get is a bit missleading, its not really a simple get operation, its searching for a provider for the given type. So i would like to keep provider_find.

I agree this is a find operation and not a get. Maybe parent_provider_find ?

it doesn't find a parent provider. it finds some kind of parent. but we already have a parent. there's loop_get which gets the loop parent (wherever it is even if not in the tree). its not a provider. its some kind of owner or master. it owns the object. e.g. the loop that owns that object (or the loop the object belongs to). it's not a provider. it's a kind of owner/manager/controller/something.

we currently have toplevel_get in elm legacy api - it may walk up the tree but it's a getter.

It doesnt do anything with the parent. It searches up in the object tree for a provider of the given instance. JUST the provider has to be a parent of the object. the object with the given type does not have to be in this parent<->child relation at all. Just the provider has to be. So i would go for this name.

jpeg added a comment.Apr 27 2016, 3:25 AM

It doesnt do anything with the parent. It searches up in the object tree for a provider of the given instance. JUST the provider has to be a parent of the object. the object with the given type does not have to be in this parent<->child relation at all. Just the provider has to be. So i would go for this name.

Ah ok then it's a bit different from what I had in mind, and it makes sense. This means basically that the Loop doesn't have to be a parent of objects using it, but those objects need a parent/grandparent/etc... that knows which Loop to use.

tasn accepted this revision.Apr 28 2016, 5:56 AM
tasn edited edge metadata.

I like it much better than loop get, that's for sure so it can already go in, but I have a few concerns still.

Is "class" a good "key" for this, or should it be key based? (more complicated and slower...). It should probably be good enough.

Also, "eo_isa" is as slow as a super call, just fyi.

I think the recursive behaviour is a bit hidden here which is a bit of a concern, but if you don't think that's a problem, I'm fine with it.

In D3909#64601, @tasn wrote:

I like it much better than loop get, that's for sure so it can already go in, but I have a few concerns still.

Is "class" a good "key" for this, or should it be key based? (more complicated and slower...). It should probably be good enough.
Also, "eo_isa" is as slow as a super call, just fyi.

I think the recursive behaviour is a bit hidden here which is a bit of a concern, but if you don't think that's a problem, I'm fine with it.

I dont know if it is a problem, maybe just run for this solution and see in the codebase if there are problems with it ?

We can of course add eo_isa, but this excludes the objects which doesnt want to get discovered.

tasn added a comment.Apr 28 2016, 7:02 AM

"class" is actually a good enough key I guess, because you want the returned type to be of a certain class and for this purpose it shouldn't pose a problem to only have one of each class.

Just to clarify, my concern was if for example we have for multiseat cases two (or more) focus objects, one for user 1 and one for user 2. For this case, asking for a focus manager wouldn't work because the object will not know which one you meant. I honestly don't know if this is a problem or if that would be implemented differently.

I dont understand this session/seat system. Can someone point me to some reading stuff ?

But in general the only one who can dcied which session is currently running is the one which returns the focus manager. So this place has to decide which manager to return. I think nothing which should be managed here in eo.

After thinking more about this, I think provider_find is the best name. We are actually searching for an object that one of the parent may I have heard off. It doesn't need to be in the same hierarchy at all (arguably it should be referenced somewhere, but that's something else). I think searching with a class is just fine, but searching always with eo_isa would be bad. It would force the entire hierarchy to be discoverable, while we really want to offer here a specific set of defined over time class of object.

Now, thinking about adding a string as suggested by @tasn, I think it is best to stick to a class, but maybe an additional key (in the form of a string) would enable more use case. As discussed on IRC, we could maybe use this to lookup for a focus manager for a specific seat. In which case we would do : eo_provider_find(obj, EFL_UI_FOCUS_MANAGER, "seat0"). I am not sure at all this is a good idea, but it sounds to me neat and align with our part/key API. Arguably this open question, should we use that API instead on edje for efl_content_get ? This would enforce the type of the object returned. Again, I am not sure this is a good idea, just sending @tasn idea further up and see if it flyes.

What do you guys think about this ?

Of course an Efl.Loop_Provider would still provide an helper function, efl_loop_provider_get(obj) which doesn't take any parameter. The Efl.Ui.Focus_Manager_Provider could do the same with an efl_ui_focus_manager_provider_get(obj, "seat0") and so on. Don't know either if that's a pattern we want to follow ?

You may be interested in this discussion.

i really dislike loop_provider_get(). it's needlessly verbose. loop_get() is sufficient. you want the loop object that this object belongs to. loops PROVIDE the functionality of driving async i/o and timing events. a loop_provider sounds more like some object that would then provide (generate) loops. just needlessly verbose.

i agree that the definition of provider_find shouldn't explicitly include finding some toplevel tree member or or in fact any member of the tree. it's job is to find an important related object to this one that is key to its functionality. how it's found should be undefined in the general case because it just puts too many limits on features and usability of the api.

as for adding key - i don't think that's useful. i do not see you'd have a focus manager per seat. you would have one. objects are known to the focus manager. this manager would manage 2 3 or more focuses - one per seat. but you wouldn't have more than 1. this means every object has to find n focus managers and register. if you plug/unplug seats on the fly this is just silly overhead as objects have to plug/unplug all the time. so i don't see the example of seat here as useful.

i still dislike provider as a name. master. owner. driver. controller. ...

I would like to stay with provider. It searches for a object which is able to provide the given type. So this is smth. like a provider in my eyes.

I dont like owner, since owner sounds related to references, and this does not have anything to do with references. Master sounds like the owner of the returned object which is definitly misleading. And driver/controller sounds like this is going to work with the object and controll its operations which is also not right.

I still like provider_find the most...

raster added a comment.May 1 2016, 3:16 PM

but it is an owner... or master... controller. you look for an object of type/class X (the input param). in the case of a loop - it is the loop that owns that object (owns/controls/drives the loop based operations of the object). in the case of a focus manager - it's the object that owns/controls focus management of the object. it's looking for an object that owns some aspect of control of that object.

jpeg added a comment.May 1 2016, 10:28 PM

Can't we just get this patch in and argue about the name later? :)

This method could be useful to find the logical parent of an elm widget. Pinging @tasn about this (who wants to remove visual parent):

my_container = efl_provider_find(my_obj, EFL_CONTAINER_CLASS); // option 1
// or maybe
my_container = efl_provider_find(eo_parent_get(my_obj), EFL_WIDGET_CLASS); // option 2

Despite eo_isa() being expensive, I still wonder if it shouldn't be part of the Eo.Base implementation, since that implementation should only be called when the providers in the parent chain could not provide the requested class.

src/tests/eo/suite/eo_test_general.c
1152

fail_if(objtmp != s)

bu5hm4n updated this revision to Diff 9003.May 2 2016, 12:57 AM
bu5hm4n edited edge metadata.

update test / doc

@jpeg if you are happy wit hit, feel free to merge it :)

@raster but this api is not about getting the owner of something, just because you can provide it does not mean it is the owner if it. The same applies for master / controler.

Last call, i think i will merge it in about 6-7 hours.

jpeg accepted this revision.May 3 2016, 7:00 PM
jpeg added a reviewer: jpeg.

just for the record, i think this patch is ok (unless i missed a detail in the implementation)

This revision was automatically updated to reflect the committed changes.