Hi,
There are still some minor issues with python garbage collection. I
briefly talked about this with Marcus on IRC but I thought it would be
a good idea to write this down and get some feedback.
-
python scripts return objects which may be destroyed before we use them
a) PythonExtension::performAction
Should return a pointer to QUndoCommand. Creating a derived
QUndoCommand derived class in python and returning this works fine as
long as the command isn’t destroyed by python garbage collection. But
even if the script holds a reference, reloading plugins will destroy
the command! However, we still want the command to work when we undo
the last action.
QUndoCommand is a simple class, it doesn’t inherit QObject which means
we can’t connect to a destroyed() signal. Since it is such a simple
class I think the best way to handle this would be to create a
Avogadro.UndoCommand class in the python bindings. This class would
inherit QUndoCommand but boost python allows us to hold the object
using a std::auto_ptr. Next we can release (take ownership) of this
auto_ptr in the PythonExtension::performAction C++ function.
b) PythonTool::settingsWidget & PythonEngine::settingsWidget
This has been solved I think, both PythonTool and PythonEngine wrap
the QWidget extracted (using boost::python::extract) in a C++ created
QWidget. So even when the python script returns a QWidget without
holding a reference to it, the wrapper QWidget is still valid and we
don’t segfault… (note: a Tool or Engine’s settings QWidget never has
to live longer than the plugin itself)
- Who owns or should own Molecule(s)?
While we could add a MoleculeStack or something similar to libavogadro
to own Molecules (currently MainWindow owns molecules), I would just
remove python functions like GLWidget::setMolecule and add a copy
function to the python bindings.
This would look like:
…
self.molCopy = Avogadro.Molecule(glwidget.molecule) # OK! this
molecule will never be used outside script
…
glwidget.molecule.copy(self.molCopy) # We don’t actually change the molecule…
…
Currently we allow:
glwidget.molecule = Avogadro.Molecule(self.molCopy) # this calls
GLWidget::setMolecule, the new Molecule is destroyed right away…
We could make this work using an std::auto_ptr which we release in a
function overloading [1] GLWidget::setMolecule in the python bindings,
but even then it doesn’t make much sense. For example, we have no way
to tell MainWindow the molecule has changed.
[1] In case anyone wondered, we can overload any function in the
python bindings like this:
// the first parameter is the object (like self in python)
void GLWidget_setMolecule(GLWidget &glwidget, std::auto_ptr mol)
{
glwidget.setMolecule(mol.get());
mol.release();
}
…
class_<Avogadro::GLWidget, boost::noncopyable>(“GLWidget”)
.add_property(“molecule”, /* get method left out */, &GLWidget_setMolecule)
Suggestions?
Thanks,
Tim