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

[Martin G. <ma...@st...>]

On Wed, 2009-04-01 at 18:28 +1100, Andrew Cowie wrote:

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

Yes, I could have done that, so I don't really need this.  I was just
porting some existing c# code and that's how it was working.  Perhaps I
will change it.

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

<snip>

> 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?

You are right, it is a modal window.  It is used as part of a calendar
selection widget that is made up of an entry, with a popup calendar
below it. There is a ToggleButton to open the calendar window, and while
the calendar window is displayed, the containing VBox is grabAdd()ed.
So the only options for the user are to select a date or toggle to close
the window. I would agree it is not ideal.

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

Yes, this works.  I think I just always assumed it worked.  I have used
it a number of times from the gtk# in c# code.  I think it is the only
way to remove the image from a button.

> 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]

Okay, I will look at creating a test case when I have a minute.
(possibly on Friday)

> > 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!

No problem.  Thanks for your time in reviewing it.

-- 
Martin.