See bug 670601 comment 6 2nd paragraph for why this is happening. 

There's two possible fixes here: The first one is to make the cell data func in ubiquity work with any input.

The second one is that my guess is that ListStore.append() is not implemented in terms of gtk_list_store_insert_with_values(G_MAXINT) but instead uses gtk_list_store_append() followed by gtk_list_store_set(). This will emit a "row-added" followed by a "row-changed" signal instead of insert_with_values() which just emits the "row-added". I think that's not a good idea, so I'll reassign this to the python bindings.

The bindings indeed use append + set.

However having to use MAXINT to mean append looks like a cludge... append and set are public api and that's what everyone looking at the api docs will use (even in their code without using the pygobject override function).

I know you probably had good reasons for the change, but if it breaks apps that use the documented api it still seems like a gtk bug to me.

At the very least an append_with_values api should be added and big red warnings should be written in the append() api docs

The list_store_append docs are clear about the fact that the new row is empty after initially. And we even have a section about atomicity in the intro that spells things out. So, I don't see an urgent need to add new api here.

I would accept a patch that adds a warning to the cell data func docs about the need to deal with empty rows.

So to be clear to anyone, here's the full stuff that happens:

(1) <app> store.append([data])
(2) <pygtk> liststore, append an empty row for me so I can fill it later.
(3) <liststore> done. Hey treeview, a new row appeared
(4) <treeview> k. Hey a11y, there's a new row
(5) <a11y> oh great, what's the content of those cells?
(6) <treeview> wait a minute, i'll call cell_data_func on it
(7) <app> treeview: at it!
(8) <app> wait a minute, all the values are 'None', wtf?
(9) <app> TypeError: argument of type 'NoneType' is not iterable

So IMO this is a bug in step (2) or (8), but not anywhere in GTK.


What happened in GTK 3.2 and before that made it not crash was:

(4.5) <a11y> k. Everything's gonna crash anyway because nobody likes me, so
I'll just queue an idle handler and update things later. *whine*

And because we like a11y because it got a makeover and is all pretty now, we
don't treat him like that anymore.

Fixing pygobject override to be something like the following would be simple

    def append(self, row=None):
        if row is not None:
            # Use insert_with_value with position -1 to get atomic behavior
            treeiter = Gtk.ListStore.insert_with_values(self, -1, row)
        else:
            treeiter = Gtk.ListStore.append(self)

        return treeiter

but it is a bit of a slippery slope since then we'd have to change also the other variants and fixing insert_before and insert_after would mean duplicating a large chunk of logic which is already in gtk.

Probably gtk should have insert_before|after_with_values


In the mean time I think the cell_data_func of the app should be fixed to deal with None since as pointed out in the other comments the fact that appending is not atomic is documented behavior.

In a way the override is already duplicating API. gtk_list_store_insert_with_valuesv() is introspectable, so in the spirit of staying close to the original API that's what Python programs should really use.

But as we have the additional override API now, I think we can just as well make it use the atomic functions, as they are also more performant.

Well, that's what I started doing but when I reached insert_before and insert_after I saw that the amount of code is not trivial and I did not really feel like adding it, but if someone wants to do it I will surely not complain :-)

In principle it's not actually that hard to change the code to convert the incoming values first, and then call Gtk.ListStore.insert_with_valuesv(). I have a prototype patch here, but it fails on str values, they always come back out as "". I'm investigating this more closely.

I reported the empty str values problem as bug 672065 and attached a small test case there. This needs to be fixed before we can use insert_with_valuesv() which expects a GValue* array.

Created attachment 209807 [details] [review]
Atomic inserts in Gtk.{List,Tree}Store overrides

What do you think about this patch?

The diff formatting is not that lucky, but essentially it does:

 * Add a new _convert_row() helper, and use it in _set_row(). This factors out the data structure conversion, and leaves the actual set_value() call in _set_row().

 * Factor out the "row is not none -> convert || row is none -> insert" common logic into a new _do_insert() helper.

 * Use do_insert in append(), prepend(), etc.

All tree and list iter test cases work fine with this, and they are quite complex and cover a lot of corner cases.

Note that with this you get a couple of

  Gtk-WARNING **: gtkliststore.c:851: Unable to convert from (null) to gint

warnings, as the test cases also cover None values in the data rows. With the previous code which set each individual cell these got filtered out, but as insert_with_valuesv() sets them all in one go, the NULLs now make it all the way to the C functions. I think that's ok, as in C you would get the exact same warning, and even in Python you are not actually supposed to have None values in rows.

Note that this patch does not change insert_{before,after}(), as there is no
counterpart of insert_with_valuesv() which takes a TreeIter instead of a
position. If someone has an idea how to efficiently convert a TreeIter into a position (and I'm not sure that's even possible in a race free manner), I'm happy to change these two as well.

Review of attachment 209807 [details] [review]:

I have no problem with this approach, but let's at least add a big FIXME near insert_before/after overrides explaining the situation

::: gi/overrides/Gtk.py
@@ +939,3 @@
 
+    def append(self, row=None):
+        return self._do_insert(0x7fffffff, row)

is this for maxint? note that the gtk api documents that you can use -1 to mean append and I find that cleaner

> big FIXME

Sure, can do.

> +    def append(self, row=None):
> +        return self._do_insert(0x7fffffff, row)
> 
> is this for maxint? note that the gtk api documents that you can use -1 to mean
> append and I find that cleaner

Yes, it's supposed to be maxint. sys.maxint doesn't work on 64 bit, as it's the full 64 bit max int, and GTK only gets along with the 32 bit value.

I did use -1 first, but that doesn't seem to work properly -- it breaks the test case, it gets 99 rows instead of the expected 103.

For the record, the depending bug 672065 got fixed now, so this can go ahead.

Actually, the documentation for both {list,tree}_store_insertv() says:

  If position is larger than the number of rows on the list, then the new row will be appended to the list.

It says nothing about "-1". If I do use -1, then I get

  Gtk-CRITICAL **: gtk_list_store_insert: assertion `position >= 0' failed

So the _store_insert_with_valuesv() functions actually do seem to get along with -1, but _store_insert() don't.

Thus I'd rather keep them as 0x7fffffff and add a comment.

Created attachment 209921 [details] [review]
Atomic inserts in Gtk.{List,Tree}Store overrides

This updates the patch with some extra comments about the 0x7ffffff and the FIXMEs for the still non-atomic insert_{before,after}.

-1 is documented only on gtk trunk since its documentation was a side effect of this discussion :)

since gtk_list_store_insert does not handle -1 I'd be tempted to avoid the generic _do_insert refactoring and do the "if" explicitely in the append/prepend override:

        if row is not None:
            # Use insert_with_value with position -1 to get atomic behavior
            treeiter = _do_insert(self, -1, row)
        else:
            treeiter = Gtk.ListStore.append(self)

Review of attachment 209921 [details] [review]:

Not sure how well tested that code is, would probably make sense to add at least a couple of tests that  verifies that row-added is emitted.

::: gi/overrides/Gtk.py
@@ +946,3 @@
 
+    def append(self, row=None):
+        # sys.maxint is too big on 64 bit machines, so use a hardcoded MAXINT here

This is confusing.

Do you want the maximum value of an int32? If so adding a constant such as _MAX_INT32 = (2 << 31) - 1 would probably make more sense.

I am adding test cases now, and it turns out that the current code is even worse: it doesn't send out two signals (add, change), but (number of non-NULL cells in that row)+1 signals, as you get a change event for each column.

Created attachment 209933 [details] [review]
Atomic inserts in Gtk.{List,Tree}Store overrides

This removes the 0x7fffffff hack and adds test cases to ensure we get the signals we expect, and none more.

Review of attachment 209933 [details] [review]:

Patch looks good to me. The fact that list_store_insert does not handle -1 looks like a bug in gtk, but we can remove the workaround when/if that is fixed.

PyGTK did allow -1 and I ran into that when porting my app to GI, so we might want to add it either way.

Pushed, thanks for the review!