Page MenuHomePhabricator

efl_ui : add item factory for using efl.ui.item on efl.ui.view.
Needs RevisionPublic

Authored by SanghyeonLee on Jan 15 2019, 2:02 AM.

Details

Summary

depends on D7529

To using Efl.Ui.Item on Efl.Ui.View, we need specific factory class which handles
item caching and provides helper method on the item.

Test Plan

efl_ui_list_example_1, 3 works well with this change.
example_2 is broken regardless of this patch,
we have to solve the problem on example case specifically.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8814
Build 7754: arc lint + arc unit
SanghyeonLee created this revision.Jan 15 2019, 2:02 AM
SanghyeonLee requested review of this revision.Jan 15 2019, 2:02 AM
cedric requested changes to this revision.Jan 15 2019, 10:23 AM
cedric added inline comments.
src/lib/elementary/efl_ui_item_factory.eo
1

Why is it not inheriting from Efl.Ui.Layout_Factory as it seems to be duplicating a lot of code logic with it?

This revision now requires changes to proceed.Jan 15 2019, 10:23 AM

rebasing patch

@cedric that's true but I wonder we still need to maintain layout_factory or not. do we need it?
and also some style set and size calculation will be different, so I don't know we can solve this with calling efl_super...
layout factory may need to be fixed too then.

and also need to fix few thing in the view... it still using layout terms and class so it need to be changed Efl.Ui.Item as I see.. will be updated soon.

@cedric that's true but I wonder we still need to maintain layout_factory or not. do we need it?

Hum, the funny things is that Efl.Ui.Widget is the one providing Efl.Ui.Widget.style { set; get; } while Efl.Ui.Layout provide Efl.Ui.Model.Connect.connect. So, what is this item things we are introducing here? See below for solving the style_set problem. But basically my question here, can we not just merge all of this with Efl.Ui.Layout_Factory actually?

and also some style set and size calculation will be different, so I don't know we can solve this with calling efl_super...
layout factory may need to be fixed too then.

If we add a @protected function that instantiate the object itself and is called by the create function from Efl.Ui.Caching_Factory. Then you would override that function in Efl.Ui.Item_Factory. This way you would solve the style_set happening at the right time and Efl.Ui.Layout_Factory will still set all the connect after the call to Efl.Ui.Caching_Factory.create succeed. If we drop Efl.Ui.Item_Factory and want to implement style_set into Efl.Ui.Layout_Factory, we could have an if in the new @protected instantiate function that will do the style_set if the style property has been provided.

What do you think about this idea?

Actually I thought that we might need to remove layout factory cause I cannot find any case of using it till now, but still you think it is necessary, I can go with your idea.
but we have to think about maintaining one more class which may not often used.... if you can find any use case, it should be left.

Actually I thought that we might need to remove layout factory cause I cannot find any case of using it till now, but still you think it is necessary, I can go with your idea.
but we have to think about maintaining one more class which may not often used.... if you can find any use case, it should be left.

Every object that inherit from Layout, would go through this Factory. Things like Signal bind should only work on object that have Efl.Layout.Signal interface. Right now, the Item_Factory looks more like an Efl.Ui.Widget_Factory and Efl.Ui.Layout_Factory should maybe inherit from it. If they both have to exist. Also do not forget that Factory is for all View, not just List and Grid, but with any View that need to generate some Widget.

SanghyeonLee added a comment.EditedJan 15 2019, 6:16 PM

hmm... yeah I agreed with that point. so I'll update the patch with making it inherit from layout.

hmm... yeah I agreed with that point. so I'll update the patch with making it inherit from layout.

I have been giving more thinking about this, and I believe the general design decision is that Efl.Ui.Caching_Factory should inherit from an Efl.Ui.Widget_Factory which would do the object creation and set the style if provided. I can do something like that tomorrow very quickly if that's ok for you.

yeah. I think that is make sense.
if we really make every thing to be an view,
the factory could generate the widget and so on.
I agreed.

Please check the patch serie finishing by D7705.

depends on D7529
renew the patch with inherit layout factory.

dependency update

fix meson build error

fix po file includes

fix meson build and warnings

fix unnecessary changes

  • eo: Fix efl_isa for class checking of recursively inherited types

fix the dependency file including bugs

cedric requested changes to this revision.Feb 11 2019, 2:54 PM
cedric added inline comments.
src/lib/elementary/efl_ui_list_view.eo
39 ↗(On Diff #19144)

I think this should be just factory. There is no point into naming it item_factory when it can take any Efl.Ui.Factory.

This revision now requires changes to proceed.Feb 11 2019, 2:54 PM

so do you think we need to restrict factory input as Efl.Ui.Item_Factory only? or just change the property name as factory not item_factory?
I like the first idea, cause every code of list and grid will guess the object created from factory is Efl.Ui.Item not other class,
so if it comes something else, it is really hard to handle exceptions.

if you agreed we restrict the input class type, I'll update it.

cedric added a comment.Apr 9 2019, 8:46 AM

so do you think we need to restrict factory input as Efl.Ui.Item_Factory only? or just change the property name as factory not item_factory?
I like the first idea, cause every code of list and grid will guess the object created from factory is Efl.Ui.Item not other class,
so if it comes something else, it is really hard to handle exceptions.

List and Grid should not assume that the object they get is anything more than an Efl.Ui.Widget in my opinion. I don't see why we should be more restrictive than that. So saying Item_Factory is quite restrictive. You could really give a Widget_Factory and it should just work. So yes, change this property to just factory sounds best. Otherwise, you have to change the type of the factory and enforce it in the input too.

if you agreed we restrict the input class type, I'll update it.

Hum, could you explain why you would like to restrict the input class type? What are the benefit you see?