Re: [java-gnome-hackers] [MERGE] added grabAdd and grabRemove methods.

[Andrew C. <an...@op...>]

On Mon, 2009-03-30 at 14:29 +0100, Martin Garton wrote:

> - A javadoc improvement for ComboBox

Great!

You missed surrounding the literal with <code> ... </code> like was done
in the other literal in that method. I've fixed it in a bundle,
attached.

It would be appreciated if you were to run the code formatter before
committing. I've done that too.

Also, it's 2009 now.


> - A change to expose removeColumn() and getColumns() in TreeView.java

Sure.

I'm a bit surprised to hear that you needed getColumns(). I mean, it's
fine, of course, but I'm curious to know what you needed it for when
just keeping a reference to the TreeViewColumns when you create them
does the trick.

{shrug}


> - A change to expose grabAdd() and grabRemove() in Widget.java

As for these two calls, interesting that you modelled them as methods on
Widget. Seems quite sensible.

I'd like to encourage you to add a bit more in the way of discussion
around what Widget's grabAdd() is actually *for*.

We go to a lot of trouble in GTK land to warn people about the dangers
of making Windows modal (which people inevitably want to do for all the
wrong reasons), and this, if anything, is even more invasive. Can you
add a line or two about what circumstances someone would want to use
this in?

Also, a mention as to how these methods are not (or are) related to
Widget's grabDefault() would be really helpful! There doesn't seem to be
any compelling reason to change their signatures, but the fact that they
are grab<COMPLETE> implies they are related.


> - A change to allow gtk_button_set_image to be called with null.

uh, ok. Great!

Does that actually work? The code in gtk/gtkbutton.c in GTK seems to do
something about marking it as a stock image in this case, but the
documentation doesn't explicitly allow NULL.

I assume you tested this with something (else you wouldn't be exposing
it, of course) but a test fixture in one of the TestCases to this effect
would be really helpful [for me and for testing], especially when you're
doing something that isn't explicitly supported by the underlying GNOME
library in question.

[which we do often enough, but test to make sure the assumptions we're
making hold both now - and more importantly, in the future]

> Comments?

These points are all minor. It's mergeable as it stands now, but if you
could touch up these few things I think it'd make a better patch. Thanks
for your contribution!

AfC
Sydney