[cp-patches] Patch: Checkbox group fix

Thomas Fitzsimmons fitzsim at redhat.com
Fri Jun 30 17:03:06 UTC 2006


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.

This looks good, but I have a few comments:

> +/* A radio button is created if we are part of a group. 
> +   groupPointer points to the corresponding group. If 0,
> +   a new group is created.
> +   This function is called when initially creating the
> +   button, so an eventbox is created.
> + */
> +JNIEXPORT jlong JNICALL 
> +Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_createRadioButton 
> +  (JNIEnv *env, jobject obj, jlong groupPointer)
> +{
> +  GtkWidget *button;
> +  GtkWidget *eventbox;
> +  GSList *native_group = NULL;
> +  
> +  gdk_threads_enter ();
> +
> +  NSA_SET_GLOBAL_REF (env, obj);
> +  eventbox = gtk_event_box_new ();
> +
> +  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.

> +  }
> +  button = gtk_radio_button_new_with_label (native_group, "");
> +  
> +  if (native_group == NULL)
> +    native_group = gtk_radio_button_get_group (GTK_RADIO_BUTTON (button));
> +  if (g_slist_index (native_group, GTK_RADIO_BUTTON (button)) == -1)
> +  {
> +    native_group = g_slist_prepend (native_group, GTK_RADIO_BUTTON (button));
> +    GTK_RADIO_BUTTON(button)->group = native_group;  
> +  }
> +  
> +  gtk_container_add (GTK_CONTAINER (eventbox), button);
> +  gtk_widget_show (button);
> +  
> +  NSA_SET_PTR (env, obj, eventbox);
> +  
> +  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.

> +}
> +
> +/* Add the object to the group pointed to by groupPointer.
> +   If groupPointer is 0, create a new group and create
> +   a radio button. Otherwise, creating a radio button in an
> +   existing group.
> + */
> +JNIEXPORT jlong JNICALL 
> +Java_gnu_java_awt_peer_gtk_GtkCheckboxPeer_addToGroup 
> +  (JNIEnv *env, jobject obj, jlong groupPointer)
> +{
> +  void *ptr;
> +  GtkWidget *container;
> +  GtkWidget *check_button;
> +  GtkWidget *radio_button;
> +  const gchar *label;
> +  GSList *native_group = NULL;
> +
> +  gdk_threads_enter ();
> +
> +  ptr = NSA_GET_PTR (env, obj);
> +  container = GTK_WIDGET (ptr);
> +  check_button = checkbox_get_widget (container);
> +  label = gtk_label_get_text (GTK_LABEL (gtk_bin_get_child 
> +                                        (GTK_BIN (check_button))));
> +                                        
> +  /* Need to remove the check_button, and replace it with 
> +     a radio button in a group.
> +   */
> +  if (groupPointer != 0)
> +    {
> +      native_group = JLONG_TO_PTR (GSList, groupPointer);
> +      if(! GTK_IS_RADIO_BUTTON (native_group->data))
> +        native_group = NULL;
> +    }
> +      
> +  radio_button = gtk_radio_button_new_with_label (native_group, label);
> +  gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (radio_button), 
> +             gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (check_button)));
> +  
> +  if (native_group == NULL)
> +    native_group = gtk_radio_button_get_group (GTK_RADIO_BUTTON (radio_button));
> +  if (g_slist_index (native_group, GTK_RADIO_BUTTON (radio_button)) == -1)
> +  {
> +    native_group = g_slist_prepend (native_group, GTK_RADIO_BUTTON (radio_button));
> +    GTK_RADIO_BUTTON(radio_button)->group = native_group;
> +  }
> +             
> +  gtk_container_remove (GTK_CONTAINER (container), check_button);
> +
> +  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.

Tom




More information about the Classpath-patches mailing list