GNOME Bugzilla – Bug 128058
gtk_tree_model_iter_previous() would be nice.
Last modified: 2011-01-06 04:40:33 UTC
It would be nice to have a gtk_tree_model_iter_previous() as well as the current gtk_tree_model_iter_next(). Totem has an implementation, though I guess it's significantly slower than gtk_tree_model_iter_next(): http://cvs.gnome.org/lxr/source/totem/src/totem-playlist.c#146
In a gtk-devel-list email, Owen Taylor said: " I've written that function before, and think it would be useful to have in GTK+. Would need a warning in the docs about performance. We could potentially add a vtable member to GtkTreeModel in the future to allow models to accelerate it when possible with the model's data structures. I've discussed this with Jonathan before, forget what his response was. But post-2.4. " and Jonathan Blandford said: " The current GtkTreeModel interface is the minimum needed for the GtkTreeView/GtkSortModel to work. People have previously requested this and other model interface changes, and like Owen said, we can certainly add them. A concern is that it's not needed for the vast majority of cases, and I am not going to add the back-pointers to make it 'fast' in either of the store models. That means that it'll really only be fast in custom models -- which can add their own prev call anyway. "
This bug doesn't depend on gtk 3.0, taking out that dependency.
Reading Murray's quotes, is that a WONTFIX then? Or could we add a gtk_list_store_get_previous_iter function to the GtkListStore?
By the way, we added this in gtkmm already even though it's not in gtk+, with clear documentation that it's slow.
Created attachment 167478 [details] [review] iter prev function implementations The perfomance problem mentioned by Jonathan Blandford has already been solved. List store depends on GSequence which is implemented internally with a balanced binary tree.
Before we can consider adding this, we need implementations for GtkTreeModelFilter and GtkTreeModelSort. It would probably also be a good idea to have a (slow) default implementation so that custom models don't suddenly break.
A default implementation is a good idea. I have always been hesitant to add new methods to the GtkTreeModel interface because of the unclear migration path. In this case, a simple default/fallback can be implemented, but for some features that is not the case unfortunately.
Created attachment 167954 [details] [review] iter prev function implementations
Created attachment 167955 [details] [review] iter prev function implementation for sort model
Created attachment 167956 [details] [review] iter prev function implementation for filter model
Created attachment 167957 [details] [review] iter prev function implementation for filter model (alternate version)
Review of attachment 167954 [details] [review]: It is probably also a very good idea to extend the unit tests for list/tree store to test the new implementations of the iter_prev method. ::: gtk/gtkliststore.c @@ +566,3 @@ + retval = g_sequence_iter_is_begin (iter->user_data); + if (retval) + iter->stamp = 0; I don't fully get this. If we get to the begin node, we invalidate the iterator? What happens if iter is at path "1", do we go to path "0" which is the begin iter and thus it will be invalidated? ::: gtk/gtktreemodel.c @@ +1208,3 @@ + return FALSE; + + retval = gtk_tree_path_prev (path) && I think we should invalidate the iter if gtk_tree_path_prev yields FALSE. Although GtkTreeModel should ideally stay generic, I think setting stamp to zero should be fine. ::: gtk/gtktreestore.c @@ +637,3 @@ + return FALSE; + } +} Perhaps have another return FALSE at the end to silence the compiler in case it spits out a warning.
Review of attachment 167955 [details] [review]: This patch looks fine to me.
(In reply to comment #11) > Created an attachment (id=167957) [details] [review] > iter prev function implementation for filter model (alternate version) Do the unit tests still fully pass with this patch in place (since iter_next has been modified). I prefer the patch from comment 10 however, even though it uses an extra i variable like the current implementation of iter_next. I think it is a little more clear, and there is a smaller risk of making mistakes due to unexpected behavior of the pointer arithmetic.
Created attachment 168461 [details] [review] iter prev function implementations with fixes of bugs has been mentioned in comment12
Created attachment 168490 [details] [review] iter prev function implementations Changes compared to the previous version of the patch: - Added since marker to the documentation - Added the new function to gtk.symbols - Added the new function to docs/reference/gtk/gtk3-sections.txt
Even though we have a gtk_tree_path_prev, I would prefer if we used a real word and called it gtk_tree_model_iter_previous. Things that are still missing: - implementations for filter and sort models
(In reply to comment #17) > Things that are still missing: > > - implementations for filter and sort models Is there any problem with patches in comment 9, comment 10 and comment 11?
(In reply to comment #18) > Is there any problem with patches in comment 9, comment 10 and comment 11? I think 9 and 10 should be fine (see comment 14).
(In reply to comment #17) > Even though we have a gtk_tree_path_prev, I would prefer if we used a real word > and called it gtk_tree_model_iter_previous. Sounds good to me. > Things that are still missing: > > - implementations for filter and sort models These are there, see comments 9 and 10. But yes, it would be good if this is merged into the same patch. I would still like to see unit tests.
Created attachment 175839 [details] [review] iter previous function implementation Changes compared to the previous version of the patches: * renamed function prefixes of _iter_prev to _iter_previous * implemented unit tests