Added synchronized blocks around groupMap.get calls.
2006-06-30 Lillian Angel <[EMAIL PROTECTED]>
* gnu/java/awt/peer/gtk/GtkCheckboxPeer.java
(create): Added synchronized block around groupMap.get calls.
(setCheckboxGroup): Likewise.
On Fri, 2006-06-30 at 15:57 -0400, Lillian Angel wrote:
> 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 <[EMAIL PROTECTED]>
>
> * 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 <[EMAIL PROTECTED]>
> >
> > * 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
Index: gnu/java/awt/peer/gtk/GtkCheckboxPeer.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/awt/peer/gtk/GtkCheckboxPeer.java,v
retrieving revision 1.28
diff -u -r1.28 GtkCheckboxPeer.java
--- gnu/java/awt/peer/gtk/GtkCheckboxPeer.java 30 Jun 2006 19:54:18 -0000 1.28
+++ gnu/java/awt/peer/gtk/GtkCheckboxPeer.java 30 Jun 2006 20:01:29 -0000
@@ -95,7 +95,12 @@
// Initially we're part of a group.
// See if this group is already stored in our map.
- Long groupPointer = (Long) groupMap.get(current_group);
+ Long groupPointer = null;
+ synchronized (groupMap)
+ {
+ groupPointer = (Long) groupMap.get(current_group);
+ }
+
if (groupPointer == null)
{
// We don't know about this group. Create a new native
@@ -150,7 +155,12 @@
current_group = group;
// See if the new group is already stored in our map.
- Long groupPointer = (Long) groupMap.get(current_group);
+ Long groupPointer = null;
+ synchronized (groupMap)
+ {
+ groupPointer = (Long) groupMap.get(current_group);
+ }
+
if (groupPointer == null)
{
// We don't know about this group. Create a new native
@@ -190,7 +200,12 @@
current_group = group;
// See if the new group is already stored in our map.
- Long groupPointer = (Long) groupMap.get(group);
+ Long groupPointer = null;
+ synchronized (groupMap)
+ {
+ groupPointer = (Long) groupMap.get(current_group);
+ }
+
if (groupPointer == null)
{
// We don't know about this group. Create a new native
