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 671610 - Make TreeStore/ListStore override API atomic wrt. change signals
Make TreeStore/ListStore override API atomic wrt. change signals
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: general
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on: 672065
Blocks:
 
 
Reported: 2012-03-08 00:04 UTC by Luke Yelavich
Modified: 2012-03-19 13:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Python source code that the traceback referrs to. (127.46 KB, text/plain)
2012-03-08 00:04 UTC, Luke Yelavich
  Details
Atomic inserts in Gtk.{List,Tree}Store overrides (5.72 KB, patch)
2012-03-15 09:01 UTC, Martin Pitt
none Details | Review
Atomic inserts in Gtk.{List,Tree}Store overrides (6.70 KB, patch)
2012-03-16 11:40 UTC, Martin Pitt
none Details | Review
Atomic inserts in Gtk.{List,Tree}Store overrides (9.74 KB, patch)
2012-03-16 14:48 UTC, Martin Pitt
accepted-commit_now Details | Review

Description Luke Yelavich 2012-03-08 00:04:42 UTC
Created attachment 209223 [details]
Python source code that the traceback referrs to.

This bug is being experienced in the Ubuntu installer, ubiquity, python 2.7, gobject-introspection 1.31.20, pygobject 3.1.1, GTK 3.3.18, latest at-spi and atk.

The advanced partitioner of the installer uses the GtkTreeViewColumn widget, and sets a custom data method for the model, file ubi-partman.py attached for reference, the traceback is as follows:

Traceback (most recent call last):
  • File "/usr/lib/ubiquity/plugins/ubi-partman.py", line 593 in partman_column_name
    if 'id' not in partition:
TypeError: argument of type 'NoneType' is not iterable

Traceback (most recent call last):
  • File "/usr/lib/ubiquity/plugins/ubi-partman.py", line 593 in partman_column_name
    if 'id' not in partition:
TypeError: argument of type 'NoneType' is not iterable

This bug occurrs when atk-bridge is loaded.

Going back through GTK releases, I found that this bug was not present in 3.3.4, and appeared in 3.3.6. Git bisecting between those two releases gave me the following commit as the possible culpret, I say possible as I may have missed something along the way.

commit 1a3226e2f7d730d17643b2abb4072d3369479448
Author: Benjamin Otte <otte@redhat.com>
Date:   Mon Dec 12 16:20:11 2011 +0100

    a11y: Emit children-changed properly for treeviews
    
    Fixes the patch reverted in b7e74ef95f1d9cd851fb81a124beca0ca11dad00
    properly.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=548782
Comment 1 Benjamin Otte (Company) 2012-03-08 00:23:40 UTC
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.
Comment 2 Paolo Borelli 2012-03-10 17:48:46 UTC
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
Comment 3 Matthias Clasen 2012-03-10 18:18:50 UTC
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.
Comment 4 Benjamin Otte (Company) 2012-03-10 18:20:02 UTC
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.
Comment 5 Paolo Borelli 2012-03-10 20:00:17 UTC
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.
Comment 6 Martin Pitt 2012-03-14 11:07:45 UTC
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.
Comment 7 Paolo Borelli 2012-03-14 11:14:58 UTC
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 :-)
Comment 8 Martin Pitt 2012-03-14 13:35:38 UTC
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.
Comment 9 Martin Pitt 2012-03-14 15:00:00 UTC
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.
Comment 10 Martin Pitt 2012-03-15 09:01:50 UTC
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.
Comment 11 Paolo Borelli 2012-03-15 09:34:33 UTC
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
Comment 12 Martin Pitt 2012-03-15 10:06:39 UTC
> 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.
Comment 13 Martin Pitt 2012-03-15 13:30:44 UTC
For the record, the depending bug 672065 got fixed now, so this can go ahead.
Comment 14 Martin Pitt 2012-03-16 11:39:39 UTC
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.
Comment 15 Martin Pitt 2012-03-16 11:40:34 UTC
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}.
Comment 16 Paolo Borelli 2012-03-16 12:00:10 UTC
-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)
Comment 17 Johan (not receiving bugmail) Dahlin 2012-03-16 12:01:01 UTC
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.
Comment 18 Martin Pitt 2012-03-16 14:44:48 UTC
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.
Comment 19 Martin Pitt 2012-03-16 14:48:55 UTC
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.
Comment 20 Paolo Borelli 2012-03-19 08:04:24 UTC
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.
Comment 21 Johan (not receiving bugmail) Dahlin 2012-03-19 11:50:22 UTC
PyGTK did allow -1 and I ran into that when porting my app to GI, so we might want to add it either way.
Comment 22 Martin Pitt 2012-03-19 13:30:34 UTC
Pushed, thanks for the review!