Page MenuHomePhabricator

elementary: add internal Efl_Ui_Model_Size.
ClosedPublic

Authored by cedric on Jan 16 2019, 6:28 PM.

Details

Summary

This model enable View that require to compute the size of their items
to rely on an interface to provide the properties they need to get the object
size. This is the base class for all the sizing logic of the new List/Grid View.

Depends on D7654

Diff Detail

Repository
rEFL core/efl
Branch
T7382-devs/cedric/homogeneous
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9160
cedric created this revision.Jan 16 2019, 6:28 PM
cedric requested review of this revision.Jan 16 2019, 6:28 PM
segfaultxavi requested changes to this revision.Jan 17 2019, 3:13 AM
segfaultxavi added inline comments.
src/lib/elementary/efl_ui_model_size.c
9

We really don't have a naming convention for this? These are global variables!
Shouldn't they be something like efl_model_property_name_itemw or similar?

src/lib/elementary/efl_ui_model_size.eo
6

*This model provides the properties...

Also, the convention so far has been that symbol names in EO files are highlighted using monospaced font instead of quotes, so: $Item.Width instead of "Item.Width".

Also, there is a bit of confusion between class properties and model properties. Maybe it can be solved by saying Retrieve these properties using @Efl.Model.Property or whatever, I still don't have that part clear :)

This revision now requires changes to proceed.Jan 17 2019, 3:13 AM
cedric updated this revision to Diff 18578.Jan 17 2019, 4:43 PM
cedric edited the summary of this revision. (Show Details)

Rebase and improve documentation.

segfaultxavi added inline comments.Jan 18 2019, 3:38 AM
src/lib/elementary/efl_ui_model_size.c
9

I repeat that these are public global variables (in the sense that they are not static) exported in a header file and should be namespaced to avoid name collisions. Don't you agree?

src/lib/elementary/efl_ui_model_size.eo
6

Much nicer! But you are still using a lot of quotes and even mixing quotes with $ which will render as "Item.Width". Please use only $.

cedric added inline comments.Jan 18 2019, 12:23 PM
src/lib/elementary/efl_ui_model_size.c
9

Yes, but private (global to elementary, not anyone outside of elementary). efl_* namespace is for outside facing one. _* is usually for local private to a file symbol, but I could maybe use that. Not to sure of the right name here. Maybe _efl_model_property_itemw ?

9

Oh no they aren't global. Only EAPI will really be always visible, otherwise when you compile with visibility=hidden this symbol disappear.

cedric updated this revision to Diff 18635.Jan 18 2019, 12:42 PM

Rebase and fix syntax.

segfaultxavi added inline comments.Jan 22 2019, 7:21 AM
src/lib/elementary/efl_ui_model_size.c
9

OK, let's use _efl_model_property_itemw.

src/lib/elementary/efl_ui_model_size.eo
6

Let's settle for $"Item.Width", with the quotes inside the monospaced text. Be careful, there are 6 such strings in this paragraph :)

cedric updated this revision to Diff 18769.Jan 23 2019, 2:39 PM
cedric edited the summary of this revision. (Show Details)

Rebase and fix documentation.

As you have probably seen in the Efl.Model_Composite ticket, monospaced quotes are not possible. Please revert back to simple quotes, without the $. Sorry.

Also, here the properties are named "Item.Width", in Camel. Case, whereas in the Efl.Model_Composite ticket, you used "child.index", in lower.case. Shouldn't we agree on a convention?

Also, I thought you were going to use _efl_model_property_itemw for the global vars :)

cedric updated this revision to Diff 18816.Jan 24 2019, 3:38 PM

Rebase and fix doc.

cedric updated this revision to Diff 18899.Jan 25 2019, 2:57 PM

Rebase and rename.

segfaultxavi resigned from this revision.Jan 28 2019, 5:47 AM

Docs look good to me. Will handle any outstanding issues in a separate commit.

Resigning as reviewer so somebody else can review the rest of the patch.

SanghyeonLee accepted this revision.Jan 29 2019, 1:48 AM

looks good to me.

This revision is now accepted and ready to land.Jan 29 2019, 1:48 AM
This revision was automatically updated to reflect the committed changes.