[Okular-devel] An outline and some questions regarding undo framework

[Jon Mease]

Greetings,

So I've spent a good bit of time studying the code, planning, and
prototyping ideas for the undo framework and I wanted to lay them out
here for general discussion.

Location of the QUndoStack:  My current thinking is that the undo
stack should be a member of DocumentPrivate.  Methods on Document that
currently directly alter the state of the annotations in the document
should be moved from Document to DocumentPrivate.  These methods
include addPageAnnotation, removePageAnnotation(s), and
modifyPageAnnotation.

New methods on Document should be added (corresponding to those listed
above) that instantiate appropriate QUndoCommands and push them to the
document's undo stack.  The QUndoCommands, in turn, will make use of
the private methods that actually alter the documents annotations.

Three QUndoCommands need to be written:  addPageAnnotationCommand,
removePageAnnotationCommand, and modifyPageAnnotationCommand.  The
first two have turned out to be fairly simple (Based on the approach
Albert suggested in my now discarded review request from a few days
ago) and I have fully functional prototypes written of both.

The trickiest part has been working out how to implement the
modifyPageAnnotationCommand.  We have lots of styles of annotations
each with lots of individual properties and so it seems pretty
impractical to me to create individual modification undo commands for
each annotation type and each property.  What would be ideal would be
to have some kind of annotation property descriptor object that would
capture all of the state of a particular annotation.  This way we
could call getPropertyDescriptor on the annotation before and after
modifying it and have the modification undo command hold on to these
two descriptors.  Then the undo and redo actions would simply call
setPropertyDescriptor() on the annotation object with the appropriate
property descriptor object to toggle its state back and forth.

But then it occurred to me that storing and loading an annotation's
state in xml is a very similar problem.  We currently have a store()
method on the Annotation class that stores the annotations state to a
QDomNode.  We also have constructors for each Annotation subclass that
input a QDomNode and use it to initialize their state.  In a sense,
these QDomNodes could be thought of as an annotation's property
descriptor object as described above.

It's straight forward to write a little getPropertyDescriptorDom
method on the Annotation class that uses store() to return a QDomNode
containing the annotation's state.  Using this method the
modifyPageAnnotation undo command could hold onto two QDomNodes
representing the before and after states of an annotation.  The undo
and redo actions would then use the appropriate QDomNode to update the
annotations internal state.

How does everyone feel about this general approach of leveraging the
xml storage logic and using QDomNodes to represent an annotation's
internal state?  I really like that the undo framework would not be
dependent upon all of the individual annotation subclasses and their
properties. Instead it would only be dependent upon a working xml
storage/loading capability.

If this general approach makes sense (of using QDomNodes to represent
an annotation's state) then the last remaining challenge is to figure
out how to use a QDomNode to update the state of an existing
annotation.  Currently the logic for parsing the QDomNode and setting
up an annotation's state is located in one of the constructors of each
annotation type.  We could refactor this logic (for each annotation
type) out of the constructor and into a standard method called
something like setAnnotationStateFromDomNode().  This way the logic to
set an annotation's state based on a QDomNode is available to both the
constructor and to the modifyPageAnnotation undo command.

I've implemented a rough version of this approach and it works pretty
well overall.  The only issue is that the QDomNode parsing code in the
constructors of some of the annotation types assume that their working
with freshly initialized instantiations of the private member
variables.  For example, the InkAnnotationPrivate class has a couple
of lists as members that are added to by the InkAnnotation constructor
without being cleared first.  So in order to be able to use a
setAnnotationStateFromDomNode() method we'd have to make some of that
initialization explicit.

One other question I have is where should the Undo and Redo Actions
live?  Right now for prototyping I've added them to the PageView's
action collection so that they can be added to the toolbar for testing
purposes.  Should Undo and Redo show up in Edit menu?  If so, any
thoughts on how to get them there?

Ok, that's enough for now.  I would appreciate feedback on all of the
above.  I would also appreciate feedback on whether it would be
feasible to try to get this in for 4.10.  Depending the feedback I get
I may be able to have something ready to review by early next week
(probably Tuesday).
Thanks!
-Jon