GNOME Bugzilla – Bug 650673
Gtk::ComboBoxText is not constructed correctly when has_entry is true
Last modified: 2011-06-14 13:50:45 UTC
Overview: This bug concerns only version 2.24 of gtkmm. When a constructor with argument has_entry was added, and some functions were marked deprecated, a few bugs crept into file comboboxtext.cc. The most serious bug is that in the constructors, set_entry_text_column() must be called instead of pack_start(), when has_entry is true. Due to this bug, nothing is shown in the combo box. Calling set_entry_text_column() after the constructor solves that problem, but an unwanted side effect is that each entry is shown twice in the combo box's dropdown menu. Steps to reproduce: Use example program http://git.gnome.org/browse/gtkmm-documentation/tree/examples/book/combobox/entry_text in "Programming with gtkmm". Actual results: If an entry is selected from the dropdown menu, it's not written in the combo box. Instead the following warning is written in the terminal window: GLib-GObject-WARNING **: unable to set property `text' of type `gchararray' from value of type `guchar' Expected results: The selected entry shall be shown in the combo box. No warning. Build date and platform: Ubuntu 11.04, gtkmm 2.24.0 Additional builds and platforms: Additional information: A complete list of all bugs found in ComboBoxText: -- ComboBoxText(bool has_entry) In comboboxtext.cc the argument is called has_model. Shall be has_entry. -- ComboBoxText(bool has_entry) Replace pack_start(m_text_columns.m_column); by if (has_entry) { set_entry_text_column(m_text_columns.m_column); } else { pack_start(m_text_columns.m_column); } -- ComboBoxText(GtkComboBox* castitem) Replace pack_start(m_text_columns.m_column); by if (gtk_combo_box_get_has_entry(castitem)) { set_entry_text_column(m_text_columns.m_column); } else { pack_start(m_text_columns.m_column); } -- void prepend_text(const Glib::ustring& text) Replace append(text); by prepend(text); -- void clear() and void clear_items() In comboboxtext.h clear_items() is within #ifndef GTKMM_DISABLE_DEPRECATED but clear() is not. In comboboxtext.cc it's the other way round. I suppose that both clear() and clear_items() should be within #ifndef GTKMM_DISABLE_DEPRECATED in both files, but I don't know if it can be corrected without breaking API and/or ABI. I can make a patch, but I'm not sure what to do with clear() and clear_items(). Leave them as they are?
Created attachment 188492 [details] [review] patch: ComboBoxText: Ctors call set_entry_text_column() when has_entry is true. Patch for branch gtkmm-2-24. In comboboxtext.cc clear_items() instead of clear() is now guarded by GTKMM_DISABLE_DEPRECATED, as in comboboxtext.h. Otherwise comboboxtext.cc can't be compiled with GTKMM_DISABLE_DEPRECATED defined. I didn't dare to move any "#ifndef GTKMM_DISABLE_DEPRECATED" in comboboxtext.h, since I guess that would count as an API break.
Shall I push the patch? I'm not sure what kinds of bugs are still fixed in branch gtkmm-2-24. This bug is not present in the master branch, and there are workarounds in gtkmm 2.24: - Instead of ComboBoxText(/* has_entry */ true), use the deprecated ComboBoxEntryText(). - Instead of the deprecated ComboBoxText::prepend_text(), use ComboBoxText::prepend().
The patch generally seems good, though I'd prefer you to fix the several problems separately. For instance, - the has_model->has_entry change is just a typo that should be fixed separately. - The deprecation ifdefs should be done separately, though I feel sure that clear() should still be deprecated, as per its documentation. Thanks.
Created attachment 189879 [details] Four patches in branch gtkmm-2-24. ComboBoxText: Fix a typo. ComboBoxText: Fix deprecation comment and inconsistent ifdefs. ComboBoxText: prepend_text() calls prepend(). ComboBoxText: Ctors call set_entry_text_column() when has_entry is true. I have split the changes into 4 patches. Both clear() and clear_items() are deprecated in comboboxtext.cc. Is it good enough now? Shall I push these patches? Should I have split it into five patches instead of just four? The second patch should perhaps have been split into one that fixes the comment in the .h file, and another one that fixes the ifdef in the .cc file? I don't quite understand what can and what cannot be included in one and the same patch. Are any guidelines described somewhere? I've noticed that in gtk+ changes are sometimes split into a huge number of commits.
Yes, please. Please push. The idea is just to keep unrelated things separate, to make it clear what the really important changes are, and to make it easier to know what individual problems were actually fixed. That makes it easier to read the commit log, to cherry-pick changes to other branches, and to identify causes of regressions. Thanks for jumping through my hoops. I generally don't like the micro-commits that GTK+ and others do, which generally just show a development history rather than any self-contained changes, but that rarely happens with gtkmm.
I have pushed the patches in comment 4.