java-gnome-hackers

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

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

Hi All,

Here is a bundle to make a few very minor changes.

- A javadoc improvement for ComboBox
- A change to expose removeColumn() and getColumns() in TreeView.java
- A change to expose grabAdd() and grabRemove() in Widget.java
- A change to allow gtk_button_set_image to be called with null.

Comments?

-- 
Martin.

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

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.

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

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

On Wed, 2009-04-01 at 21:44 +0100, Martin Garton wrote:

> Yes, I could have done that, so I don't really need this.  I was just
> porting some existing [GTK] code and that's how it was working.


And that's as good a reason as any to expose it. There's nothing
illegitimate about it - but at least you know why it wasn't exposed as
yet.

[Likewise, I personally tend not to be in to much of a rush to expose
property getters; after all, in most cases, you know if you set the damn
thing in the first place. Other people have legitimate uses for them;
they can do the work to expose them. All good]

> > We go to a lot of trouble in GTK land to warn people about the dangers
> > of making Windows modal...

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

(huh. Neat)

Well, as I said, if you can just put a bit of explanation or cautionary
text beside it, that'll help developers in the future who are busy
learning GTK and wonder what the heck they're supposed to use [the only
problem with Widget being the ancestor ... so many methods visible &
inherited, as opposed to decorating as you get richer. {shrug} Is what
it is]

> I think it is the only
> way to remove the image from a button.

Sure.

It makes sense, and that's the remove idiom for lots of other
properties, and the getter can return null. But I have been caught by
surprise MANY times by GTK with regards to this sort of thing, and
normalizing such in our public API has been a lot of [im]patient work.
Rewarding, but work. :)

AfC
Sydney