GNOME Bugzilla – Bug 621076
GtkTreeModelFilter does not emit all signals in some situations
Last modified: 2011-08-22 19:50:45 UTC
In Empathy, I'm adding a live search to filter contacts. So when I type "Xavier", the GtkTreeModelFilter only keep contacts that have "Xavier" in their name visible. Since contacts are in groups (tree view levels), a group is visible if and only if there is at least one contact visible in it. The problem appear in that particular situation: STEPS: 1) The live search is activated, and accept only contacts with "Xavier" in the name. 2) There are no contact matching "Xavier", so the treeview is empty. 3) You have a disabled account where there is a contact "Xavier" inside a group "awsome people" 4) enable that account 5) see that nothing appear in the tree view. What happens: 1) Enable that account. 2) in the base model, the group "awsome people" is created. The filter gets that new row, that happens in gtk_tree_model_filter_row_inserted(), it see that there is no root level, so it check if c_iter is visible. But the visible_func says NO because that row still don't have any child. So it goes to 'gone' and the root is not created. 3) in the base model, the contact "Xavier" is created, as child row inside the (still invisible) goup. The filter model gets that new row. This time the c_iter is visible, so it creates the root level with gtk_tree_model_filter_build_level (filter, NULL, -1, FALSE);. That will pull the childs of the root level, so checks if the "awsome people" group row is now visible. Since the row now has a child, the group row is also visible, and gets created in the filter. BUT THAT CHANGE IS NOT EMITTED because emit_inserted arg is FALSE. So in gtk_tree_model_filter_row_inserted() we goes to 'done_and_emit' which will emit that the row "Xavier" is inserted, but since its parent row was not emitted, the view display nothing. I hope this is clear enough. i'll try to write a unit test.
Created attachment 163188 [details] [review] Unit test showing the bug With that test, the only signal emitted is ROW_HAS_CHILD_TOGGLED with path "0". So the test fails because it is expecting ROW_INSERTED too.
Note that this bug also happen if a row was invisible and CHANGED to become visible. In that case the row's parents implicitely become visible too. so gtk_tree_model_filter_row_changed() builds the root level with emit_inserted arg TRUE (so first level elements gets signaled), but it creates other levels using gtk_real_tree_model_filter_convert_child_path_to_path() which does not emit signals. Also note that it is not coherent with gtk_tree_model_filter_row_inserted() which build root level with emit_inserted arg FALSE. To fix this, I think the only solution is when a row is inserted (or changed) in base model and that row is visible in the filter, iterate from the root of the tree to the row's parent and for each level that is not yet build but is now visible, build the level and emit insertion signals.
I made a more complete test case and made a fix. http://git.collabora.co.uk/?p=user/xclaesse/gtk.git;a=shortlog;h=refs/heads/filter Note that the test case specific_root_has_child_filter() is similar and I think it actually suffer from the same bug. If you display a window with the treeview there, I don't think all items will be displayed. Now I have a similar issue when hiding the last child, the parent becomes implicitely invisible, but that's not reflected by the treeview.
Also made the patch for the case where removing the last visible row of a group makes implicitely the group invisible. I also rebased my code on gtk-2-22 branch: http://git.collabora.co.uk/?p=user/xclaesse/gtk.git;a=shortlog;h=refs/heads/bug621076
I made a workaround in Empathy for this bug, now I connect for "row-inserted", "row-deleted" and "row-changed" on the base model, and call gtk_tree_model_row_changed() on its parent row to force the filter to update the visibility of the parent if needed. That works fine even without path in Gtk. However I think could be common usecase, so I would still be good to be fixed in gtk itself. Especially the inconsistency between row_inserted() and row_changed(), when both are inserting the row they should be doing the same thing.
(In reply to comment #3) > Note that the test case specific_root_has_child_filter() is similar and I think > it actually suffer from the same bug. If you display a window with the treeview > there, I don't think all items will be displayed. That's correct, good catch. Although the consistency of the model was being tested in that test, the signals were not checked which is why this was not caught earlier. I will update the test case to also check the signals. Of course your new unit tests will be added as well, they are more sophisticated because they function over multiple levels.
(In reply to comment #2) > To fix this, I think the only solution is when a row is inserted (or changed) > in base model and that row is visible in the filter, iterate from the root of > the tree to the row's parent and for each level that is not yet build but is > now visible, build the level and emit insertion signals. Thinking about this, I do not think it is necessary to build each level along the path. Rather, only row-inserted and row-has-child-toggled have to be emitted for the first node along the path that is invisible. This will make the parent node appear and row-has-child-toggled will get tree view to check the child level of that parent. When this is done, the level is built in the filter model and no row-inserted signals are needed in this case, because the rows are not visible in the tree view anyway (the signals would be ignored). Also no signal is necessary for the node that has actually been inserted in the child model if that level has not been built yet (which means it is not visible in the tree view). I will think more about it later this weekend and start working on a fix. I need to split your patch into chunks, because currently it feels like too much is being changed at once, which makes it very hard to review. For example, it looks like the part which is dealing with virtual roots properly in gtk_tree_model_filter_row_inserted() has been removed and a node lookup is done on a non-corrected path? My plan is to get the refactoring in place first (which I agree is a good idea) and go from there.
Great, thanks for taking care of this! This code is a bit old in my head and I've been doing QT for too long now (sad face) but if you need help on improving the proposed patches, please ask ;-)
I spent quite some hours today doing a very careful refactor (and writing more unit tests along the way) of the row-changed and row-inserted handlers. Now I should be ready to get to the gist of the bug :)
I fixed up the specific-root-has-child-filter and specific-has-child-filter test cases to verify the signals that come in. This has made the tests much more stringent already. These two tests use a visible function which solely depends on the path of the node. Your test case is much more interesting, the visible filter uses both the model content as well as the path. Additionally, it is recursive. I made a few slight changes to your test case: more checks and an additional test for checking that the row-inserted is also emitted when needed after a row-changed on a child node. Next I will look at the remove case and finish up the patch. It looks like it will largely boil down to changing the arguments of two calls to gtk_tree_model_filter_build_level() and adding a new function gtk_tree_model_filter_check_ancestors().
Created attachment 188279 [details] [review] Main patch for this bug This is the main patch I ended up with after a couple of long hacking/debugging sessions. This does not include all refactoring I did before this patch as well as the unit tests which are separate commits in the branch. The plan is as follows (as also highlighted on gtk-devel-list). The patch is now on my treemodel-fix branch. I will address 4 more bugs, as well as write up unit testing for the reference counting in the filter and sort models. When that is done and all nasty filter model bugs that I know of have been fixed, I will merge this into master. We can likely also backmerge this in the latest 2.x branch if that is helpful. Thanks again for your test cases, they are always useful to bring the exact problem across in an efficient way :)
That refactoring seems good and cleaner than my initial patch, good work. However, I have a question in gtk_tree_model_filter_check_ancestors(), in the case elt->visible == false and requested_state==true: Are you sure it's fine to emit child-toggled on its parent even if the parent already had other childs?
(In reply to comment #12) > However, I have a question in gtk_tree_model_filter_check_ancestors(), in the > case elt->visible == false and requested_state==true: Are you sure it's fine to > emit child-toggled on its parent even if the parent already had other childs? Yes, it should not be a problem. row-has-child-toggled is more or less just a "hint". In response a view or model usually checks whether the parent now has children or whether the children have been removed. Or, whether nothing has changed. Good that you bring this up though, I forgot to mention that with this patch, the filter model emits more signals than is strictly necessary. Especially row-has-child-toggled is sometimes emitted twice: once the GtkTreeStore emits such a signal which is propagated, and once a signal generated by the filter model itself. I think we should live with the extraneous signals for now, which are harmless, because it is very hard to reduce these (it might even be impossible). Reducing the signals will likely involve checking for more and more very specific cases, which will make the code messier and it is messy enough as it is already ...
This has been merged into master today.