[Okular-devel] Review Request 107442: Add undo/redo support for annotations

[Jon Mease]

> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote:
> >

Thanks for the feedback.  I have a couple questions inline below


> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote:
> > ui/annotationpropertiesdialog.cpp, line 209
> > <http://git.reviewboard.kde.org/r/107442/diff/6/?file=119429#file119429line209>
> >
> >     Maybe we could not modify the annot in ::slotapply but modify it in Document::modifyPageAnnotationProperties? And then you wouldn't need the map in there either because you would get the "old" annotation as parameter?

The difficulty here is that most of the work in modifying the annotation's properties is actually performed in the applyChanges() method of the AnnotationWidget (m_annotWidget).  And we can't pass the AnnotationWidget into the Document::modifyPageAnnotationProperties method because of the GUI dependency.  So I'm not really sure how this could be done (but I'm open to suggestions).


> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote:
> > core/document.cpp, line 3282
> > <http://git.reviewboard.kde.org/r/107442/diff/6/?file=119425#file119425line3282>
> >
> >     I am wondering why do we need this map for the prev contents, as far as i can see annotwindow does not modify the annotation anymore, so the annotation * you get here should have the old values still no?

Yes, we could assume that the old contents are still in the annotation but this makes me a little nervous because there's nothing stopping a user of Okular core from modifying the annotation's contents as well which would then break the undo stack.  But we could do it this way and just add API documentation specifying that the annotation's contents should not be updated.  

If we do go this route I have a question.  The AnnotationWindow class uses the function GuiUtils::contents rather than Annotation::contents when retrieving the contents of an annotation.  Would the Document class need to use this GuiUtils version as well in order to grab the old contents (rather than Annotation::contents?).  I'm not really clear from the source what conditions the GuiUtils version is handling.  If so we'll need to move this function out of GuiUtils (because of the GUI dependency) and into some other core class.  Let me know if you have a preference of where to move it to.


> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote:
> > core/document.h, line 394
> > <http://git.reviewboard.kde.org/r/107442/diff/6/?file=119424#file119424line394>
> >
> >     Do we need the bool? Seems it's only called once with true, no?

Yes, it looks like we're only ever setting it to true now.  I will get rid of it.


> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote:
> > ui/annotwindow.h, line 38
> > <http://git.reviewboard.kde.org/r/107442/diff/6/?file=119430#file119430line38>
> >
> >     We don't seem to use it anymore, or maybe we didn't already, but can you kill the annotation() method?

Will do


- Jon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107442/#review29227
-----------------------------------------------------------


On March 12, 2013, 1:26 a.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
> 
> (Updated March 12, 2013, 1:26 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> This patch adds undo/redo support to Okular annotation manipulation commands.
> 
> Functionality:
> The following actions can be undone and redone: creation and removal of annotations, editing arbitrary annotation properties, relocating annotations with Ctrl+drag, and editing the text contents of an annotation.
> 
> This patch does not include support for undoing and redoing editing actions on forms.
> 
>   
> 
> 
> This addresses bug 177501.
>     http://bugs.kde.org/show_bug.cgi?id=177501
> 
> 
> Diffs
> -----
> 
>   core/annotations.h 72abdff 
>   core/annotations.cpp 49ab5bd 
>   core/annotations_p.h 221572d 
>   core/document.h 6ff6536 
>   core/document.cpp 5ab759e 
>   core/document_p.h fb3aec6 
>   core/page.cpp 1db2763 
>   part.rc 39c1571 
>   ui/annotationpropertiesdialog.cpp 4b02258 
>   ui/annotwindow.h f7df9f6 
>   ui/annotwindow.cpp c1bafb9 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/107442/diff/
> 
> 
> Testing
> -------
> 
> I have tested the undoing and redoing of the specified annotation actions using .dvi and .pdf documents.
> 
> 
> Thanks,
> 
> Jon Mease
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130316/b6c38187/attachment-0001.html>