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

[Jon Mease]

> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/document.h, line 408
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97031#file97031line408>
> >
> >     Wondering if "newProps" is redundant. I mean, isn't newProps always the same as annotation->getAnnotationPropertiesDomNode() ?

Good catch, yes newProps is the same as annotation->getAnnotationPropertiesDomNode().  Removed newProps param in v3


> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/document.h, line 410
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97031#file97031line410>
> >
> >     Please document what completeDrag is for

Removed completeDrag from version 3 and documented the method


> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/document.h, line 412
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97031#file97031line412>
> >
> >     Please document textEdit and commit too

Removed textEdit from method in v3


> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/document.cpp, line 2016
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97032#file97032line2016>
> >
> >     We need to check that uc is a TranslateAnnotationCommand too, don't we? (see example at http://qt-project.org/doc/qt-4.8/qundocommand.html#mergeWith)
> 
> Fabio D'Urso wrote:
>     Sorry, just read the doc again. Even tough the example code seems to suggest this check is needed, it's not necessary.

Thanks for checking into it, I'll drop the issue then.


> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/document.cpp, line 2018
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97032#file97032line2018>
> >
> >     Yeah, uniqueName sounds like a good way to identify annotations, but actually there's no guarantee that these names are unique. Will it work if we just compare tuc->m_annotation != m_annotation instead?
> >     
> >     Okular-created annotations are always assigned random UUIDs and chances they collide are low, but according to PDF specifications such unique names are optional and, if they exist, they are unique only locally for a given page.

Thanks for explaining the issue with "uniqueName". In version 3 we're comparing the Annotation pointers themselves rather than the unique name.


> On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote:
> > core/page.cpp, line 869
> > <http://git.reviewboard.kde.org/r/107442/diff/2/?file=97034#file97034line869>
> >
> >     Any reason we have to this for each page? Can't we do this at the end of DocumentPrivate::loadDocumentInfo, or, even better, bypass the undo stack while loading saved annotations?

Good point, in version 3 we're bypassing the undo stack when loading saved annotations.


- Jon


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


On Dec. 31, 2012, 3:12 a.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2012, 3:12 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> This patch is a first cut at adding undo/redo support to Okular.  This patch is not yet complete, however it is far enough along that I would like to begin incorporating feedback from the community.
> 
> 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 yet include support for undoing and redoing editing actions on forms.  I plan to implement this form undo functionality before the functionality of this patch is included in Okular.
> 
> Known Issue:
> When editing an annotation's properties in a .dvi file the annotation is altered and the action can be undone as expected.  However, when editing an annotation's properties in a .pdf file the image of the original annotation is not removed from the document when the altered annotation appears.  I would appreciate any possible leads on this issue.  
>   
> 
> 
> This addresses bug 177501.
>     http://bugs.kde.org/show_bug.cgi?id=177501
> 
> 
> Diffs
> -----
> 
>   core/annotations.h 72abdff 
>   core/annotations.cpp 49ab5bd 
>   core/document.h 1d825e1 
>   core/document.cpp 00e290d 
>   core/document_p.h 4a20561 
>   core/page.cpp 4df58e0 
>   generators/poppler/annots.cpp b7fb9f7 
>   part.rc 39c1571 
>   ui/annotationpropertiesdialog.cpp 4b02258 
>   ui/annotwindow.h f7df9f6 
>   ui/annotwindow.cpp c1bafb9 
>   ui/pageview.cpp 811a169 
> 
> 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.  The only known issue is the one described above when using .pdf files. 
> 
> 
> Thanks,
> 
> Jon Mease
> 
>

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