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.26
diff -u -r1.26 GtkCheckboxPeer.java
--- gnu/java/awt/peer/gtk/GtkCheckboxPeer.java 30 Jun 2006 14:47:17 -0000 1.26
+++ gnu/java/awt/peer/gtk/GtkCheckboxPeer.java 30 Jun 2006 19:19:55 -0000
@@ -60,11 +60,11 @@
private static WeakHashMap groupMap = new WeakHashMap();
public native void createCheckButton ();
- public native long createRadioButton (long groupPointer);
+ public native void createRadioButton (long groupPointer);
- public native long addToGroup (long groupPointer);
- public native long removeFromGroup ();
- public native long switchToGroup (long groupPointer);
+ public native void addToGroup (long groupPointer);
+ public native void removeFromGroup ();
+ public native void switchToGroup (long groupPointer);
public native void connectSignals ();
@@ -80,7 +80,7 @@
super (c);
}
- public void create ()
+ public synchronized void create ()
{
Checkbox checkbox = (Checkbox) awtComponent;
current_group = checkbox.getCheckboxGroup ();
@@ -100,15 +100,14 @@
{
// We don't know about this group. Create a new native
// group pointer for this group and store it in our map.
- groupMap.put(current_group, new Long (createRadioButton(0)));
+ createRadioButton(0);
}
else
{
// We already know about this group. Pass the
// corresponding native group pointer value to the native
// create method.
- groupMap.put(current_group,
- new Long(createRadioButton(groupPointer.longValue())));
+ createRadioButton(groupPointer.longValue());
}
}
currentState = checkbox.getState();
@@ -125,7 +124,7 @@
* event since events should only be posted for user initiated
* clicks on the GtkCheckButton.
*/
- synchronized public void setState (boolean state)
+ public synchronized void setState (boolean state)
{
if (currentState != state)
{
@@ -134,12 +133,12 @@
}
}
- public void setLabel (String label)
+ public synchronized void setLabel (String label)
{
gtkButtonSetLabel (label);
}
- public void setCheckboxGroup (CheckboxGroup group)
+ public synchronized void setCheckboxGroup (CheckboxGroup group)
{
if (current_group == null && group != null)
{
@@ -156,15 +155,14 @@
{
// We don't know about this group. Create a new native
// group pointer for this group and store it in our map.
- groupMap.put(current_group, new Long (addToGroup(0)));
+ addToGroup(0);
}
else
{
// We already know about this group. Pass the
// corresponding native group pointer value to the native
// create method.
- groupMap.put(current_group,
- new Long(addToGroup(groupPointer.longValue())));
+ addToGroup(groupPointer.longValue());
}
}
else if (current_group != null && group == null)
@@ -173,8 +171,8 @@
// removing it from a group. This means that the backing
// GtkWidget will change from a GtkRadioButton to a
// GtkCheckButton.
- groupMap.put(current_group, new Long (removeFromGroup()));
- current_group = group;
+ removeFromGroup();
+ current_group = null;
}
else if (current_group == null && group == null)
{
@@ -189,30 +187,30 @@
// remove the backing GtkRadioButton from one group and add it
// to the other group.
+ current_group = group;
+
// See if the new group is already stored in our map.
Long groupPointer = (Long) groupMap.get(group);
if (groupPointer == null)
{
// We don't know about this group. Create a new native
// group pointer for this group and store it in our map.
- groupMap.put(group, new Long (switchToGroup(0)));
+ switchToGroup(0);
}
else
{
// We already know about this group. Pass the
// corresponding native group pointer value to the native
// create method.
- groupMap.put(group,
- new Long(switchToGroup(groupPointer.longValue())));
+ switchToGroup(groupPointer.longValue());
}
- current_group = group;
}
}
// Override the superclass postItemEvent so that the peer doesn't
// need information that we have.
// called back by native side: item_toggled_cb
- synchronized public void postItemEvent(Object item, boolean state)
+ public synchronized void postItemEvent(Object item, boolean state)
{
// Only fire event is state actually changed.
if (currentState != state)
@@ -222,8 +220,13 @@
state ? ItemEvent.SELECTED : ItemEvent.DESELECTED);
}
}
+
+ public synchronized void addToGroupMap(long groupPointer)
+ {
+ groupMap.put(current_group, new Long (groupPointer));
+ }
- public void dispose ()
+ public synchronized void dispose ()
{
super.dispose ();
}
Index: include/gnu_java_awt_peer_gtk_GtkCheckboxPeer.h
===================================================================
RCS file: /cvsroot/classpath/classpath/include/gnu_java_awt_peer_gtk_GtkCheckboxPeer.h,v
retrieving revision 1.11
diff -u -r1.11 gnu_java_awt_peer_gtk_GtkCheckboxPeer.h
--- include/gnu_java_awt_peer_gtk_GtkCheckboxPeer.h 30 Jun 2006 14:47:17 -0000 1.11
+++ include/gnu_java_awt_peer_gtk_GtkCheckboxPeer.h 30 Jun 2006 19:19:56 -0000
@@ -11,10 +11,10 @@
#endif
JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createCheckButton (JNIEnv *env, jobject);
-JNIEXPORT jlong JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createRadioButton (JNIEnv *env, jobject, jlong);
-JNIEXPORT jlong JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_addToGroup (JNIEnv *env, jobject, jlong);
-JNIEXPORT jlong JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_removeFromGroup (JNIEnv *env, jobject);
-JNIEXPORT jlong JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_switchToGroup (JNIEnv *env, jobject, jlong);
+JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createRadioButton (JNIEnv *env, jobject, jlong);
+JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_addToGroup (JNIEnv *env, jobject, jlong);
+JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_removeFromGroup (JNIEnv *env, jobject);
+JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_switchToGroup (JNIEnv *env, jobject, jlong);
JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_connectSignals (JNIEnv *env, jobject);
JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_gtkWidgetModifyFont (JNIEnv *env, jobject, jstring, jint, jint);
JNIEXPORT void JNICALL Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_gtkButtonSetLabel (JNIEnv *env, jobject, jstring);
Index: native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkCheckboxPeer.c
===================================================================
RCS file: /cvsroot/classpath/classpath/native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkCheckboxPeer.c,v
retrieving revision 1.26
diff -u -r1.26 gnu_java_awt_peer_gtk_GtkCheckboxPeer.c
--- native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkCheckboxPeer.c 30 Jun 2006 14:47:17 -0000 1.26
+++ native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkCheckboxPeer.c 30 Jun 2006 19:19:57 -0000
@@ -43,6 +43,7 @@
#include "jcl.h"
static jmethodID postItemEventID;
+static jmethodID addToGroupMapID;
static GtkWidget *checkbox_get_widget (GtkWidget *widget);
static void item_toggled_cb (GtkToggleButton *item, jobject peer);
@@ -57,6 +58,10 @@
postItemEventID = (*cp_gtk_gdk_env())->GetMethodID (cp_gtk_gdk_env(), gtkcheckboxpeer,
"postItemEvent",
"(Ljava/lang/Object;Z)V");
+
+ addToGroupMapID = (*cp_gtk_gdk_env())->GetMethodID (cp_gtk_gdk_env(), gtkcheckboxpeer,
+ "addToGroupMap",
+ "(J)V");
}
JNIEXPORT void JNICALL
@@ -195,7 +200,7 @@
This function is called when initially creating the
button, so an eventbox is created.
*/
-JNIEXPORT jlong JNICALL
+JNIEXPORT void JNICALL
Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createRadioButton
(JNIEnv *env, jobject obj, jlong groupPointer)
{
@@ -211,8 +216,7 @@
if (groupPointer != 0)
{
native_group = JLONG_TO_PTR (GSList, groupPointer);
- if(! GTK_IS_RADIO_BUTTON (native_group->data))
- native_group = NULL;
+ g_assert (GTK_IS_RADIO_BUTTON (native_group->data));
}
button = gtk_radio_button_new_with_label (native_group, "");
@@ -229,9 +233,11 @@
NSA_SET_PTR (env, obj, eventbox);
- gdk_threads_leave ();
+ (*cp_gtk_gdk_env())->CallVoidMethod (cp_gtk_gdk_env(), obj,
+ addToGroupMapID,
+ PTR_TO_JLONG (native_group));
- return PTR_TO_JLONG (native_group);
+ gdk_threads_leave ();
}
/* Add the object to the group pointed to by groupPointer.
@@ -239,7 +245,7 @@
a radio button. Otherwise, creating a radio button in an
existing group.
*/
-JNIEXPORT jlong JNICALL
+JNIEXPORT void JNICALL
Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_addToGroup
(JNIEnv *env, jobject obj, jlong groupPointer)
{
@@ -264,8 +270,7 @@
if (groupPointer != 0)
{
native_group = JLONG_TO_PTR (GSList, groupPointer);
- if(! GTK_IS_RADIO_BUTTON (native_group->data))
- native_group = NULL;
+ g_assert (GTK_IS_RADIO_BUTTON (native_group->data));
}
radio_button = gtk_radio_button_new_with_label (native_group, label);
@@ -281,22 +286,20 @@
}
gtk_container_remove (GTK_CONTAINER (container), check_button);
-
- NSA_DEL_PTR (env, obj);
- NSA_SET_PTR (env, obj, container);
-
gtk_container_add (GTK_CONTAINER (container), radio_button);
gtk_widget_show (radio_button);
- gdk_threads_leave ();
+ (*cp_gtk_gdk_env())->CallVoidMethod (cp_gtk_gdk_env(), obj,
+ addToGroupMapID,
+ PTR_TO_JLONG (native_group));
- return PTR_TO_JLONG (native_group);
+ gdk_threads_leave ();
}
/* Remove the object from the group pointed to by groupPointer.
We are removing the radio button and creating a check button.
*/
-JNIEXPORT jlong JNICALL
+JNIEXPORT void JNICALL
Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_removeFromGroup
(JNIEnv *env, jobject obj)
{
@@ -324,25 +327,27 @@
native_group = gtk_radio_button_get_group (GTK_RADIO_BUTTON (radio_button));
native_group = g_slist_remove (native_group, GTK_RADIO_BUTTON (radio_button));
- GTK_RADIO_BUTTON(radio_button)->group = NULL;
- gtk_container_remove (GTK_CONTAINER (container), radio_button);
-
- NSA_DEL_PTR (env, obj);
- NSA_SET_PTR (env, obj, container);
+ if (native_group && ! GTK_IS_RADIO_BUTTON (native_group->data))
+ native_group = NULL;
+ GTK_RADIO_BUTTON(radio_button)->group = NULL;
+
+ gtk_container_remove (GTK_CONTAINER (container), radio_button);
gtk_container_add (GTK_CONTAINER (container), check_button);
gtk_widget_show (check_button);
- gdk_threads_leave ();
+ (*cp_gtk_gdk_env())->CallVoidMethod (cp_gtk_gdk_env(), obj,
+ addToGroupMapID,
+ PTR_TO_JLONG (native_group));
- return PTR_TO_JLONG (native_group);
+ gdk_threads_leave ();
}
/* Move the radio button to a new group. If groupPointer is
0, create a new group.
*/
-JNIEXPORT jlong JNICALL
+JNIEXPORT void JNICALL
Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_switchToGroup
(JNIEnv *env, jobject obj, jlong groupPointer)
{
@@ -362,8 +367,7 @@
if (groupPointer != 0)
{
native_group = JLONG_TO_PTR (GSList, groupPointer);
- if(! GTK_IS_RADIO_BUTTON (native_group->data))
- native_group = NULL;
+ g_assert (GTK_IS_RADIO_BUTTON (native_group->data));
}
gtk_radio_button_set_group (GTK_RADIO_BUTTON (radio_button), native_group);
@@ -374,9 +378,11 @@
GTK_RADIO_BUTTON(radio_button)->group = native_group;
}
- gdk_threads_leave ();
+ (*cp_gtk_gdk_env())->CallVoidMethod (cp_gtk_gdk_env(), obj,
+ addToGroupMapID,
+ PTR_TO_JLONG (native_group));
- return PTR_TO_JLONG (native_group);
+ gdk_threads_leave ();
}
static void
