GNOME Bugzilla – Bug 631547
Annoying thing while porting to introspection (Gtk.TreeModel.set_value/get_value)
Last modified: 2010-10-21 17:18:51 UTC
I used to be able to do: model[iter][COLUMN] = value And now, I have to do: model.set_value (iter, COLUMN, value) It's okay. But the first version feels so much more python-y that it'd be great to have it back. (same for get_value) Don't know if it's possible, though?
I think it is possible if I can overload indexes in python. I'll take a look.
This involves recreating the TreeModelRow API in pygobject model.__getitem__ must be overridden to accept a TreeIter, TreePath, integer or tuple (which represents a TreePath) and it must return a TreeModelRow. Similarly TreeModelRow must implement __getitem__ to take an integer which represents the column which the user is trying to access. __setitem__ must also be implemented so we may set the column value. docs here: http://www.pygtk.org/docs/pygtk/class-pygtktreemodelrow.html Not sure if I want to take time to write the entire API but I will start it.
Created attachment 171911 [details] [review] Restore TreeModel behavior Many of the iter_* and get_iter_* functions used to return a tuple where the first element indicated the return value of the C function. These functions now return None or throw an error depending on their behavior in GTK+ 2.x. TreeModel implements __getitem__ which can deal with Gtk.TreeIter, Gtk.TreePath, int, str, and tuple. In addition, it implements __iter__ to iterate the topmost level of the tree. The TreeModelRow class has been added, it should exactly behave like in GTK+ 2.x. However, TreeModelRow.iterchildren() currently returns None if there are no children (as stated in http://www.pygtk.org/docs/pygtk/class-pygtktreemodelrow.html#method-gtktreemodelrow--iterchildren). In my opinion it would make more sense to return an (empty) iterator nevertheless. If I missed a method or feature, please let me know. I would add proper test cases and doc strings if requested.
Created attachment 171912 [details] [review] Fixed bug where I should have used get_n_columns Fixed a small bug where I used the wrong function to get the number of columns.
Comment on attachment 171912 [details] [review] Fixed bug where I should have used get_n_columns >From 12db54b72e363aea1d58618b08bafff56ba3d0f3 Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Sebastian=20P=C3=B6lsterl?= <sebp@k-d-w.org> >Date: Thu, 7 Oct 2010 19:37:53 +0200 >Subject: [PATCH] Make TreeModel behave like in GTK-2.x > >--- > gi/overrides/Gtk.py | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 147 insertions(+), 0 deletions(-) > >diff --git a/gi/overrides/Gtk.py b/gi/overrides/Gtk.py >index 1f6901c..1591026 100644 >--- a/gi/overrides/Gtk.py >+++ b/gi/overrides/Gtk.py >@@ -358,6 +358,67 @@ class TreeModel(Gtk.TreeModel): > def __len__(self): > return self.iter_n_children(None) > >+ def __getitem__(self, key): >+ if isinstance(key, (Gtk.TreeIter, Gtk.TreePath,)): >+ return TreeModelRow(self, key) >+ elif isinstance(key, (int, str,)): >+ aiter = self.get_iter_from_string(str(key)) >+ return TreeModelRow(self, aiter) >+ elif isinstance(key, tuple): >+ if len(key) == 0: >+ raise ValueError("invalid tree path") >+ path_str = ":".join(str(val) for val in key) >+ aiter = self.get_iter_from_string(path_str) >+ return TreeModelRow(self, aiter) >+ else: >+ raise TypeError, key We need to be be Python 3 compatible here so: raise TypeError(key) Also is that a sufficient enough error string? The rest looks good to me but you need tests
FWIW there are quite a few tests from pygtk that should be reused; http://git.gnome.org/browse/pygtk/tree/tests/test_liststore.py Not too comprehensive but a good starting point
Created attachment 171971 [details] [review] Improved patch I added a better error description. I moved most of the stuff from __getitem__ to get_iter, because in PyGTK 2.x get_iter used to work with paths, int, str and tuple as well. TreePath.__cmp__ was added. Regarding iterchildren() it turned out that the documentation was wrong, actually it *always* returns an iterator in PyGTK 2.x. Therefore, I changed the behavior of this function. In addition, I tried my best to raise the same errors as in PyGTK 2.x.
Created attachment 171972 [details] [review] Test cases I adjusted the existing tests and added test_tree_model, which completes both in PyGI and PyGTK 2.21.
(In reply to comment #6) > FWIW there are quite a few tests from pygtk that should be reused; > http://git.gnome.org/browse/pygtk/tree/tests/test_liststore.py > > Not too comprehensive but a good starting point Just had a look at those tests which contain tests for negative indices. I didn't know that they were supported, therefore this feature is still missing.
*** Bug 626384 has been marked as a duplicate of this bug. ***
Review of attachment 171971 [details] [review]: You should probably include the tests and the implementation in the same patch. ::: gi/overrides/Gtk.py @@ +358,1 @@ def __len__(self): Is __nonzero__() properly implemented as well? @@ +366,3 @@ + aiter = self.get_iter(key) + except ValueError: + raise IndexError("could not find tree path") Include the argument in the exception message, for improved debuggability @@ +386,3 @@ + success, aiter = super(TreeModel, self).get_iter(path) + if not success: + raise ValueError("invalid tree path") Ditto @@ +393,3 @@ + return TreePath.new_from_string(path) + except TypeError: + raise TypeError("could not parse subscript as a tree path") Ditto @@ +397,3 @@ + def get_iter_first(self): + success, aiter = super(TreeModel, self).get_iter_first() + if not success: You can replace the following 3 lines with: if success: return aiter @@ +404,3 @@ + success, aiter = super(TreeModel, self).get_iter_from_string(path_string) + if not success: + raise ValueError("invalid tree path") Include the path_string in the error message @@ +410,3 @@ + next_iter = aiter.copy() + success = super(TreeModel, self).iter_next(next_iter) + if not success: See above about return values for this and the following three functions @@ +470,3 @@ + %s found" % type(iter_or_path).__name__) + + def __getattr__(self, name): Properties are more appropriate for this kind of mappings; @property def path(self): return self.model.get_path(self.iter) etc @@ +493,3 @@ + + def __getitem__(self, key): + if isinstance(key, int): Float should be accepted here as well, at least if PyGTK permits it @@ +514,3 @@ +__all__.append('TreeModelRow') + +class TreeModelRowIter: new-style classes makes sense here
(In reply to comment #11) > Review of attachment 171971 [details] [review]: > ::: gi/overrides/Gtk.py > @@ +358,1 @@ > def __len__(self): > > Is __nonzero__() properly implemented as well? > If __nonzero__ is not implemented __len__ is used, so it's fine. > @@ +493,3 @@ > + > + def __getitem__(self, key): > + if isinstance(key, int): > > Float should be accepted here as well, at least if PyGTK permits it > PyGTK does not permit it.
Created attachment 172043 [details] [review] Fixed issues, merged patches, support negative indices I fixed the issues you mentioned and added support for negative row and column indices.
__cmp__ is no longer supported in Python 3.x. Please implement the richcompare methods as needed: object.__lt__(self, other) object.__le__(self, other) object.__eq__(self, other) object.__ne__(self, other) object.__gt__(self, other) object.__ge__(self, other) object.__hash__(self) # may not be required http://docs.python.org/release/3.1/reference/datamodel.html#object.__lt__
Created attachment 172124 [details] [review] Use rich comparison methods Replaces __cmp__ with rich comparison methods, __hash__ is not implemented.
(In reply to comment #13) > (In reply to comment #11) > > Review of attachment 171971 [details] [review] [details]: > > ::: gi/overrides/Gtk.py > > @@ +358,1 @@ > > def __len__(self): > > > > Is __nonzero__() properly implemented as well? > > > If __nonzero__ is not implemented __len__ is used, so it's fine. No, __nonzero__ and __len__ are two different things, if __len__ is implemented you should generally implement __nonzero__ returning false for classes that are not exclusively used as list/map abstractions. For instance if you have a function returning a model and have code such as: model = self.create_model() if model: .. use model .. You don't have if model to return false if there are no items in the model. So please implement __nonzero__ as returning False which was the behavior in old PyGTK
Also, implement __bool__ which is what __nonzero__ was renamed to in 3.x. Since we are mainly supporting Python 3 in the future you could write this method as def __bool__(...): # do checks here __nonzero__ = Class.__bool__
From http://docs.python.org/release/3.1/reference/datamodel.html#object.__bool__: "When this method is not defined, __len__() is called, if it is defined, and the object is considered true if its result is nonzero." That's what I'm referring to, same is true for __nonzero__.
Sorry, now I get the difference. That means __bool__ always returns True to indicate that this a valid model, right?
Created attachment 172199 [details] [review] Implemented TreeModel.__bool__ This patch adds TreeModel.__bool__ and __nonzero__
Looks good but I'm getting this in make check: (runtests.py:23357): Gtk-CRITICAL **: gtk_tree_path_new_from_string: assertion `*path != '\000'' failed (runtests.py:23357): Gtk-CRITICAL **: gtk_tree_path_new_from_string: assertion `*path != '\000'' failed That needs to be looked into before this is committed
Created attachment 172814 [details] [review] Raise error if tree path string is empty I added a check in the override to avoid the assertion.