Re: Patch: add ActionEvent and TextEvent handling to TextComponentpeers

[Thomas Fitzsimmons <fitzsim at redhat dot com>]

On Fri, 2003-07-04 at 17:37, Tom Tromey wrote:
> >>>>> "Tom" == Thomas Fitzsimmons <fitzsim@redhat.com> writes:
> 
> Tom> Please review/comment.
> 
> Looks good.  Just a couple minor points/questions.
> 
> Tom> -  public void setCaretPosition (int pos)
> Tom> -  {
> Tom> -    set ("text_position", pos);
> Tom> -  }
> 
> I haven't looked at the peers in quite a while.  As I recall there are
> old and new styles of setting attributes on a widget.  Do you know
> which is which?  Or why we should prefer one over the other?

Yes I looked into that (see below).

> My recollection is that the string-based one is the "new" API.  I
> don't care so much about that -- I just would like to know where we're
> headed, if possible.
> 

In GTK 2.0, all widgets are derived from the GObject type.  GObjects can
have gettable and settable properties associated with them.  GtkWidgets
make use of properties to expose some of their internal state; GtkEntry
and GtkTextView each have a boolean "editable" property, for example. 
Properties can be 'get' and set directly by calling appropriate GObject
functions.

Most GtkWidgets also provide widget-specific functions to get and set
certain commonly-used properties.

The first method of accessing a widget's internal state is the more
general, because the GObject property functions can be called on any
widget.  The second method is widget-specific, and is probably available
mostly for convenience.

I think it is best to use the more general approach, where possible,
because it can potentially reduce the amount of native code, and make
the native code that remains more "object-oriented."

In the case of the text components, it wasn't really possible though, so
I just used the existing implementations, which use the GTK 2.0
widget-specific get/set functions.

> Tom> +  public native void setCaretPosition (int pos);
> 
> I don't see new native implementations of setEditable or
> setCaretPosition.

Yes, I should have mentioned this; the setEditable and setCaretPosition
already have working native implementations.  It's just that they were
being overridden by non-working Java implementations, for whatever
reason.
  
> 
> Tom> Index: java/awt/event/ActionEvent.java
> Tom> -    s.append(actionCommand).append(",when=").append(when).append("modifiers");
> Tom> +    s.append(actionCommand).append(",when=").append(when).append(",modifiers");
> 
> This bit is definitely ok; I just wanted to point out that something
> like this can go in as an obvious change.  Just post a note and check
> it in separately; no approval required.
> 

OK.

> I took a quick look at this method, though, and I think the
> setLength() call should be removed.  It seems to me that the code is
> intended to overwrite the first `+' with and `=', but instead we end
> up with "modifiers=+foo", due to the setLength.  Am I reading this
> right?
> 

I'll look into this when I test KeyEvents.  This may have been
deliberate, to match the JDK's output...

> Tom> +  if ((vk >= 0x20 && vk <= 0x7e)	/* Most printable keysyms on a standard US keyboard. */
> 
> Just FYI, the usual GNU rules apply.  We should wrap lines at 80
> characters.
> 

Right, sorry.

> Tom> -  jclass gtkcomponentpeer, gtkwindowpeer, gtkscrollbarpeer, gtklistpeer, gtkmenuitempeer;
> Tom> +  jclass gtkcomponentpeer, gtkwindowpeer, gtkscrollbarpeer, gtklistpeer, gtkmenuitempeer, gtktextcomponentpeer;
> 
> Likewise here, though I realize this one isn't your fault.
> 
> Pure formatting cleanups can also go in as separate patches without
> approval under the obvious rule.
> 

OK.

> 
> This will have to go in to Classpath when it goes into libgcj.
> 

OK, thanks for the comments.

Tom

> Tom
-- 
Thomas Fitzsimmons <fitzsim@redhat.com>