RE: 2.4 features - Indenting container

[Murray Cumming Comneon com]

> From: Owen Taylor [mailto:otaylor redhat com] 
> > 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.

OK. I'll try to update it when I get connected.
 
> > 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.)

OK, well I suppose this .c file can go into tests instead then.
 
> 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.

Sure consistency is good. I just chose something to begin with, in the
absence of any other good reason.
 
> * 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 :-)

OK. I think it's nice to justify/explain our decisions.
 
> * 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)

I wasn't sure. I didn't see the point of repeating the same information with
different capitalization, and I thought maybe one was supposed to be more
human readable. I thought that a common prefix ("padding") might help to
organize things in Glade, which would probably just list them
alphabetically.

> * Spelling:
> 
> +  /* Intialize padding with default values: */

OK.
 
>   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.

I generally think it's a good idea to use comments to clearly mark blocks of
code. And I don't think you can have too many comments. Personally I think
GTK+ code, though well-structured, is less readable because there are almost
no comments. But it is a matter of opinion, and it depends what you're used
to, so I'll remove them if you like.
 
> * 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.

Yes.
 
> * 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.

OK. I guessed that Gergory got this from somewhere else authorative.
 
>   (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;;

OK.

> ====
> 
> * 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.)

OK. I wondered why Gregory did this. I figured it was just the done thing.
I'll remove them.
 
>   And also, you don't actually notify the _single_ value
>   you do set.

OK. I'll do that.

 
> * In get_padding, you have:
> 
> +    return alignment->padding_top;
> +    break;
> 
>    Etc. We would typically omit the useless break; in this case.

OK.

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

OK.

Thanks for your time, Owen. This is my first largish GTK+ patch.


Murray Cumming
murrayc usa net
www.murrayc.com