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 622987 - Can't insert a Python dict into a GtkTreeStore
Can't insert a Python dict into a GtkTreeStore
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks: 622963
 
 
Reported: 2010-06-27 22:07 UTC by Philip Withnall
Modified: 2010-11-12 15:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[gi] make parameter check less strict when dealing with GValue params (10.91 KB, patch)
2010-10-12 16:49 UTC, johnp
accepted-commit_now Details | Review
GDB log (24.94 KB, text/plain)
2010-10-12 20:41 UTC, Sebastian Pölsterl
  Details
Sample (936 bytes, text/plain)
2010-10-13 12:33 UTC, Sebastian Pölsterl
  Details

Description Philip Withnall 2010-06-27 22:07:57 UTC
An operation that used to work with PyGTK, I have a GtkTreeStore with a column of type PyObject. Appending a row containing a dict errors: the call to set_value in the TreeStore.append override now throws the error: "TypeError: argument 3: Must be of a known GType, not dict".

Apparently this worked before the tree view overrides landed.
Comment 1 johnp 2010-09-07 02:32:10 UTC
This should work now that I use the internal method for converting python types to GTypes.  Can you confirm?
Comment 2 Philip Withnall 2010-09-07 21:16:54 UTC
I can't test it at the moment, as libpeas hasn't been updated to follow the latest gobject-introspection changes, which are required for PyGObject to build (GIArgument rename).

Once that's all sorted, I'll give it a test. Thanks!
Comment 3 Philip Withnall 2010-09-19 13:19:30 UTC
It doesn't appear to be working using pygobject master (269ff8564eeb597dc06c27e293354b7ff7a71a82):

Traceback (most recent call last):
  • File "/opt/gnome2/build/lib64/totem/plugins/jamendo/jamendo.py", line 355 in on_fetch_albums_loop
    self.add_treeview_item(treeview, album)
  • File "/opt/gnome2/build/lib64/totem/plugins/jamendo/jamendo.py", line 267 in add_treeview_item
    [album, album['image'], title, dur, tip]
  • File "/opt/gnome2/build/lib64/python2.6/site-packages/gtk-2.0/gi/overrides/Gtk.py", line 401 in append
    self.set_value(treeiter, i, row[i])
  • File "/opt/gnome2/build/lib64/python2.6/site-packages/gtk-2.0/gi/types.py", line 40 in function
    return info.invoke(*args)
TypeError: argument 3: Must be of a known GType, not dict


jamendo.py:266–268 (http://git.gnome.org/browse/totem/tree/src/plugins/jamendo/jamendo.py#n266):

parent = treeview.get_model().append(None,
    [album, album['image'], title, dur, tip]
)

where "album" is a dict.
Comment 4 Sebastian Pölsterl 2010-10-09 15:26:48 UTC
It seems that all *store only accept exactly the defined type, but not any type that inherit from it. If you use Gtk.ListStore(object) it will only accept object types, but nothing else, where it actually should accept all Python types.
Comment 5 johnp 2010-10-11 13:59:40 UTC
I see, this comes down to the strict type checking we do.  I'm looking into this.
Comment 6 johnp 2010-10-11 15:33:46 UTC
The issue is pyg_type_from_object.  Is it safe to return PY_TYPE_OBJECT for anything?  I mean everything is a PyBaseObject_Type at the end of the day so this will remove a level of error checking.  

At issue is PyGtk does its own pointer checking using GValues while we use the generic parameter checking algorithm.  Looking into it more
Comment 7 johnp 2010-10-11 19:52:27 UTC
We lose here.  In PyGTK the type checking happens in the override and then a GValue is passed without further checking.  The problem we have is in the generic case we have no context on what that GValue should be.  By returning a PY_TYPE_OBJECT for anything that passes through we start to hit asserts in the tests which crash them.

The only way I see us fixing this is being able to send in GValues which are checked in the overrides and just passed through.  I however feel that this opens us up to some potential bugs.

The workaround on the app side is to create a wrapper gobject class which you used to wrap any python objects before sending them into a store.

I'll see if I can at least get dictionaries and lists working.
Comment 8 johnp 2010-10-11 21:15:31 UTC
I have come to the conclusion that checks for GValue are inane as there is little context to make the right decisions.  They are like Python interfaces, anything goes, you need to check the inputs yourself.  The best option would be to register a type checking function in the overrides for any interface that takes a GValue.  If it is not registered, it shouldn't be able to be called or should use the current limited check.  I'll see if I can whip that up tomorrow.
Comment 9 johnp 2010-10-12 16:49:45 UTC
Created attachment 172200 [details] [review]
[gi] make parameter check less strict when dealing with GValue params

* Some GValue API can store a pointer to a python object for later
  use but our parameter checking was too strict to allow this
* Add pyg_type_from_object_full API which takes a strict boolean and
  returns PY_TYPE_OBJECT if no other GType can be found
* Since we don't have enough info to genrically check GValue parameters
  use the less strict type guessing when encountering a GValue param
* Other API stays the same and continues to do strict testing
Comment 10 johnp 2010-10-12 16:50:29 UTC
Please see if this fixes your issues.  We still need to add checks to the override.
Comment 11 Sebastian Pölsterl 2010-10-12 20:41:42 UTC
Created attachment 172214 [details]
GDB log

With the patch applied I can no longer import gst. python -c "import gst" results in a segfault, gdb output is attached.
Comment 12 johnp 2010-10-13 00:10:12 UTC
Did you recompile the python-gst?  I think I changed the ABI.  I might need to move the pointer to the function to the end of the API struct
Comment 13 Sebastian Pölsterl 2010-10-13 12:33:01 UTC
Created attachment 172257 [details]
Sample

I did not, but doesn't matter, should have tried an easier example anyway.

With the attached example I get:
Traceback (most recent call last):
  • File "treeview-test.py", line 35 in <module>
    win.add(view)
  • File "/opt/gnome3/lib/python2.6/site-packages/gtk-2.0/gi/types.py", line 40 in function
    return info.invoke(*args)
TypeError: argument 1: Must be Gtk.Widget, not MyView

Seems I can't subclass any widget anymore, which is not a good thing at all.
Comment 14 Sebastian Pölsterl 2010-10-13 12:34:29 UTC
To clarify, this doesn't work with or without the patch.
Comment 15 johnp 2010-10-13 14:35:19 UTC
Ah, ok, try the patch in bug #631631.  That should fix the inheritance issue.
Comment 16 Sebastian Pölsterl 2010-10-13 19:16:24 UTC
Unfortunately not, the error still persists.
Comment 17 johnp 2010-10-14 17:33:28 UTC
I don't know what is going on with your instance of pygobject.  Works fine on my machine with or without the patches on my local branch.  Can you run a make check and post the output?
Comment 18 Sebastian Pölsterl 2010-10-20 08:52:36 UTC
I cleaned my build tree, now with the two patches applied everything works like expected.
Comment 19 Philip Withnall 2010-10-20 18:30:33 UTC
With both patches applied, it's no longer throwing the original error, and appears to work as well as with the workaround. It's emitting these warnings:

(totem:6851): Gtk-WARNING **: gtktreestore.c:765: Unable to convert from PyObject to GObject

** (totem:6851): WARNING **: Out argument 2 in get_path_at_pos returns a struct with a transfer mode of "full". Transfer mode should be set to "none" for struct type returns as there is no way to free them safely.  Ignoring transfer mode to prevent a potential invalid free. This may cause a leak in your application.


The second warning is emitted even when using the workaround, so it's probably what's causing the code to not work properly (with or without the workaround).
Comment 20 johnp 2010-10-21 16:10:59 UTC
Philip,

The second warning simply doesn't free the struct.  We can't free it if it is not boxed (how do we know how it was allocated?) It shouldn't effect your application but really paths should be boxed.

What is at issue is gtk_tree_view_get_path_at_pos is a completely broken API as far as introspection is concerned (well the whole tree API is since it trades off consistent API for and simplicity for flexibility and optimisation points).  What is at issue is the method signature has a weird mix of caller allocated pointers and callee allocated out parameters.  It is just not wrapable and a new API needs to be added which simply returns the path, column and cell coordinates in a sane way. Perhaps something that packs them all in a boxed structure.
Comment 21 johnp 2010-10-21 16:53:39 UTC
Also, can you provide a backtrace for us to look at?
Comment 22 johnp 2010-10-21 16:59:10 UTC
Hmm, actually when looking at the first warning, this issue isn't happening in gtk_tree_view_get_path_at_pos.   It is happening in gtk_tree_store_real_set_value.  Can you break at gtktreestore.c:765 and copy the backtrace here?  Also if you can distil it down to a testcase that would be great.
Comment 23 Philip Withnall 2010-10-23 12:27:36 UTC
(totem:14525): Gtk-WARNING **: gtktreestore.c:765: Unable to convert from PyObject to GObject


Breakpoint 1, gtk_tree_store_real_set_value (tree_store=0xe171f0, iter=0x89e620, column=0, value=0x89e060, sort=1) at gtktreestore.c:766
766		  return retval;
Missing separate debuginfos, use: debuginfo-install alsa-plugins-pulseaudio-1.0.22-1.fc13.x86_64 libffi-3.0.9-1.fc13.x86_64 libudev-153-4.fc13.x86_64 libuuid-2.17.2-9.fc13.x86_64 orc-0.4.10-1.fc13.x86_64
(gdb) t a a bt

Thread 1 (Thread 0x7ffff7fb5920 (LWP 14525))

  • #0 gtk_tree_store_real_set_value
    at gtktreestore.c line 766
  • #1 gtk_tree_store_set_value
    at gtktreestore.c line 860
  • #2 ffi_call_unix64
    from /usr/lib64/libffi.so.5
  • #3 ffi_call
    from /usr/lib64/libffi.so.5
  • #4 g_function_info_invoke
    at gifunctioninfo.c line 417
  • #5 _invoke_function
  • #6 _wrap_g_function_info_invoke
  • #7 ext_do_call
    at Python/ceval.c line 4074
  • #8 PyEval_EvalFrameEx
    at Python/ceval.c line 2485
  • #9 PyEval_EvalCodeEx
  • #10 fast_function
    at Python/ceval.c line 3860
  • #11 call_function
    at Python/ceval.c line 3785
  • #12 PyEval_EvalFrameEx
    at Python/ceval.c line 2445
  • #13 PyEval_EvalCodeEx
    at Python/ceval.c line 3026
  • #14 fast_function
    at Python/ceval.c line 3860
  • #15 call_function
    at Python/ceval.c line 3785
  • #16 PyEval_EvalFrameEx
    at Python/ceval.c line 2445
  • #17 fast_function
    at Python/ceval.c line 3850
  • #18 call_function
    at Python/ceval.c line 3785
  • #19 PyEval_EvalFrameEx
    at Python/ceval.c line 2445
  • #20 PyEval_EvalCodeEx
    at Python/ceval.c line 3026
  • #21 function_call
  • #22 PyObject_Call
    at Objects/abstract.c line 2492
  • #23 instancemethod_call
  • #24 PyObject_Call
    at Objects/abstract.c line 2492
  • #25 PyEval_CallObjectWithKeywords
  • #26 _pyglib_handler_marshal
    at pyglib.c line 562
  • #27 g_idle_dispatch
    at gmain.c line 4254
  • #28 g_main_dispatch
    at gmain.c line 2149
  • #29 g_main_context_dispatch
    at gmain.c line 2702
  • #30 g_main_context_iterate
    at gmain.c line 2780
  • #31 g_main_loop_run
    at gmain.c line 2988
  • #32 gtk_main
    at gtkmain.c line 1321
  • #33 main
    at totem.c line 297

Comment 24 Philip Withnall 2010-10-23 12:55:19 UTC
Investigating a little more, the patch is working properly; the plugin was just defining the column in question as a GObject column in the UI file. Changing it to a PyObject fixed things. The other problems in the plugin are separate problems.

When can we have a release with this patch applied? :-)
Comment 25 johnp 2010-10-25 16:03:28 UTC
oh, yay,  I was thinking the problem was a lot deeper and would need a lot of work to fix but since you found the issue, I'll do a release this week.
Comment 26 Tomeu Vizoso 2010-10-25 17:03:01 UTC
Review of attachment 172200 [details] [review]:

Looks good to me, some comments:

* may exist a better name than pyg_type_from_object_full, what will happen if we need to add another parameter? Also, there's no indication of what that TRUE/FALSE means to the casual reader.

* there's spacing errors in test_overrides.py.
Comment 27 Philip Withnall 2010-11-11 16:07:57 UTC
Ping? Has this been committed yet? :-)
Comment 28 johnp 2010-11-12 15:46:33 UTC
Yep, it is in the 2.27.0 release :)  Thought I closed this.