GNOME Bugzilla – Bug 647806
gtk_combo_box_set_active_id() nit picks
Last modified: 2011-04-26 12:02:15 UTC
The new "active-id" feature in GtkComboBox is really handy, but I have a few nit picks about how gtk_combo_box_set_active_id() is written currently. 1. Passing a NULL ID string should be equivalent to gtk_combo_box_set_active (combo_box, -1). Currently, passing a NULL ID string isn't even guarded against and will cause a crash. See (2). 2. I have a use case for Evolution where one of the rows has a NULL value in the ID column. Please use g_strcmp0() instead of strcmp() when comparing ID strings to avoid a crash. 3. Return a boolean indicating if the ID was found or not, so I can write things like: if (!gtk_combo_box_set_active_id (combo_box, desired_id)) gtk_combo_box_set_active (combo_box, 0); /* fallback */
Created attachment 185978 [details] [review] Proposed patch
Review of attachment 185978 [details] [review]: ::: gtk/gtkcombobox.c @@ +5645,3 @@ * gtk_combo_box_set_active_id: * @combo_box: a #GtkComboBox + * @active_id: the ID of the row to select, or %NULL Needs an allow-none annotation @@ +5653,3 @@ * the given ID then nothing happens. * + * Returns: %TRUE if a row with a matching ID was found Maybe mention the 'debatable' case below in the return value docs ? @@ +5689,2 @@ gtk_tree_model_get (model, &iter, column, &id, -1); + match = g_strcmp0 (id, active_id) == 0; This one seems 'debatable' to me. We could just as well mandate that the id column must not contain NULLs. Changing this strcmp here doesn't really hurt, but once we officially allow NULL as an id, wouldn't you expect gtk_combo_box_set_active_id (..., NULL) to activate the row with that id ? It seems that if we allow this, we should document NULL ids as non-activatable.
(In reply to comment #2) > Review of attachment 185978 [details] [review]: > This one seems 'debatable' to me. We could just as well mandate that the id > column must not contain NULLs. Changing this strcmp here doesn't really hurt, > but once we officially allow NULL as an id, wouldn't you expect > gtk_combo_box_set_active_id (..., NULL) to activate the row with that id ? It > seems that if we allow this, we should document NULL ids as non-activatable. It seems most intuitive to me to say that rows having no ID string simply can't be selected by this function. The function should just ignore such rows. For consistency with the rest of the API, I would expect passing NULL to set_active_id() to behave exactly the same as passing NULL to set_active_iter(), which the patch does. If that sounds reasonable I'll try to make the docs clearer about this.
I'd say it's quite natural to have NULLs in the ID column, e.g. for separator rows.
Created attachment 186017 [details] [review] Revised patch Updated the documentation as suggested, and tried to clarify how rows with NULL IDs strings are handled -- or at least how I think they should be handled.
Ok, I think thats fine. Lets see if Ryan wants to chime in, since this is his baby...
Seems quite sane from the description (haven't looked at the patch). I pretty much agree.
Review of attachment 186017 [details] [review]: Looks pretty fine overall, with some very small nit-picks. I'd actually be happy enough if you merged the patch as-is, but maybe you agree with some of the suggestions. ::: gtk/gtkcombobox.c @@ +5674,3 @@ + { + gtk_combo_box_set_active (combo_box, -1); + return FALSE; /* debatable, but TRUE seems less correct */ In terms of the common convention of returning "did the thing that I just requested work out the way I wanted?" I'd go for TRUE here. Either way, I think it should be documented. Just pick one -- it's not too important. @@ +5692,2 @@ gtk_tree_model_get (model, &iter, column, &id, -1); + match = g_strcmp0 (id, active_id) == 0; maybe it is a bit more clear if this is written instead: if (id == NULL) continue; match = strcmp (...) because it looks like maybe we consider the possibility of active_id == NULL here, which is not the case.
Both suggestions are fine with me. Makes sense to fall back to the "TRUE means success" convention.
Created attachment 186083 [details] [review] Revised patch Hopefully the final patch with Ryan's suggestions.
I'm happy.
Is this okay to commit then?
Review of attachment 186083 [details] [review]: Yes, fine to commit