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 628828 - Add padding and alignment properties to GtkWidget
Add padding and alignment properties to GtkWidget
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on: 628829
Blocks:
 
 
Reported: 2010-09-05 17:03 UTC by Havoc Pennington
Modified: 2010-09-19 05:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add _gtk_widget_get_aux_info_or_defaults() (2.25 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
Use _gtk_widget_get_aux_info_or_defaults() when possible in gtkwidget.c (2.08 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
GtkWidget: add adjust_size_request adjust_size_allocation virtual funcs (13.22 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
Add testadjustsize test, to test new adjust size methods and related features (8.21 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
add gtk_container_class_handle_border_width() so subclasses can ignore border_width (5.89 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
GtkButton: let GtkContainer handle border width (5.58 KB, patch)
2010-09-05 17:03 UTC, Havoc Pennington
none Details | Review
Add padding and alignment properties to GtkWidget (22.15 KB, patch)
2010-09-05 17:04 UTC, Havoc Pennington
none Details | Review
Add padding and alignment tests to testadjustsize.c (5.70 KB, patch)
2010-09-05 17:04 UTC, Havoc Pennington
none Details | Review
Make set_size_request override container border width (patch to be squashed) (1.81 KB, patch)
2010-09-06 00:41 UTC, Havoc Pennington
none Details | Review
call gtk_widget_set_redraw_on_allocate() on alignment that we paint to (to be squashed) (957 bytes, patch)
2010-09-06 00:41 UTC, Havoc Pennington
none Details | Review
document that widget padding is outside set_size_request (to be squashed) (2.83 KB, patch)
2010-09-06 00:41 UTC, Havoc Pennington
none Details | Review

Description Havoc Pennington 2010-09-05 17:03:34 UTC
This patch series first adds adjust_size_request, adjust_size_allocation
virtual methods to GtkWidget, then uses those to implement existing
aux info, then to implement GtkContainer::border-width (with opt-in,
GtkButton is ported as an example), and finally to add padding
and alignment to aux info.

I am leaning toward renaming padding-left,right,top,bottom to 
margin-left,right,top,bottom and padding-all-sizes to simply "margin"
But I'll let people discuss names before I do the search-and-replace.
Comment 1 Havoc Pennington 2010-09-05 17:03:35 UTC
Created attachment 169509 [details] [review]
add _gtk_widget_get_aux_info_or_defaults()

This is better than peeking aux info then testing != NULL
in several ways:
- it returns const aux info so if we don't create we can't write
- it ensures that the default we assume if aux_info is NULL is
  the same as the default we set if we've created the aux info
- it avoids typing in != NULL checks
Comment 2 Havoc Pennington 2010-09-05 17:03:40 UTC
Created attachment 169510 [details] [review]
Use _gtk_widget_get_aux_info_or_defaults() when possible in gtkwidget.c

Did not update uses in other files because the plan is to
get rid of those other uses anyhow. So don't want to make
this function available in the header.
Comment 3 Havoc Pennington 2010-09-05 17:03:43 UTC
Created attachment 169511 [details] [review]
GtkWidget: add adjust_size_request adjust_size_allocation virtual funcs

Use these new methods to handle set_size_request (aka aux_info)
inside gtkwidget.c, instead of having external code mess with it.

The virtual functions can be used for other purposes in the
future. For example, GtkContainer::border_width could be
automatically implemented for all container subclasses.
Comment 4 Havoc Pennington 2010-09-05 17:03:46 UTC
Created attachment 169512 [details] [review]
Add testadjustsize test, to test new adjust size methods and related features

This will test size adjust, and interactions with other padding and border
properties.
Comment 5 Havoc Pennington 2010-09-05 17:03:55 UTC
Created attachment 169513 [details] [review]
add gtk_container_class_handle_border_width() so subclasses can ignore border_width

A subclass calls gtk_container_class_handle_border_width()
in its class_init

This marks the subclass as expecting GtkContainer to deal with
border width automatically, which GtkContainer then does.
Comment 6 Havoc Pennington 2010-09-05 17:03:59 UTC
Created attachment 169514 [details] [review]
GtkButton: let GtkContainer handle border width
Comment 7 Havoc Pennington 2010-09-05 17:04:02 UTC
Created attachment 169515 [details] [review]
Add padding and alignment properties to GtkWidget

h-align = START,END,CENTER,FILL
v-align = START,END,CENTER,FILL
padding-left,right,top,bottom,all-sides

These should obsolete all such similar properties on
layout containers, GtkMisc, GtkAlignment, GtkContainer::border-width
Comment 8 Havoc Pennington 2010-09-05 17:04:06 UTC
Created attachment 169516 [details] [review]
Add padding and alignment tests to testadjustsize.c
Comment 9 Havoc Pennington 2010-09-05 17:07:56 UTC
bug 628829 fix is required for this series to work.
Comment 10 Havoc Pennington 2010-09-06 00:41:03 UTC
Created attachment 169537 [details] [review]
Make set_size_request override container border width (patch to be squashed)

When chaining up to adjust_size_request in GtkWidget base class, GtkWidget
has to go last so the values from set_size_request can force the request
back to what was set.

With the previous code, border_width was added on top of set_size_request
values.
Comment 11 Havoc Pennington 2010-09-06 00:41:06 UTC
Created attachment 169538 [details] [review]
call gtk_widget_set_redraw_on_allocate() on alignment that we paint to (to be squashed)

This patch should be squashed. The alignment in the test program didn't
paint properly because it didn't queue redraw on allocate.
Comment 12 Havoc Pennington 2010-09-06 00:41:09 UTC
Created attachment 169539 [details] [review]
document that widget padding is outside set_size_request (to be squashed)

This patch should be squashed

If padding were not outside the set_size_request() it would not work the
same way as container-supplied (child property) padding.

Conceptually set_size_request() forces the value from the subclass
(the original unadjusted request) and then we go on to adjust
the request further by adding the padding.
Comment 13 Matthias Clasen 2010-09-06 02:38:59 UTC
It would be instructive to see how a container (say GtkBox) uses the new align properties instead of its ::fill child property. In this new world, does ::expand stay a child property ?
Comment 14 Havoc Pennington 2010-09-06 03:03:10 UTC
GtkBox doesn't have to do anything, of course; it's just that now its fill and padding properties are redundant. So if you were inventing GtkBox from scratch you'd want to omit fill and padding. Since we already have GtkBox, those props are there - and still work. In fact, I guess you have to set the box's fill property to TRUE to use widget alignment in the box's orientation, because otherwise the child won't get extra space and the child's align won't come into effect. Perpendicular to the box, GtkBox always has fill=true behavior, so the perpendicular align prop on Widget would work without doing anything special.

With this patch, the ideal GtkBox built from scratch would not have a fill child property (but would always do fill=true); would still have a child property for expand; and would not have a child property for padding.

A side note, I defaulted h-align and v-align to FILL, not CENTER, while GtkBox defaults to fill=false which is like CENTER. h-align and v-align really have to default to FILL for backward compat; otherwise setting fill=true on a GtkBox child would not work anymore. Without compat concerns, maybe CENTER would be preferred, since it seems GtkBox switched to fill=false at some point.
But, I think defaulting to CENTER would break a _lot_ of stuff and is not realistic.

expand is unaffected by this series of patches. I saw a note in the wiki about a "spread" flag that Rapicorn has, which is like expand except it propagates up through all containers. I haven't looked, but my imaginary theory on this feature is that it's basically like a get_h_expand()/get_v_expand() virtual method on Widget, where for containers if any child has set expand=true, the container is also expand=true unless it's been explicitly set false. Then layout containers would look at get_h_expand/get_v_expand on their children, rather than having their own child property for it. And at the top level, Window would be resizable if it had this expand flag set. Anyway this sounds like a nice idea, but it is a separate (though perhaps related) problem, not touched in this patch.

With just this patch, containers still need child properties for expand.

Widget::h-align and ::v-align are just about what a child does with extra space (whether the child leaves it in the allocation or takes it out), they don't affect the allocation that comes from the parent container.
Comment 15 Matthias Clasen 2010-09-06 03:39:02 UTC
Ok, that makes sense.

So, I guess with this way of doing things, we will deprecate child properties like

GtkBox::fill
GtkBox::padding
GTK_FILL in GtkTable::x/y-options
GtkTable::x/y-padding
GTK_WRAP_BOX_H/V_FILL in GtkWrapBox::packing
GtkWrapBox::horizontal/vertical-padding

and aim for removing the whole lot in 4.0 (together with GtkMisc and GtkAlignment).

Do you think it makes sense to look at baseline alignment in this context ?
Comment 16 Matthias Clasen 2010-09-06 04:35:55 UTC
Review of attachment 169515 [details] [review]:

::: gtk/gtkwidget.h
@@ +568,3 @@
+   * Just fix GtkBorder itself? The main danger is probably going signed
+   * to unsigned, rather than 32 to 16.
+   */

I think we should just fix GtkBorder.
Just doing that test-wise seems to work fine.
Probably just need to adjust gtk_rc_property_parse_border.
Comment 17 Havoc Pennington 2010-09-06 14:52:50 UTC
btw I'll look at an expand flag on Widget, started playing with that a bit already. will make a new bug once there's anything to show.

I'm not sure how baseline stuff works or would work. I guess if the allocation contained a baseline somehow, then GTK_ALIGN_ON_BASELINE, ABOVE_BASELINE, BELOW_BASELINE, CENTER_ON_BASELINE, could work. GTK_ALIGN_ON_BASELINE would require getting the baseline of the child, so widgets would need get_baseline_for_height or something. If your allocation has a baseline and you know the child's baseline, you could line up the child baseline with allocation baseline. get_baseline_for_height probably also covers the needs of size request, so a "line up everything on the baseline" container would be possible.

It seems like baseline layout is complex enough that it's going to require adding some new interfaces and a new container, so maybe it should just be kept separate from the non-baseline layout. Or maybe it's impossible to do non-holistically and GtkSizeRequest needs get_baseline_for_height and GtkAllocation needs a baseline field. I'm really not sure. (GTK has no existing baseline-layout features, right?)

I do agree that designers enjoy specifying things in terms of baseline and that it would be nice to support.

Surely baseline support can be a separate patch set from this, it may modify how this stuff works, but it would also modify how the previous stuff worked.
Comment 18 Matthias Clasen 2010-09-06 22:24:09 UTC
(In reply to comment #17)

> 
> It seems like baseline layout is complex enough that it's going to require
> adding some new interfaces and a new container, so maybe it should just be kept
> separate from the non-baseline layout. Or maybe it's impossible to do
> non-holistically and GtkSizeRequest needs get_baseline_for_height and
> GtkAllocation needs a baseline field. I'm really not sure. (GTK has no existing
> baseline-layout features, right?)

The original height-for-width SoC had baseline features in its scope, but that didn't really work out, and Mathias Hasselmann focused more on the hfw part.

> Surely baseline support can be a separate patch set from this, it may modify
> how this stuff works, but it would also modify how the previous stuff worked.

Yeah
Comment 19 Benjamin Otte (Company) 2010-09-07 20:37:11 UTC
Could someone explain how the API for drawing the widget is supposed to look?
I assume widgets don't have to care about borders, padding, etc anymore?
Comment 20 Havoc Pennington 2010-09-07 20:40:56 UTC
See the patch to GtkButton https://bugzilla.gnome.org/attachment.cgi?id=169514&action=diff

If you call gtk_container_class_handle_border_width() then your allocation has border pre-removed so you can just ignore its existence. However you have to switch each widget over to this mode, as in that patch. I only did GtkButton so far.

The same thing could be done for GtkMisc and its subclasses, if desired, but I haven't done that yet.

The new padding and alignment properties on GtkWidget are different since there's no back compat issue; they are always pre-handled - you can ignore them when drawing.