After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 703809 - Some LayoutManager fixes
Some LayoutManager fixes
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks: 703810 703833
 
 
Reported: 2013-07-08 17:51 UTC by Florian Müllner
Modified: 2013-08-19 23:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
actor: Minor cleanup (1.16 KB, patch)
2013-07-08 17:54 UTC, Florian Müllner
committed Details | Review
box-layout: Fix child offsets (1.74 KB, patch)
2013-07-08 17:54 UTC, Florian Müllner
committed Details | Review
bin-layout: Fix offsets (1.17 KB, patch)
2013-07-08 17:55 UTC, Florian Müllner
committed Details | Review
box-layout: Fix (legacy) expand/fill properties (1.66 KB, patch)
2013-07-08 17:56 UTC, Florian Müllner
committed Details | Review
table-layout: Fix default values for expand/fill child properties (1.20 KB, patch)
2013-07-09 12:29 UTC, Florian Müllner
reviewed Details | Review
table-layout: Fix default values for expand/fill child properties (2.54 KB, patch)
2013-07-12 22:49 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2013-07-08 17:51:40 UTC
See patches.
Comment 1 Florian Müllner 2013-07-08 17:54:19 UTC
Created attachment 248637 [details] [review]
actor: Minor cleanup

Just a drive-by cleanup, but didn't bother filing a separate bug ...
Comment 2 Florian Müllner 2013-07-08 17:54:48 UTC
Created attachment 248638 [details] [review]
box-layout: Fix child offsets
Comment 3 Florian Müllner 2013-07-08 17:55:18 UTC
Created attachment 248639 [details] [review]
bin-layout: Fix offsets
Comment 4 Florian Müllner 2013-07-08 17:56:02 UTC
Created attachment 248641 [details] [review]
box-layout: Fix (legacy) expand/fill properties
Comment 5 Emmanuele Bassi (:ebassi) 2013-07-08 18:06:08 UTC
Review of attachment 248637 [details] [review]:

looks good.
Comment 6 Emmanuele Bassi (:ebassi) 2013-07-08 18:07:44 UTC
Review of attachment 248641 [details] [review]:

this is obviously correct.
Comment 7 Florian Müllner 2013-07-09 12:29:39 UTC
Created attachment 248717 [details] [review]
table-layout: Fix default values for expand/fill child properties

According to their param spec, those properties should default to
FALSE, yet they are initialized to TRUE. Make the initialization
match the documented behavior, otherwise constructs like

  expand = clutter_actor_needs_expand () || meta->x_expand;

cannot be used with the recommended ClutterActor properties if
the row/column should not expand.
Comment 8 Emmanuele Bassi (:ebassi) 2013-07-10 09:59:15 UTC
Review of attachment 248717 [details] [review]:

mmh, this seems to be breaking the interactive table layout test.

usually, when GParamSpec and initialization code do not match we fix the GParamSpec instead, as changing the initialization will break existing code that does not initialize those values. in this case it's a bit of a mess, with the old API and the new one.

since we have ClutterGridLayout I wonder if we shouldn't just deprecate ClutterTableLayout altogether...
Comment 9 Emmanuele Bassi (:ebassi) 2013-07-10 10:09:19 UTC
the allocation passed to the ClutterLayoutManager::allocate() has been adjusted (when clutter_actor_allocate() is called on the actor) to take into account the margin set on the actor. we normalize that allocation to be pure [ width, height] because the actor is going to paint its children with allocation.x and allocation.y already placed into the modelview matrix initial transform, so adding the allocation's origin to the allocation of the children would, effectively, cause the children to have their origin shifted.

just so that I understand attachment 248638 [details] [review] and attachment 248639 [details] [review]: when you say "callers" you're implying that you're effectively overriding ClutterActor::allocate() on the actor and then calling clutter_layout_manager_allocate(), am I correct? so you're taking the allocation passed to the actor, fudging around with it, and then passing it to the layout manager's allocate() implementation.

I think I'll need some examples for this.
Comment 10 Florian Müllner 2013-07-10 14:11:31 UTC
(In reply to comment #8)
> usually, when GParamSpec and initialization code do not match we fix the
> GParamSpec instead, as changing the initialization will break existing code
> that does not initialize those values.

Yeah, feel free to reject the patch - it's just ugly that it will require using the old properties over the new ones, but not a big deal.
Comment 11 Florian Müllner 2013-07-10 14:11:50 UTC
(In reply to comment #9)
> just so that I understand attachment 248638 [details] [review] and attachment 248639 [details] [review]: when you say
> "callers" you're implying that you're effectively overriding
> ClutterActor::allocate() on the actor and then calling
> clutter_layout_manager_allocate(), am I correct?

Exactly, see https://git.gnome.org/browse/gnome-shell/tree/src/st/st-widget.c#n425. We adjust the allocation to take the container's CSS into account. I'm sure we can come up with a different approach to make LayoutManagers work for us (the most elegant probably to translate borders/padding into margins), but at least the documentation for clutter_actor_allocate_align_fill (which may still be used by layout managers provided by clutter) suggests that non-0 offsets are supported - in which case they really should be interpreted consistently.
Comment 12 Florian Müllner 2013-07-12 22:49:00 UTC
Created attachment 249052 [details] [review]
table-layout: Fix default values for expand/fill child properties

Currently the default values according to their param spec don't
match the actually used defaults, so update the former to reflect
the actual behavior.