Page MenuHomePhabricator

efl_ui: add scrollable_content mixin
ClosedPublic

Authored by zmike on Aug 21 2019, 7:42 AM.

Details

Summary

this allows content to be set with a scroller that automatically handles
its own sizing calcs so that widgets/apps don't have to

@feature

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.
zmike created this revision.Aug 21 2019, 7:42 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

zmike requested review of this revision.Aug 21 2019, 7:42 AM
bu5hm4n requested changes to this revision.Aug 21 2019, 10:56 AM
bu5hm4n added a subscriber: bu5hm4n.

Looks great otherwise :)

src/lib/elementary/efl_ui_widget_scrollable_content.c
153

Please make efl_key_data_get a constructor-property on this mixin.

This revision now requires changes to proceed.Aug 21 2019, 10:56 AM

So you want to have efl.ui.widget.scrollable_content with a scroller_style property?

I actually think that this is an ok usage of key/data API. The call has to stay private. So implementing it as a constructor property require the addition of a private .h function and all the custom thingy to be done manually. A lot more code for something that two line of code solve here. As I am against writing unnecessary line as they bring more problem in the future usually, let's just go with a simple solution aka key/data.

I actually think that this is an ok usage of key/data API. The call has to stay private. So implementing it as a constructor property require the addition of a private .h function and all the custom thingy to be done manually. A lot more code for something that two line of code solve here. As I am against writing unnecessary line as they bring more problem in the future usually, let's just go with a simple solution aka key/data.

No - just make it a constructor property that can only be set once. This is set during construction of the popup classes. So there is no need to make it private. Plus additionally, this works with nice API even from binded languages.

zmike added a comment.Aug 22 2019, 5:20 AM

My issue with a property for this is that then it seems like this opens up the possibility of having this sort of property in more places. I don't think we want to end up with properties to set styles for internal objects anywhere, as they should generally be using the same style as the widget. This is a special case where the internal widget should not be using the same style.

I can understand your concern, but for now we only have it in 1 place, if there comes more we can overthink it, we can keep it beta for now.
My biggest concern is: Any user could just set this key before _scroller_setup gets called, and the will then be applied, this looks to me just too easy to abuse. And for us, no protection of hiding it...
On another thought, is there another usecase where we want to have a different theme ? We could just generate the style and take the widget name + no_inset_shadow, it would serve the case for now, and we can still extend it later on...

zmike added a comment.Aug 22 2019, 6:07 AM

My biggest concern is: Any user could just set this key before _scroller_setup gets called, and the will then be applied, this looks to me just too easy to abuse. And for us, no protection of hiding it...

...so what if they do? The worst that happens is that they end up using a different scroller style.

In D9674#180038, @zmike wrote:

...so what if they do? The worst that happens is that they end up using a different scroller style.

In a undocumented and not intended way, one day we change the implementation in that regard, and destroy this behaviour. There are so many things about key_data that are hurting in that regard:

  • We do not have the key in a exclusive manner, someone else could just mess with it
  • We do not have proper ownership handling of the attached string
  • It does not work on bindings

[...]

zmike updated this revision to Diff 24405.Thu, Aug 22, 3:57 PM

remove configurability of styles

zmike added a comment.Thu, Aug 22, 3:59 PM

I've removed this functionality entirely since there is currently no demand for it and we aren't able to come to an agreement.

This revision is now accepted and ready to land.Thu, Aug 22, 10:23 PM
Closed by commit rEFL1a517b4c2db7: efl_ui: add scrollable_content mixin (authored by zmike, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyThu, Aug 22, 10:35 PM
This revision was automatically updated to reflect the committed changes.

I know I am late to the party, but... could you fix the docs (in another patch)?

(These refs didn't build and you stealthily fixed them in D9676... you thought doc cop wouldn't notice?)

src/lib/elementary/efl_ui_widget_scrollable_content.eo
9

Is this still true after the discussion?

25

Correct event ref is @[Efl.Ui.Widget.Scrollable_Content.optimal_size,calc].

27

Unfortunately we do not have a mechanism in Eolian to mark whole blocks as monospaced. I prefer you remove the backticks and just leave it as plain text.