Created attachment 328559 [details] [review]
overrides: add iterable protocol to Gio.ListModel

This allows you to iterate Gio.ListModel instances with something like:

 for item in model:
     print(item)

Review of attachment 328559 [details] [review]:

Hi Christian,

Thanks for the patch. This version is not caching as far as I can tell. The returned iterable's next is called as needed.

You could probably get away with using list comprehension for caching and a generator for non-caching:

# list comp
def __iter__(self):
    return iter([self.get_item(i) for i in self.get_n_items()])


# generator
def __iter__(self):
    for i in self.get_n_items():
        yield self.get_item(i)

Both implementations cache the item count. Personally I prefer the non caching generator version since it will distribute the performance hit of a large model. Developers can then wrap the result in a list if they want a cache: list(model)

This will also need unit tests.

Created attachment 342319 [details] [review]
[PATCH] overrides: Add Gio.ListStore overrides

This should address the previous comment and it also implements overrides for Gio.ListStore for setting and removing items.

Created attachment 342336 [details] [review]
[PATCH] overrides: Add Gio.ListStore and Gio.ListModel overrides

Add custom ListModel implementation tests

Review of attachment 342336 [details] [review]:

Thanks for the added tests. Here's some initial feedback. I would also vote for using monkey patching rather than inheritance:

def _list_model_iter(self):
    for i in range(self.get_n_items()):
        yield self.get_item(i)

Gio.ListModel.__iter__ = _list_model_iter
Gio.ListModel.__len__ = Gio.ListModel.get_n_items

::: gi/overrides/Gio.py
@@ +245,3 @@
+            raise TypeError
+        if key < 0:
+            raise IndexError

I don't think we need the explicit type or range check. PyGI will do the int type check for us and raise an OverflowError if a negative is passed in.

@@ +246,3 @@
+        if key < 0:
+            raise IndexError
+    def __getitem__(self, key):

Any reason for using the vfunc directly instead of the invoker (self.get_item) ?

@@ +248,3 @@
+        ret = self.do_get_item(key)
+        if ret is None:
+        if not isinstance(key, int):

Give a more informative message:

    "index 42 out of range 0-41"

@@ +256,3 @@
+    def __iter__(self):
+        for i in range(len(self)):
+        ret = self.do_get_item(key)

Avoid an extra level of indirection for internals by using get_item() directly?

@@ +264,3 @@
+
+class ListStore(Gio.ListStore):
+

Since ListStore implements ListModel, I don't think getitem and len need overrides?

@@ +266,3 @@
+    def __getitem__(self, key):
+        if not isinstance(key, int):
+    def __len__(self):

Same comment as above, we should get these type checks for free from PyGI (unless you are adding a more informative error message).

@@ +280,3 @@
+            raise IndexError
+        if not isinstance(value, GObject.Object):
+

This case should be handled by the "is a" test below.

@@ +281,3 @@
+        if not isinstance(value, GObject.Object):
+            raise TypeError
+ListModel = override(ListModel)

possibly use: self.get_item_type().is_a(value)

@@ +283,3 @@
+        if not GObject.type_is_a(value.__gtype__, self.get_item_type()):
+            raise TypeError
+__all__.append('ListModel')

insert has different semantics than setitem, perhaps splice needs to be used here?

::: tests/test_overrides_gio.py
@@ +9,3 @@
+
+class TestObject(GObject.Object):
+	id = GObject.Property(type=int)

Use four spaces instead of tabs.

@@ +19,3 @@
+
+	def setUp(self):
+		self.l = Gio.ListStore.new(TestObject)

Can we use a more meaningful variable name here? self.list_store for example.

(In reply to Simon Feltman from comment #5)
> Review of attachment 342336 [details] [review] [review]:
> 
> Thanks for the added tests. Here's some initial feedback.
>
> ::: gi/overrides/Gio.py
> @@ +245,3 @@
> +            raise TypeError
> +        if key < 0:
> +            raise IndexError
> 
> I don't think we need the explicit type or range check. PyGI will do the int
> type check for us and raise an OverflowError if a negative is passed in.

Yes it did raise an OverflowError, I just wasn't sure if that was ideal.
 
> @@ +246,3 @@
> +        if key < 0:
> +            raise IndexError
> +    def __getitem__(self, key):
> 
> Any reason for using the vfunc directly instead of the invoker
> (self.get_item) ?
>
> @@ +256,3 @@
> +    def __iter__(self):
> +        for i in range(len(self)):
> +        ret = self.do_get_item(key)
> 
> Avoid an extra level of indirection for internals by using get_item()
> directly?
>
> @@ +264,3 @@
> +
> +class ListStore(Gio.ListStore):
> +
> 
> Since ListStore implements ListModel, I don't think getitem and len need
> overrides?
> 

I don't know why but if the custom implementation that implements this interface used get_item() it would segfault but it worked for the liststore subclass of course.


> @@ +283,3 @@
> +        if not GObject.type_is_a(value.__gtype__, self.get_item_type()):
> +            raise TypeError
> +__all__.append('ListModel')
> 
> insert has different semantics than setitem, perhaps splice needs to be used
> here?

Oh didn't even notice Gio.ListStore.splice(). Yes I'll try that out.

(In reply to Patrick Griffis (tingping) from comment #6)
> (In reply to Simon Feltman from comment #5)
> > @@ +283,3 @@
> > +        if not GObject.type_is_a(value.__gtype__, self.get_item_type()):
> > +            raise TypeError
> > +__all__.append('ListModel')
> > 
> > insert has different semantics than setitem, perhaps splice needs to be used
> > here?
> 
> Oh didn't even notice Gio.ListStore.splice(). Yes I'll try that out.

The bindings for this actually seem broken?

    self.splice(key, 1, (value,))

That does remove 1 item but the new values are never inserted.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/115.

I've opened an MR for this: https://gitlab.gnome.org/GNOME/pygobject/merge_requests/59