GNOME Bugzilla – Bug 324899
GtkComboBoxText needs API to remove all items
Last modified: 2010-10-21 12:30:37 UTC
Only way to do it currently is this way. Nautilus does it currently in the following way combo_box = GTK_COMBO_BOX (NAUTILUS_NAVIGATION_WINDOW (window)->view_as_combo_box); /* Clear the contents of ComboBox in a wacky way because there * is no function to clear all items and also no function to obtain * the number of items in a combobox. */ model = gtk_combo_box_get_model (combo_box); g_return_if_fail (GTK_IS_LIST_STORE (model)); store = GTK_LIST_STORE (model); gtk_list_store_clear (store); My proposal would be to add the following functions to GTK gtk_combo_box_clear_text () /* Removes all text elements from a text combo */ and gtk_combo_box_get_num_entries_text () /* Returns number of elements in the combo */
I'm also voting for the addition of a gtk_combo_box_clear_text() API. I'm currently using the following workaround to get the same results, but it's not 'the right way' : gtk_combo_box_set_active(GTK_COMBO_BOX(combo1), 0); while (gtk_combo_box_get_active_text(GTK_COMBO_BOX(combo1))) { gtk_combo_box_remove_text(GTK_COMBO_BOX(combo1), 0); gtk_combo_box_set_active(GTK_COMBO_BOX(combo1), 0); } Is it still feasible to add the gtk_combo_box_clear_text() API to GTK 2.9/2.10 ?
Created attachment 84261 [details] [review] Proposed patch to add both methods to the combobox Hi!!! This is the patch I've implemented to add to the combobox the suggested functions gtk_combo_box_clear_text and gtk_combo_box_get_num_entries_text. - gtk_combo_box_clear_text uses gtk_list_store_clear on the GtkListStore that is the model of the combobox created with gtk_combo_box_new_text to clear it. - gtk_combo_box_get_num_entries_text uses a GtkTreeIter to iterate over the GtkListStore that is the model of the combobox and get the number of elements. Greetings!!
Created attachment 84263 [details] Test to try the patch Hi again!! This is the program I used to test the patch. It allows you to add and remove strings from a combobox and to clear it, showing at every moment the number of elements stored in the comobox. I hope it helps :) Greetings!!!
Tested on revision 17552 Just the changelog failed... Miguel, can you please update the patch? root@makuchaku:~/gtk/trunk# patch --dry-run --verbose -p0 < patches/gtkcombobox.patch Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: gtk/gtkcombobox.c |=================================================================== |--- gtk/gtkcombobox.c (revision 17432) |+++ gtk/gtkcombobox.c (working copy) -------------------------- Patching file gtk/gtkcombobox.c using Plan A... Hunk #1 succeeded at 5027 (offset 85 lines). Hmm... The next patch looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: gtk/gtkcombobox.h |=================================================================== |--- gtk/gtkcombobox.h (revision 17432) |+++ gtk/gtkcombobox.h (working copy) -------------------------- Patching file gtk/gtkcombobox.h using Plan A... Hunk #1 succeeded at 111. Hmm... The next patch looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: ChangeLog |=================================================================== |--- ChangeLog (revision 17432) |+++ ChangeLog (working copy) -------------------------- Patching file ChangeLog using Plan A... Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file ChangeLog.rej done
Created attachment 85118 [details] [review] Initial patch with Changelog updated Here it goes!!!!! ;) Greetings!!
Created attachment 85172 [details] [review] Initial patch without Changelog As I upated gtk+, I noticed that Chagelog has changed, so the patch is out of date again due to those changes. I made this patch without the Changelog file so it works fine in spite of changes in that file. This is the text that should go in the Changelog if the patch is accepted (just change the date). I'm not sure if my name should go there, so you can move it if needed. Greetings!! 2007-03-08 Miguel Gomez <magomez@igalia.com> * gtk/gtkcombobox.c (gtk_combo_box_clear_text), (gtk_combo_box_get_num_entries_text): Added functions to clear the contents of a combo_box constructed with gtk_combo_box_new_text. (#324899) * gtk/gtkcombobox.h: functions added to the API.
Yes, the patch now works on revision 17552. Thanks for your quick response.
In general, don't worry too much about ChangeLog clashes with patches. It will always happen, and people take care of it when applying patches. The patch looks good to me. It's very straightforward. Permission to apply?
This patch adds both gtk_combo_box_clear_text() and gtk_combo_box_get_num_entries_text(). How is gtk_combo_box_get_num_entries_text() useful?
Can one of the maintainers look at the patch? Thanks
Do you want me to upload a patch that only adds gtk_combo_box_clear_text() instead of both fuctions? I'll be glad to do it if you think is more useful
I think that would be more likely to be approved, but it's generally difficult to get any patch approved.
Created attachment 100820 [details] [review] Patch that only adds gtk_combo_box_clear_text () Here we go again! As it's not clear whether gtk_combobox_get_n_entries_text is useful, this version of the patch only contains gtk_combobox_clear_text. My suggest for the changelog: * gtk/gtkcombobox.c (gtk_combo_box_clear_text): Added function to clear the contents of a combo_box constructed with gtk_combo_box_new_text. (#324899) * gtk/gtkcombobox.h: function added to the API. Greetings!!
PING!
Applies cleanly, quick fix. http://live.gnome.org/GtkLove/PatchTriaging spam.
Ping...
The patch is missing a new entry in gtk.symbols. Every new public function needs to be added to that file.
Created attachment 112786 [details] [review] Updated patch which also updates gtk.symbols Can I apply the updated patch. I have an SVN account
> Created an attachment (id=112786) [edit] > Updated patch which also updates gtk.symbols > > Can I apply the updated patch. I have an SVN account > Sorry to jump in now but: gtk_combo_box_append_text +gtk_combo_box_clear_text gtk_combo_box_get_active(In reply to comment #18) Are we sure the name of the function is *clear*? And I mean, maybe we should use a better name, because the API consumer may think two behaviours about it (without looking at the documentation of course): a) Clear all elements on the combo box, living it empty. b) Unselect all the options, so the active text is empty. That's why I would advocate another name, for example gtk_combo_box_clear_texts (plural) gtk_combo_box_clear_items.
Typos in last comment: s/living/leaving s/(plural)/(plural) or/ (Sorry for bugspam.)
But texts is not a real word. The _text word is needed to make it clear that it's (only) for use with the other _text functions, because (unwisely, I think) this is not a derived class.
I second that _clear_text is confusing to some extend. It reads to me as something that operates on the text visible in the combobox. This is not true for the other accessors. In fact _append_text, _get_active_text, _remove_text and _insert_text are all function names that can be read literally and still make sense. I would hope for someone to come up with an alternative name for this.
Can we go with the name "gtk_combo_box_clear_all_texts"? I don't see why we should delay this more just for not finding the perfect name. (In reply to comment #21) > But texts is not a real word. Do you mean that the word "text" in English cannot be used in plural?
Texts is actually a word, but it tends to mean two articles (such as two essays or two books). texts is not a way to say "two items of text".
(In reply to comment #19) > Sorry to jump in now but: > > gtk_combo_box_append_text > +gtk_combo_box_clear_text > gtk_combo_box_get_active(In reply to comment #18) > > Are we sure the name of the function is *clear*? And I mean, maybe we should > use a better name, because the API consumer may think two behaviours about it > (without looking at the documentation of course): Without looking at the documentation functions like gtk_combo_box_append_text and gtk_combo_box_get_active_text are just as confusing as the proposed gtk_combo_box_clear_text, so I don't see a problem with the proposed name. Maybe it would be best if the name gtk_combo_box_clear_text is used for the next GTK 2.x release and for GTK 3.x to rename the names of all the gtk_combo_box_*text* functions to something more sane. Or maybe to mark all the gtk_combo_box_*text* functions deprecated and introducing the more sane function names for the next GTK 2.x release already.
(In reply to comment #25) > (In reply to comment #19) > > Sorry to jump in now but: > > > > gtk_combo_box_append_text > > +gtk_combo_box_clear_text > > gtk_combo_box_get_active(In reply to comment #18) > > > > Are we sure the name of the function is *clear*? And I mean, maybe we should > > use a better name, because the API consumer may think two behaviours about it > > (without looking at the documentation of course): > > Without looking at the documentation functions like gtk_combo_box_append_text > and gtk_combo_box_get_active_text are just as confusing as the proposed > gtk_combo_box_clear_text, so I don't see a problem with the proposed name. I don't agree at all. The existing function names are pretty understandable in my opinion. However the proposed _combo_box_clear_text function is actually ambiguous. It's not obvious if it relates to removing the visible text or all text strings in the listmodel. The _text suffix can so far at the same time be read as a common suffix and literally as part of the function. > Maybe it would be best if the name gtk_combo_box_clear_text is used for the > next GTK 2.x release and for GTK 3.x to rename the names of all the > gtk_combo_box_*text* functions to something more sane. Or maybe to mark all the > gtk_combo_box_*text* functions deprecated and introducing the more sane > function names for the next GTK 2.x release already. If you actually want a better solution, it should rather be a subclass or an interface. That would avoid any ambiguity.
Are those two functions really needed? combo.get_model().clear() and len(combo.get_model()) works fine for me... I know not everyone is using Python, but not every not-that-frequently used five lines of C code can be added as a function to gtk.
(In reply to comment #27) > Are those two functions really needed? combo.get_model().clear() and > len(combo.get_model()) works fine for me... I know not everyone is using > Python, but not every not-that-frequently used five lines of C code can be > added as a function to gtk. They are not strictly needed, but I think they are good to finish off the convenience API. I see only two good options for a good name here: - gtk_combo_box_clear_text - gtk_combo_box_remove_all_text "Clear" because it is often used in a context where the entire container is emptied. It *could* mean that the "entry" of the combo box (or this case the "sample" view) will be cleared, but to me such API does not make sense for GtkComboBox as the sample is by default not user-editable. Still I would not use "text" but some plural form in combination with clear. Like some others already mentioned, "clear_texts" does not look very nice. I see no problems with remove_all_text and it nicely complements the already existing remove_text IMHO. For the other function the best I can come up with is "gtk_combo_box_get_text_count ()".
(In reply to comment #28) > (In reply to comment #27) > ... > "Clear" because it is often used in a context where the entire container is > emptied. It *could* mean that the "entry" of the combo box (or this case the > "sample" view) will be cleared, but to me such API does not make sense for > GtkComboBox as the sample is by default not user-editable. Still I would not > use "text" but some plural form in combination with clear. Like some others > already mentioned, "clear_texts" does not look very nice. I'm not a native english speaker so I can't understand what's wrong with 'texts'. But what I can say for sure is that, if you are removing *multiple* elements and you're not using a plural, the API is confusing. Maybe what should have been used in the first place is "item" instead of "text", this way we could put "items". (1) Anyway, leaving this bug langishing without fixing it is what we shouldn't do. Let's pick one and commit, this is a serious API flaw. (1) (1) We have to learn so many good things from other toolkits...
(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > ... > > "Clear" because it is often used in a context where the entire container is > > emptied. It *could* mean that the "entry" of the combo box (or this case the > > "sample" view) will be cleared, but to me such API does not make sense for > > GtkComboBox as the sample is by default not user-editable. Still I would not > > use "text" but some plural form in combination with clear. Like some others > > already mentioned, "clear_texts" does not look very nice. > > I'm not a native english speaker so I can't understand what's wrong with > 'texts'. But what I can say for sure is that, if you are removing *multiple* > elements and you're not using a plural, the API is confusing. Maybe what should > have been used in the first place is "item" instead of "text", this way we > could put "items". (1) "texts" would be breaking the naming in the first place, so you might just as well ignore the linguistic aspect. > For the other function the best I can come up with is > "gtk_combo_box_get_text_count ()". We can't really do that as long as the suffix _text is the only way to indicate the _text convenience API. gtk_combo_box_get_count_text or gtk_combo_box_count_text might work.
*** Bug 579989 has been marked as a duplicate of this bug. ***
I agree with Kristian that gtk_combo_box_remove_all_text would be nice addition to the current gtk_combo_box_remove_text. Is someone opposed to this name or was this patch forgotten ? :)
Created attachment 143370 [details] [review] Implement gtk_combo_box_clear_all_text
Oops, ignore the title of the attachment, the function us actually called gtk_combo_box_remove_all_text as suggested by Kris.
Review of attachment 143370 [details] [review]: Looks fine to me. If you get Matthias Clasen to agree (also on the function name ;), then feel free to commit I would say. ::: gtk/gtkcombobox.c @@ +5310,3 @@ + g_return_if_fail (GTK_IS_COMBO_BOX (combo_box)); + g_return_if_fail (GTK_IS_LIST_STORE (combo_box->priv->model)); + Can you also add the check for the model column type like all other gtk_combo_box_*_text() functions do? The check is: g_return_if_fail (gtk_tree_model_get_column_type (combo_box->priv->model, 0) == G_TYPE_STRING); @@ +5312,3 @@ + + store = GTK_LIST_STORE (combo_box->priv->model); + gtk_list_store_clear(store); Needs a space after function identifier. ::: gtk/gtkcombobox.h @@ +131,3 @@ gint position); gchar *gtk_combo_box_get_active_text (GtkComboBox *combo_box); +void gtk_combo_box_remove_all_text (GtkComboBox *combo_box); Any reason why the opening parenthesis of the parameter list cannot be aligned with the parenthesis of _get_active_text()?
Created attachment 147666 [details] [review] Implement gtk_combo_box_remove_all_text Updated according to Kris' comments.
Kris?
Fwiw, I agree with comment #27. Imo, the entire combo box convenience api is a failure, and adding more is only making it worse. But if kris wants to add it, go ahead...
Well, it would make much more sense as a derived class. In gtkmm we ignore this API and re-implement it in a derived class, trying to keep it slightly similar.
I filed bug 612396.
We still need this in the new GtkComboBoxText class. I suggest that gtk_combo_box_remove_all_text() (or gtk_combo_box_text_remove_all_text) would not be ideal because it could be misinterpreted as also clearing the text of the GtkEntry. Personally, I'd prefer gtk_combo_box_text_clear_items().
commit 5862075e9dbf6ce76b9b18f250d4694ecafe85e5 Author: Christian Dywan <christian@twotoasts.de> Date: Thu Oct 21 14:25:08 2010 +0200 comboboxtext: Add gtk_combo_box_text_remove_all() https://bugzilla.gnome.org/show_bug.cgi?id=324899