GNOME Bugzilla – Bug 582981
an editable TreeViewColumn when a TreeModelFilter is used as TreeModel doesn't work properly
Last modified: 2011-03-30 09:42:19 UTC
Please describe the problem: In a TreeView containing an editable boolean column (added using append_column_editable (Glib::ustring, TreeModelColumn<ColumnType>) ) and using a TreeModelFilter as TreeModel, when toggling the N-th row in the TreeView, the N-th row in the TreeModel on which the TreeModelFilter is based is changed. And they mostly don't are the same. Steps to reproduce: I've attached an example showing this bug Actual results: Expected results: Does this happen every time? Other information:
Created attachment 134834 [details] A simple program affected by this bug
I thought I had fixed this ages ago in this commit: 2004-10-30 Murray Cumming <murrayc@murrayc.com> * gtk/src/treemodelfilter.[hg|ccg]: Added implementations of set_value_impl() to ouput a warning when it is used instead of the child model. Added const versions of patch convert methods. * gtk/src/treeview.[hg|ccg]: Added _get_base_model() non-public API member method to get the child model, if necessary. Used it in the cell renderer signal handlers instead of get_model(). * Added examples/book/treeview/filter, from the gtkmm-2-4 branch. Maybe you could investigate why that is not working. Of course there are only convenience methods, so in the worst case you can just construct the view columns manually.
I've tried to workaround the bug by creating the column manually, and by doing it I think I've discovered what causes it. I'll try to explain it as good as I can. When connecting the "toggled" signal to the cellrenderer, the callback function receives as argument the path of the row, in the form of a ustring. If it tries to get the iter using the treemodelfilter (by doing filter->get_iter(path)), it doesn't work and gtkmm prints a warning, I think because of the changes you've made in the commit you've mentioned above. If it tries to get the iter using the treemodel on which the treemodelfilter is based (by doing filter->get_model()->get_iter(path)) it gets the wrong iter, because the path (which is simply a number) is not referred to that treemodel. This is the actual behavior of gtkmm, and because of this the bug appears. If it tries to get the iter using the treemodelfilter, but then it converts it to a treemodel iter (using filter->convert_iter_to_child_iter) it gets the right iter and behaves as expected. Resuming: the bug is related to the fact that the model passed to TreeView::_auto_store_on_cellrenderer_toggle_edited_with_model is obtained using TreeView::_get_base_model. There should be a control inside the first function (or inside the function which calls it) to check whether the model is a treemodelfilter or not, and, if it is, to modify the iter opportunely.
I've also tried to make a patch to resolve it (at least for boolean columns). I attach it but it's probably so bad-written that you won't use it.
Created attachment 134878 [details] [review] An ugly patch to resolve the problem I've made this patch using the git version of gtkmm, and using git's tool for patch creation
I've seen that even non-bool columns are affected by this bug, so I've written a more complete patch that fixes this bug for every column type. As said above, I've used the git version of gtkmm to make this patch. I don't know if there are some regression tests that have to be done before submitting a patch, but the modified code compiles fine and, using the example attached above, it seems to resolve the bug.
Created attachment 136982 [details] [review] A more complete patch to resolve this bug
What are all these changes about:? - void _auto_store_on_cellrenderer_toggle_edited_with_model(const Glib::ustring& path_string, int model_column, const Glib::RefPtr<Gtk::TreeModel>& model); + void _auto_store_on_cellrenderer_toggle_edited_with_model(const Glib::ustring& path_string, int model_column, Glib::RefPtr<Gtk::TreeModel> model);
I'm not an expert at using const but I thought that: - since the reference to the TreeModel may be modified, the const should be removed - since I don't know if the parent function continues to use the reference after the call to these functions, it's better to make a copy of the reference and not to use the same, so if it gets modified it doesn't influence the parent function. So I removed the & too Would it be better to leave them there?
I don't think you understand the constness of the (smart)pointers there. Please try it without changing that. For a function parameter, const something& is just more efficient than something.
Created attachment 138630 [details] [review] Another patch, with const parameters If I only revert the changes to the parameters, leaving them in the form of "const something&", the build fails saying that using the operator= on a const RefPtr "discards qualifiers". But I changed the other code so that it doesn't need to modify the model passed as parameter, and now it seems to work even with const parameters. I attach the new patch (ah, I've also made some very little aesthetic changes)
Created attachment 173829 [details] Test case, including proposed bug fix This bug report describes the same bug as #403707 - TreeModelFilter: Wrong row updated when TreeView::append_column*(). I think there is a solution which is both simpler and more general than the one in comment 11. That solution can be found in Gtk::TreeModelSort::set_value_impl, which forwards the request for changing a value to its child model. Let TreeModelFilter do the same thing, and remove all special treatment of TreeModelFilter from TreeView. This solution is more general because it works for all combinations of TreeModelFilter and TreeModelSort, also when a TreeModelFilter is the child of a TreeModelSort. See the attached test case. It's a modification of the test case in comment 1. You can test with or without my proposed solution. I've tested the solution on the test case in bug #403707. It works there too. Tested on Ubuntu 10.10 with gcc 4.4.5, gtkmm 2.20.3, gtk+ 2.22.0. The solution in my test case is implemented in an ugly way, because I didn't want to build the whole gtkmm library file from source code. A proper implementation would be: 1. Make TreeModelFilter a friend of TreeModel, just as TreeModelSort is. 2. Remove TreeView::_get_base_model, and replace all calls to it by calls to TreeView::get_model. 3. Make TreeModelFilter::set_value_impl almost identical to TreeModelSort::set_value_impl. The only differences shall be that calls to gtk_tree_model_sort_* shall be replaced by calls to gtk_tree_model_filter_*.
Created attachment 173888 [details] [review] New proposed patch This is the patch resulting from the application of your suggestions. I didn't test it at all because I was asked to have gtk+-3 or gtk+-2.22 which my distribution still doesn't provide, so I can't even tell if it will compile. I'd be glad if someone with a up-to-date build environment tries to apply this patch and does some testing, like running one of the above programs.
As you may guess from comment 12, I have no build environment for gtkmm, and therefore I can't test your patch in comment 13. But I've looked at it. It seems correct except for one (or possibly two) details. - In treemodel.hg, add class TreeModelFilter; (Search for the corresponding declaration of TreeModelSort.) - Not necessary, but recommended: In treeview.ccg, remove #include <gtkmm/treemodelfilter.h>
Created attachment 177092 [details] [review] Proposed patch that uses set_value_impl() of TreeModelFilter in auto-stores I corrected the patch above: * I added "class TreeModelFilter;" as suggested, * I chose to keep the _get_base_model() method, modifying it so that it would get the base model of a chain of TreeModelFilter *and* TreeModelSort. So, instead of removing the #include in treeview.ccg, I added also the one for treemodelsort.h. I did it because I thought that removing this method may break the API/ABI. I made these changes to version 2.20.1 of gtkmm (the one that's installed on my system) so I could test it, and then I merged it with git's master branch. I tested it with both examples and it works fine. If this patch will be accepted, I think this bug can be considered fixed.
The patch in comment 15 is a good solution in branches where no API/ABI breaks are allowed. And since all calls to TreeView::_get_base_model() have been removed, that function can also be removed where API/ABI breaks are allowed, such as in gtkmm 3. Now I have installed the build environment for gtkmm with jhbuild. If more patches are needed, I can help with making and testing them. Actually jhbuild is a great tool. It was easier than I had thought to get it all to work.
The patch in comment 15 was implemented on 2011-03-16 with commits http://git.gnome.org/browse/gtkmm/commit/?id=3e1de6ce4475933b7a3f72403d6c6e23775e1cbd http://git.gnome.org/browse/gtkmm/commit/?id=8073fc59effc57669fa0a183aa436f05dfd7def3 Shouldn't this bug be closed (fixed)? And bug 403707 can be marked as a duplicate of this one.
Yes, sorry. I must have been in a rush. Or something was lost during the bugzilla downtime. Thanks for the patch, Luca.
*** Bug 403707 has been marked as a duplicate of this bug. ***