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

Thu Dec 27 15:22:16 UTC 2012

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

core/annotations.cpp
<http://git.reviewboard.kde.org/r/107442/#comment18342>

    I'm not sure it is legal to keep a QDomNode around after its owner QDomDocument has been destroyed. I remember doing some research when I wrote commit 86c92ffec26b0a8e9f6e40767df0a57743224279 and I came to the conclusion it isn't. But let's wait to hear from someone more knowledgeable.

core/document.h
<http://git.reviewboard.kde.org/r/107442/#comment18343>

    Seems you don't actually use these three classes in document.h

core/document.h
<http://git.reviewboard.kde.org/r/107442/#comment18344>

    Wondering if "newProps" is redundant. I mean, isn't newProps always the same as annotation->getAnnotationPropertiesDomNode() ?

core/document.h
<http://git.reviewboard.kde.org/r/107442/#comment18345>

    Please document what completeDrag is for

core/document.h
<http://git.reviewboard.kde.org/r/107442/#comment18346>

    Please document textEdit and commit too

core/document.h
<http://git.reviewboard.kde.org/r/107442/#comment18347>

    s/undo/redo/

core/document.cpp
<http://git.reviewboard.kde.org/r/107442/#comment18348>

    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)

core/document.cpp
<http://git.reviewboard.kde.org/r/107442/#comment18349>

    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.

core/page.cpp
<http://git.reviewboard.kde.org/r/107442/#comment18354>

    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?

part.rc
<http://git.reviewboard.kde.org/r/107442/#comment18339>

    Please increase version #

part.rc
<http://git.reviewboard.kde.org/r/107442/#comment18340>

    In other programs (eg kwrite) I see that undo/redo are usually placed before copy/select stuff

Sorry for the long delay, I've started to have a look at this new version. Apart from the minor issues I've noted, most code looks good to me.

I'm not fully convinced by AnnotWindow changes, I've tested the code a bit and I've found that popup text changes easily go out of sync. For example:
 1) Create ink annotation
 2) Set its popup text to "foo"
 3) Create ellipse annotation
 4) Append "bar" to ink annotation's text
 5) Edit -> Undo: the ellipse is deleted, instead of reverting "foobar" to "foo"
Or another example:
 1) Create annotation
 2) Set some popup text
 3) Right click on the popup and press Undo from the context menu
 4) Notice that the Document hasn't noticed that we've undone our text change (no redo is available, pressing undo doesn't delete the annotation)
Seems like we're missing some editing events from QTextEdit. I've had a look at the Qt manual and unfortunately there seems to be no way to access QTextEdit's internal undo stack.
I'm afraid we'll have to disable QtextEdit's internal undo stack and reinvent the wheel to handle QTextEdit text changes ourselves.

About the other issue with Annotation holding references to QTextEdit, maybe we can keep track of each annotation's QTextEdit in UI code using a QHash<Anntation*,QTextEdit*> or something. Actually, there seems to be already an interesting QHash< Okular::Annotation *, AnnotWindow * > m_annowindows field in class PageViewPrivate (pageview.cpp).

Let me know what you think about it, and sorry again :)

- Fabio D'Urso

On Dec. 1, 2012, 9:33 p.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2012, 9:33 p.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 3e4e21a 
>   core/document_p.h 4a20561 
>   core/page.cpp 4df58e0 
>   part.rc 39c1571 
>   ui/annotationpropertiesdialog.cpp 4b02258 
>   ui/annotwindow.h f7df9f6 
>   ui/annotwindow.cpp c1bafb9 
>   ui/pageview.cpp e5ec39b 
> 
> 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/20121227/9b25bcbb/attachment-0001.html>