[cp-patches] FYI: Checkbox group fix

Lillian Angel langel at redhat.com
Fri Jun 30 19:57:06 UTC 2006


fitzsim suggested that not all functions be synchronized, but there be a
synchronized block around the code that adds the new pointer to the
groupMap.

Fixed.


2006-06-30  Lillian Angel  <langel at redhat.com>

        * gnu/java/awt/peer/gtk/GtkCheckboxPeer.java
        (create): Changed to be non-synchronized.
        (setLabel): Likewise.
        (setCheckboxGroup): Likewise.
        (addToGroupMap): Likewise. Added synchronized block around
        code.
        (dispose): Changed to be non-synchronized.


On Fri, 2006-06-30 at 15:31 -0400, Lillian Angel wrote:
> Hi,
> 
> I fixed the issues Tom stated below.
> 
> On Fri, 2006-06-30 at 13:03 -0400, Thomas Fitzsimmons wrote:
> > Hi,
> > 
> > Lillian Angel wrote:
> > > This patch fixes the long standing problem of not being able to
> > > dynamically switch between a checkbox and a radio button. I.e. moving a
> > > checkbox to a checkbox group changes the checkbox to a radio button.
> > > 
> > > Tom helped a lot with this patch. He removed the CheckboxGroupPeer and
> > > we fixed it so everything is handled in GtkCheckboxPeer.
> > > 
> > > There is a mauve test for this. The harmony test
> > > (test.java.awt.CheckboxTest) now passes.
> 
> [...]
> > > +  if (groupPointer != 0)
> > > +  {
> > > +    native_group = JLONG_TO_PTR (GSList, groupPointer);
> > > +    if(! GTK_IS_RADIO_BUTTON (native_group->data))
> > > +      native_group = NULL;
> > 
> > When does it happen that the data field is not a GtkRadioButton?  I suspect only 
> > after removing a Checkbox from a CheckboxGroup?  This is likely a memory leak -- 
> > if the native_group's data field is no longer valid then we should probably free 
> > the GSList.
> 
> Gtk actually frees the list when needed. I changed these checks to
> asserts and set the native_group to null in *_removeFromGroup (when
> needed).
> 
> [...]
> 
> > > +  gdk_threads_leave ();
> > > +  
> > > +  return PTR_TO_JLONG (native_group);
> > 
> > I realized that there is a race condition here.  Instead of returning the 
> > pointer value, we should use JNI to set the groupPointer field with the GDK lock 
> > held.
> 
> I changed all functions in GtkCheckboxPeer to be syncronized, and added
> a new function to put the new group in the groupMap. This function is
> called from the native peer.
> 
> [...]
> > > +  NSA_DEL_PTR (env, obj);
> > > +  NSA_SET_PTR (env, obj, container);
> > 
> > The NSA table points to the containing event box, so I don't think these two 
> > lines are necessary.
> 
> Removed
> > 
> 2006-06-30  Lillian Angel  <langel at redhat.com>
> 
>         * gnu/java/awt/peer/gtk/GtkCheckboxPeer.java:
>         Changed all return values of native functions to void.
>         (create): Changed function to be syncronized. Removed
>         call to put value in groupMap, this is now done from
>         the native code.
>         (setState): Changed function to be syncronized.
>         (setLabel): Changed function to be syncronized.
>         (setCheckboxGroup): Changed function to be syncronized. Removed
>         call to put value in groupMap, this is now done from
>         the native code.
>         (postItemEvent): Changed function to be syncronized.
>         (addToGroupMap): New function. Called by native code to add
>         new value to the group.
>         (dispose): Changed function to be syncronized.
>         * include/gnu_java_awt_peer_gtk_GtkCheckboxPeer.h: Updated
>         all functions.
>         * native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkCheckboxPeer.c
>         (cp_gtk_checkbox_init_jni): Added code to link to
>         java function.
>         (Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createRadioButton):
>         Changed return value to void. Added call
>         to java function to set pointer in groupMap.
>         (Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_addtoGroup): 
> 	Likewise. Also, changed check to an assert. Also, removed call 
> 	to set/del pointer.
>         (Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_removeFromGroup):
>         Likewise. Also, added check to determine if native_group should 
> 	be set to NULL.
>         (Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_switchToGroup): 
> 	Likewise.
> 
> Committed,
> Lillian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1405 bytes
Desc: not available
Url : http://developer.classpath.org/pipermail/classpath-patches/attachments/20060630/a877f951/patch-0001.bin


More information about the Classpath-patches mailing list