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

[Albert Astals Cid]

El Dissabte, 20 d'octubre de 2012, a les 18:36:40, Jon Mease va escriure:
> Greetings,

Hi

> 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.

That or you need to give the annotations a property system, that'd be the 
ideal since it'd be generic and you could even generalize the xml saving code, 
but that's a different beast, i for one are ok with using the xml for the 
states (if that does not explode the used memory for state tracking)

> 
> 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?  

Of course, where would you put them otherwise?

> If so, any thoughts on how to get them there?

sure, xmlgui is your friend, have a look at the .rc files

> 
> 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).

For 4.10 we have lots of other stuff in the pipeline that needs to be reviewed 
before than yours (just because it came before).

Also it seems you did not take into account undo/redo in forms, which i guess 
should be added if you want to have "proper" undo/redo in Okular, not sure if 
we should release a version of okular with undo/redo in annotations but not in 
forms, opinions?

Cheers,
  Albert

> Thanks!
> -Jon
> _______________________________________________
> Okular-devel mailing list
> Okular-devel at kde.org
> https://mail.kde.org/mailman/listinfo/okular-devel