Hi list,
I’m sure many of you saw this:
http://sourceforge.net/tracker/?func=detail&aid=3459981&group_id=165310&atid=835077
This bug report summarizes a lot of the feedback I get from my
colleagues after they use avogadro for a while – they like the
program, see potential, but it’s just not stable enough for everyday
use. I’ve noticed this myself – in fact, I always get a tad nervous
and mentally review recent changes before hitting “Undo” in case I did
anything slightly out of the ordinary that may trigger a crash.
I think that “Undo” is one area of avogadro that really needs some
attention. I’m as guilty as anyone; many of my early patches did not
implement undo at all, and even recent patches that had undo (e.g. the
crystallography extension) did so in a way that cluttered the internal
data structures and left some things inconsistent.
I’ve been thinking about how we can improve this, and my current
favorite solution would be a “MoleculeDocument” class. The key
concepts that go into this class:
- MoleculeDocument holds a pointer to the current molecule
- MoleculeDocument has API functions that modify the molecule
(addAtom, removeBond, etc) - Each function that modifies the molecule creates an UndoCommand to
carry out the specified changes - Changes can be concatenated
Concatenation can be carried out automatically behind the scenes using
QUndoStack::(begin|end)Macro() and/or QUndoCommand::mergeWith():
http://developer.qt.nokia.com/doc/qt-4.8/qundostack.html#id-288389e4-1582-4772-bda4-381569e3ada4
If all modification to the molecule is performed through the
MoleculeDocument and the class is heavily unit-tested and stable, then
undo becomes much simpler and more reliable. As an example, here are
some things I’d like to be able to do from an extension/tool/etc:
Add a methane molecule at the origin. This code would produce at least
9 individual undo commands, 5 AddAtomCommands and 4 AddBondCommands.
Depending on how the calls are made, there may be
SetBondOrderCommands, TranslateAtomCommands, etc may be produced as
well. They will all be concatenated into a single UndoCommand with the
name “Add methane”.
MoleculeDocument *doc = getCurrentMoleculeDocument();
doc->beginModify(“Add methane”);
doc->addAtom(6, 0.0, 0.0, 0.0); // Carbon at origin
doc->adjustHydrogens();
doc->endModify();
We could do something similar for crystals:
MoleculeDocument *mdoc = getCurrentMoleculeDocument();
CrystalDocument doc = qobject_cast<CrystalDocument>(mdoc);
if (!doc) return; // Not a crystal
doc->beginModify(“Construct crystal”);
doc->setCellParams(a, b, c, alpha, beta, gamma);
doc->addAtom(…); // etc
doc->setSpacegroup(“Pnma”);
doc->fillUnitCell();
doc->reduceToPrimitiveCell();
doc->endModify()
Again, each function in the Document class will produce a number of
UndoCommands that are concatenated into a single command, “Construct
crystal”.
This would be a lot of work and it might prove inefficient for some
corner cases (like extremely large-scale changes), but I think that
the reliability payoff far exceeds this, and we could always allow for
custom undo commands to be used for those “special” cases that are
problematic.
Any thoughts or opinions? This would be a large scale change, and I’m
not sure what version something like this should be slotted for. But
as evidenced by the bug report, we are turning away a lot of potential
users with a negative experience and this is a major area that needs
improvement.
Dave
