[PyQt] Bug in QTableView.setModel()

[Phil Thompson]

On Tuesday 08 January 2008, Aaron Digulla wrote:
> Quoting Andreas Pakulat <apaku at gmx.de>:
> >> I'm talking about preventing the python class which is used as the model
> >> being garbage collected; it doesn't matter from which Qt class it is
> >> derived.
> >>
> >> As it is, the class pointer is copied into the view class and then
> >> python frees the memory. That causes spurious display errors and
> >> crashes. No matter how you look at it, that's a bug.
> >
> > No its not, the view doesn't take ownership of the model, neither in Qt
> > C++ nor in PyQt, thats why your object is garbage collected. A reason
> > for not taking ownership is that a model might be used in multiple
> > views.
>
> Incrementing the usage count != ownership. I'm the one who decides
> which object in my project "owns" the model. This is a design issue.

Wrong. The owner of a C++ instance has responsibility for calling it's dtor. A 
QObject parent will call its children's dtors whether you think you "own" the 
child or not.

> That the table doesn't say "please don't GC this because I still need
> it" has nothing to do with ownership. It's a usage counter bug.
>
> Proof: If N tables access the model, the usage count will be N. As
> every table is GC'd, the usage count drops. When the last table is
> GC'd, the usage count will be 0 and the model will be GC'd, too. So
> there is no reason why a table must not increment the usage count.

Wrong. You are talking about the Python object that wraps the C++ model, but 
its the C++ model that matters. Keeping the Python object alive when the true 
owner has deleted the C++ model doesn't help.

> But there are plenty of reasons why it should:
>
> 1. In Python, I expect all any every object to manage the ownership of
> anything I store in it. If it needs it beyond the time of the call
> where it sees the object for the first time, it must increment and
> decrement the usage counter accordingly. See
> http://docs.python.org/ext/refcounts.html

As I said, we are dealing with the lifecycle of C++ instances, not Python 
objects.

> 2. This bug doesn't produce deterministic errors. That makes it hard
> to locate and leads to the developer thinking "PyQt is unreliable" (in
> addition to the many wasted hours trying to find the bug).

Maybe spending a minute with Google would have helped.

> 3. It's simple to fix: In setModel(), you just have to check if there
> already is an old model. If so, check if it's a python object and
> decrement its usage count if it is. Then save the new pointer and
> increment its usage count if it's a python object.

It's doesn't fix all circumstances. It also needs to support the cyclic GC.

> 4. The fix doesn't break anything.

It breaks a behaviour pattern that is consistent throughout PyQt.

The concept of ownership is generally well understood by PyQt programmers. 
setModel() seems to be the only place that catches people out. People wrongly 
assume that the view takes ownership of the model - the fact that it doesn't 
is part of the Qt API, and PyQt isn't in the business of changing the API.

In C++ you should be managing the lifecycle of the model you create (either by 
giving it a parent or by keeping a pointer to it and deleting it when 
appropriate). The same applies in PyQt. If you don't do so then you have a 
bug in your code. The difference is that in C++ the effect of the bug is a 
memory leak, but in PyQt it is often a crash.

I'm happy to consider ways to make this more obvious (even if it's just a 
better FAQ), but I'm not happy about departing from the Qt API and 
introducing inconsistencies in fundamental behaviour.

Phil