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

[Jon Mease]

On Dec. 27, 2012, 3:22 p.m., Jon Mease wrote:
> > 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 :)
> >

Thanks for the feedback.  Yeah, I agree that the current AnnotWindow approach isn't fully satisfactory.  The issue, like you observed, is that we can't get full access to the QTextDocument's undo stack.  The only information we have is from the undoCommandAdded() signal.  So I agree that we'll have to disable our KTextEdit's undo stack and implement something from scratch.  On the bright side this will solve the Core/GUI dependency issue :-)  

For my next version I think I'll try to implement a keystroke-by-keystroke undo/redo capability that properly restores cursor position on undo/redo.  Once this is working I'll work out the logic for merging consecutive edits together appropriately.


- Jon


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


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/20121228/13cceff0/attachment-0001.html>