Re: misc. properties

[Owen Taylor <otaylor redhat com>]

Michael Meeks <michael ximian com> writes:

> Hi there,
> 
> 	I attach a patch to add 'enable_empty' and 'value_in_list'
> properties to GtkCombo - which are noticably absent ( currently
> working on libglade2 and don't want to add evil widget specific
> hacks ); may I commit ?

Some comments on the patch below. Other than that looks fine
to commit.
 
> 	Also I'm somewhat concerned that GtkEntry doesn't have a
> 'text' property; but it doesn't seem quite clear to me where to add it
> - if at all. So; is this a feature ? should such a property be
> registered against the GtkEditable interface ? 

We already have an "implicit property" for GtkEditable - the
"editable" property. All GtkEditable implementors are supposed
to implement that as well. "text" could be done the same way,
but for now, I'd just add it as a property on GtkEntry. (The
difference between these two approaches is admittedly small...)

Adding such a property to GtkEntry should be trivial since we
already emit ::changed whenever this changes.

(I actually have some doubts whether a "text" property for GtkEntry
makes a lot of sense ... but since this is the fourth or fifth
time this has come up, probably just best to add it to squash
the issue.)

> should g_param_spec_pool_lookup be traversing up the interface
> hierarchy for registered interfaces - it doesn't seem to do that
> currently, but perhaps I'm missing something.

Properties on interfaces are not a GLib-2.0 feature. I forget
the details but when it was discussed earlier there were some
issues that made it not just quick addition.

> @@ -51,7 +51,9 @@
>    PROP_0,
>    PROP_ENABLE_ARROW_KEYS,
>    PROP_ENABLE_ARROWS_ALWAYS,
> -  PROP_CASE_SENSITIVE
> +  PROP_CASE_SENSITIVE,
> +  PROP_ENABLE_EMPTY,
> +  PROP_VALUE_IN_LIST
>  };
> 
>  static void         gtk_combo_class_init         (GtkComboClass    *klass);
> @@ -148,6 +150,22 @@
>                                                           _("Whether list item matching is case sensitive"),
>                                                           FALSE,
>                                                           G_PARAM_READABLE | G_PARAM_WRITABLE));
> +
> +  g_object_class_install_property (gobject_class,
> +                                   PROP_ENABLE_EMPTY,
> +                                   g_param_spec_boolean ("enable_empty",
> +                                                         _("Allow empty field"),
> +                                                         _("Whether the entry may be empty"),
> +                                                         TRUE,
> +                                                         G_PARAM_READABLE | G_PARAM_WRITABLE));

In almost all cases, the "nick" for properties in GTK+ are the same as
the "name" but with case and spaces. If nothing else, this will help
people who are both using a GUI builder and also accessing properties
programmatically.

It shouldn't be too long, since that will make the lists look ugly.

Here I'd go with something along the lines of:

 allow_empty
 Allow Empty
 Whether an empty value may be entered in this field

> +  g_object_class_install_property (gobject_class,
> +                                   PROP_VALUE_IN_LIST,
> +                                   g_param_spec_boolean ("value_in_list",
> +                                                         _("Restrict to listed"),
> +                                                         _("Whether to allow a value not in the list to be entered"),
> +                                                         FALSE,
> +                                                         G_PARAM_READABLE | G_PARAM_WRITABLE));

There is a tradeoff between picking a good name for a property and 
making the name correspond to the associated (and existing setter)
Though I think value_in_list isn't a great name, it's probably
"good enough" to make it worth sticking with gtk_combo_set_value_in_list

I'd make the nick the same: "Value in List" though.
 
The blurb strikes me as backwards - implies that TRUE == values
not in the list may be entered. I'd say:

 Whether entered values must already be present in the list.

> @@ -876,6 +894,8 @@
> 
>    combo->value_in_list = val;
>    combo->ok_if_empty = ok_if_empty;
> +  g_object_notify (G_OBJECT (combo), "value_in_list");
> +  g_object_notify (G_OBJECT (combo), "enable_empty");
>  }

In general, when doing multiple notifies, you should 

 g_object_freeze_notify()/g_object_thaw_notify() around them.

> @@ -1032,6 +1052,14 @@
>        /* This call does the notification */
>        gtk_combo_set_case_sensitive (combo, g_value_get_boolean (value));
>        break;
> +    case PROP_ENABLE_EMPTY:
> +      combo->ok_if_empty = g_value_get_boolean (value);
> +      g_object_notify (G_OBJECT (combo), "enable_empty");
> +      break;
> +    case PROP_VALUE_IN_LIST:
> +      combo->value_in_list = g_value_get_boolean (value);
> +      g_object_notify (G_OBJECT (combo), "value_in_list");
> +      break;

You don't need these g_object_notify() calls here ... 
the '/* This call does the notification */' comments for the
other properties here are confusing and should be removed.

Regards,
                                        Owen