[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Minor(ish) changes to PsppireValueEntry
From: |
Ben Pfaff |
Subject: |
Re: Minor(ish) changes to PsppireValueEntry |
Date: |
Sun, 22 Apr 2012 16:37:11 -0700 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) |
John Darrington <address@hidden> writes:
> The attached patch makes some changes to PsppireValueEntry - a few bugs,
> a few cleanups where I thought the code was hard to follow, and a few
> improvements. Hopefully all self explanitary.
>
> It also uses this widget in the state variable entry of the ROC dialog.
In general I think that these are good changes. Thank you.
I think that it would be better to break the PsppireValueEntry
changes into a separate commit.
I have a few detailed comments, inlined below:
> - if (val_labs_count (obj->val_labs) > 0)
> + if (obj->val_labs && val_labs_count (obj->val_labs) > 0)
The extra clause above, though harmless, is not necessary because
val_labs_count() returns 0 if passed NULL.
> @@ -269,10 +269,22 @@ psppire_value_entry_refresh_model (PsppireValueEntry
> *obj)
> }
> else
> model = NULL;
> +
> + old_model = gtk_combo_box_get_model (GTK_COMBO_BOX (obj));
> +
> + if (old_model != model)
> + {
> + GtkEntry *entry = GTK_ENTRY (gtk_bin_get_child (GTK_BIN (obj)));
> + gtk_entry_set_text (entry, "");
> + }
> +
> + if (old_model)
> + g_object_unref (old_model);
gtk_combo_box_get_model() doesn't ref the returned pointer, so
I'm pretty sure this "unref" call is wrong.
> gtk_combo_box_set_model (GTK_COMBO_BOX (obj), model);
> gtk_combo_box_entry_set_text_column (GTK_COMBO_BOX_ENTRY (obj), COL_LABEL);
> - gtk_widget_set_sensitive (entry, model != NULL);
> + gtk_combo_box_set_button_sensitivity (GTK_COMBO_BOX (obj), model != NULL
> + ? GTK_SENSITIVITY_ON :
> GTK_SENSITIVITY_OFF);
I didn't know about gtk_combo_box_set_button_sensitivity().
However, the description of the default "auto" setting seems like
it does the right thing, so I wonder whether we can just remove
the gtk_widget_set_sensitive() call entirely?
The reason for the psppire_value_entry_set_value_labels() change
isn't obvious to me. Can you explain?
Thanks,
Ben.
- Minor(ish) changes to PsppireValueEntry, John Darrington, 2012/04/22
- Re: Minor(ish) changes to PsppireValueEntry,
Ben Pfaff <=
- Re: Minor(ish) changes to PsppireValueEntry, John Darrington, 2012/04/23
- Re: Minor(ish) changes to PsppireValueEntry, Ben Pfaff, 2012/04/24
- Re: Minor(ish) changes to PsppireValueEntry, Ben Pfaff, 2012/04/24
- Re: Minor(ish) changes to PsppireValueEntry, John Darrington, 2012/04/24
- Re: Minor(ish) changes to PsppireValueEntry, Ben Pfaff, 2012/04/24
- Re: Minor(ish) changes to PsppireValueEntry, Ben Pfaff, 2012/04/24
- Re: Minor(ish) changes to PsppireValueEntry, Ben Pfaff, 2012/04/24