Page MenuHomePhabricator

efl_pack: split algin and padding property
ClosedPublic

Authored by bu5hm4n on Wed, Apr 24, 7:33 AM.

Details

Summary

the pack interface is a general interface for how we pack things into a
container. the align and padding property has less to nothing to do with
this. Hence this commit splits the two properties into theire own
interface.

fix T7825

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 created this revision.Wed, Apr 24, 7:33 AM
bu5hm4n requested review of this revision.Wed, Apr 24, 7:33 AM
cedric requested changes to this revision.Wed, Apr 24, 9:32 AM
cedric added inline comments.
src/lib/efl/interfaces/efl_pack_child_placement.eo
6 ↗(On Diff #21604)

I am not going to be very constructive here, but I find the function resulting from this naming really ugly. Sadly I have no better suggestion. Let's try summoning @segfaultxavi who usually does better than us!

This revision now requires changes to proceed.Wed, Apr 24, 9:32 AM

Let's keep the conversation here instead of the inline comments.

To begin with, I was expecting this new interface to be in the Efl.Ui namespace, since it deals with graphic stuff. Don't you agree?

Regarding the generated method names, it's going to be hard to beat efl_pack_align_set()...

How about Efl.Ui.Arrangement for the interface?

I would like to remove pack_ from the properties, but we already have some align and some padding properties in other classes, which might clash (and some halign, pad_horiz...).
How about arrangement_align and arrangement_padding? This would generate efl_ui_arrangement_align_set() in C which is not THAT bad and Efl.Ui.Button.ArrangementAlign in C#.

Mhm, one problem with your proposal: It does not express that the align and padding will be applied to the children of a container. But other than that, fine :)

Mhm, one problem with your proposal: It does not express that the align and padding will be applied to the children of a container.

I think that Arrangement sort of implies a number of elements and therefore alignment and padding among them, but we might need a native for such language finesse... which means @zmike.

bu5hm4n added a comment.EditedThu, Apr 25, 5:06 AM

For me this could also imply that those properties do change the way the widget, where you set them on, gets placed. The attribute "child" is missing somehow ...

Arrangement + child_ prefix in the two functions ?

I like that, but how about item instead of child? In the docs we talk about items inside the containers.
So... interface Efl.Ui.Arrangement with methods item_align and item_padding.
This would generate efl_ui_arrangement_item_align_set() and Efl.Ui.Table.ItemAlign.

I would be really carefull with item. things in here are *not* items.

After IRC discussion, seems like content would be a good prefix for the methods.

bu5hm4n updated this revision to Diff 21659.Fri, Apr 26, 1:35 AM

here comes the maaaaan in blaaaaaaaaaaaaaaaaaaaaaaaaaaack

bu5hm4n updated this revision to Diff 21660.Fri, Apr 26, 1:43 AM

dont ... stop me NOOOOOOOOOW

bu5hm4n updated this revision to Diff 21661.Fri, Apr 26, 1:56 AM

you don't have to put on the red light

bu5hm4n updated this revision to Diff 21662.Fri, Apr 26, 2:25 AM

All that you touch, and all that you see

segfaultxavi requested changes to this revision.Fri, Apr 26, 2:57 AM
segfaultxavi added inline comments.
src/lib/elementary/efl_ui_list.eo
79

Either remove the line or rename it correctly :D

This revision now requires changes to proceed.Fri, Apr 26, 2:57 AM
segfaultxavi accepted this revision.Fri, Apr 26, 3:04 AM

Finally!

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Apr 26, 3:06 AM
Closed by commit rEFL7aa9ea071a23: efl_pack: split algin and padding property (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.