RE: 2.4 features - Indenting container

[Owen Taylor <otaylor redhat com>]

On Wed, 2003-04-09 at 05:41, Murray Cumming Comneon com wrote:
> > From: Owen Taylor [mailto:otaylor redhat com] 
> > On Thu, 2003-03-13 at 10:35, Murray Cumming Comneon com wrote:
> > > So, can we agree that a version of Gregory's patch that does 4-sided
> > > (independent) padding could go into 2.4?
> > > 
> > http://mail.gnome.org/archives/gtk-devel-list/2002-December/msg00090.html
> 
> > Well, I don't think it would actually share any _code_ with
> > that patch ... nothing wrong with that patch, but once you go with
> > 4-sided padding instead of 1-sided padding just about everything
> > changes. 
> >
> > But yes, I'd be in favor of a 4-sided padding addition to GtkAlignment.
> 
> OK. I have adapted that patch for 4 sides:
> http://bugzilla.gnome.org/show_bug.cgi?id=110365
> 
> It does share some code with Gregory's patch because I don't have much clue
> about this requisition/allocation stuff, so I welcome criticism.
> 
> Also, it should be changed to use the private-data stuff. I believe that
> this is not yet in cvs. Is there a bug number for it so I can make this one
> depend on it?

It's in CVS; all you'd need to do is up GLIB_REQUIRED_VERSION in
gtk+/configure.in.

> Also, this patch adds examples/alignment_padding, which exercises the
> new functionality.

examples/ are stuff auto-extracted from the tutorial, and shouldn't
be touched in general. The two choices are:

 - Write an example with straightforward code that is helpful
   to application writers and put it in gtk-demo.

 - Write a test case that exhaustively tests your additions and
   put it in testgtk. (The prop-editor stuff there, as used by
   the entry example may make this easier.)

Review comments:

* I was a little suprised by the C API

void       gtk_alignment_set_padding (GtkAlignment      *alignment,
			              GtkPositionType    pad_pos,
				      guint              padding);
guint       gtk_alignment_get_padding (GtkAlignment      *alignment,
				       GtkPositionType    pad_pos);

  I would have expected functions that get/set all four parameters 
  at once; similar to the way gtk_misc_set/get_padding
  work. I don't think the above is _bad_; it might even be
  a nicer API. But there is something to be said for consistency.

  I'd be interested to hear other people's thoughts on this.

* Don't editorialize in comments:

+  /* The Padding could almost be in a separate widget, but it's loosely related to alignment. */

  We're doing it this way, so it's the right way to do it :-)

* You have name=padding_top, nick="Top padding" - this is going to 
  confuse people ... I don't think there is any other place in
  GTK+ like this. I'd suggest name="top_padding", nick="Top Padding".
  (Note capitalization as well)

* Spelling:

+  /* Intialize padding with default values: */

  But also, really, what does that comment add?

* I know it's a matter of opinion, but I think some of the other
  comments are of pretty dubious value:

+    /* Padding. Call gtk_alignment_set_padding() with the appropriate GTK_POS_* value: */
+    /* Padding. Call gtk_alignment_get_padding() with the appropriate GTK_POS_* value: */
+  /* Set the appropriate struct field, depending on the padding position: */
+  /* Get the appropriate struct field, depending on the padding position: */

  If the code itself is transparent:

+    case PROP_PADDING_TOP:
+      gtk_alignment_set_padding (alignment, GTK_POS_TOP, g_value_get_uint (value));

  Then adding a comment just means more code to read.

* To answer your question:

===      
+
+      /* TODO: Rename x and y to border_x and border_y? murrayc */
* For the allocation code:
===

  I think it would do more for the clarity of the code to remove
  x/y and replace them with a border_width variable, since 
  x == y == border_width.

  Which would make the comment:

=========
+      /* 2*x because there are 2 borders - 1 on each side: */
+      width = MAX (allocation->width - padding_horizontal - 2 * x, 0);
+      height = MAX (allocation->height - padding_vertical - 2 * y, 0);
=========

  probably unnecessary.

* You have a common bug in your size computation:

+  guint padding_horizontal, padding_vertical;

+      width = MAX (allocation->width - padding_horizontal - 2 * x, 0);
+      height = MAX (allocation->height - padding_vertical - 2 * y, 0);


  allocation->width - padding_horizontal - 2 * x will be unsigned, 
  so the MAX (, 0) does nothing. You already aren't worrying about
  overflow for padding_left + padding_right, so we are saying "pass
  in ridiculous values, get what you deserve", I'd just make 
  padding_horizontal/padding_vertical signed. In fact, I might make
  the padding values signed everywhere ... generally we've switched
  to using signed dimensions in GTK+ to avoid these problems with
  promotion.

  (If you have a signed value, you can also catch a not-unlikely
  programming problem with:

    g_return_if_fail (top_padding < 0)

  rather than computing ridiculous sizes, and having the program
  die probably with a BadAlloc X error somewhere down the line.)

* Note the ;; at the end of:

==== 
-      child_allocation.x = alignment->xalign * (width - child_allocation.width) + allocation->x + x;
-      child_allocation.y = alignment->yalign * (height - child_allocation.height) + allocation->y + y;
+      child_allocation.x = alignment->xalign * (width - child_allocation.width) + allocation->x + x + alignment->padding_left;
+      child_allocation.y = alignment->yalign * (height - child_allocation.height) + allocation->y + y + alignment->padding_top;;
====

* In gtk_alignment_set_padding(), you have a

  g_object_freeze_notify/thaw_notify pair, which is only 
  needed if you are notifying multiple values. (But see
  my above comment about possible API changes.)

  And also, you don't actually notify the _single_ value
  you do set.

* In get_padding, you have:

+    return alignment->padding_top;
+    break;

   Etc. We would typically omit the useless break; in this case.

* There are some extra blank lines introduced in the header.

Regards,
                                               Owen