After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 128058 - gtk_tree_model_iter_previous() would be nice.
gtk_tree_model_iter_previous() would be nice.
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkTreeView
2.3.x
Other All
: Normal enhancement
: Small API
Assigned To: gtktreeview-bugs
gtktreeview-bugs
Depends on:
Blocks: 88667
 
 
Reported: 2003-11-27 14:10 UTC by Murray Cumming
Modified: 2011-01-06 04:40 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
iter prev function implementations (5.43 KB, patch)
2010-08-10 06:39 UTC, Szilárd Pfeiffer
needs-work Details | Review
iter prev function implementations (5.96 KB, patch)
2010-08-16 11:00 UTC, Szilárd Pfeiffer
needs-work Details | Review
iter prev function implementation for sort model (2.41 KB, patch)
2010-08-16 11:02 UTC, Szilárd Pfeiffer
none Details | Review
iter prev function implementation for filter model (2.80 KB, patch)
2010-08-16 11:02 UTC, Szilárd Pfeiffer
none Details | Review
iter prev function implementation for filter model (alternate version) (3.31 KB, patch)
2010-08-16 11:05 UTC, Szilárd Pfeiffer
none Details | Review
iter prev function implementations (6.30 KB, patch)
2010-08-21 17:24 UTC, Szilárd Pfeiffer
none Details | Review
iter prev function implementations (7.12 KB, patch)
2010-08-22 09:10 UTC, Szilárd Pfeiffer
needs-work Details | Review
iter previous function implementation (24.45 KB, patch)
2010-12-04 15:25 UTC, Szilárd Pfeiffer
none Details | Review

Description Murray Cumming 2003-11-27 14:10:20 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
Comment 1 Murray Cumming 2004-01-22 10:47:42 UTC
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.
"
Comment 2 Michael Natterer 2007-03-07 12:45:08 UTC
This bug doesn't depend on gtk 3.0, taking out that dependency.
Comment 3 Bastien Nocera 2007-04-11 11:34:07 UTC
Reading Murray's quotes, is that a WONTFIX then? Or could we add a gtk_list_store_get_previous_iter function to the GtkListStore?
Comment 4 Murray Cumming 2007-04-14 13:25:22 UTC
By the way, we added this in gtkmm already even though it's not in gtk+, with clear documentation that it's slow.
Comment 5 Szilárd Pfeiffer 2010-08-10 06:39:30 UTC
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.
Comment 6 Matthias Clasen 2010-08-10 11:52:15 UTC
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.
Comment 7 Kristian Rietveld 2010-08-10 12:22:59 UTC
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.
Comment 8 Szilárd Pfeiffer 2010-08-16 11:00:16 UTC
Created attachment 167954 [details] [review]
iter prev function implementations
Comment 9 Szilárd Pfeiffer 2010-08-16 11:02:28 UTC
Created attachment 167955 [details] [review]
iter prev function implementation for sort model
Comment 10 Szilárd Pfeiffer 2010-08-16 11:02:59 UTC
Created attachment 167956 [details] [review]
iter prev function implementation for filter model
Comment 11 Szilárd Pfeiffer 2010-08-16 11:05:12 UTC
Created attachment 167957 [details] [review]
iter prev function implementation for filter model (alternate version)
Comment 12 Kristian Rietveld 2010-08-21 12:04:35 UTC
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.
Comment 13 Kristian Rietveld 2010-08-21 12:05:59 UTC
Review of attachment 167955 [details] [review]:

This patch looks fine to me.
Comment 14 Kristian Rietveld 2010-08-21 12:09:54 UTC
(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.
Comment 15 Szilárd Pfeiffer 2010-08-21 17:24:33 UTC
Created attachment 168461 [details] [review]
iter prev function implementations

with fixes of bugs has been mentioned in comment12
Comment 16 Szilárd Pfeiffer 2010-08-22 09:10:10 UTC
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
Comment 17 Matthias Clasen 2010-08-25 02:27:22 UTC
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
Comment 18 Szilárd Pfeiffer 2010-08-25 08:11:37 UTC
(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?
Comment 19 Kristian Rietveld 2010-08-25 18:54:47 UTC
(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).
Comment 20 Kristian Rietveld 2010-08-25 18:55:25 UTC
(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.
Comment 21 Szilárd Pfeiffer 2010-12-04 15:25:34 UTC
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