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

[Fabio D'Urso]

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


Hi :)

It seems that the patch doesn't apply any more and needs to be rebased.
I've tried to reconstruct it and have a quick look, but I couldn't test it as it doesn't compile.
Some comments in random order:

- DocumentPrivate::perform{Add|Remove}PageAnnotations method are not used anywhere
- You've removed Document::removePageAnnotations. This needs to be discussed with Albert as it breaks the API. This patch changes the signature of other public methods too (this is just a reminder for a later discussion)
- You can't do the Annotation::m_textEdit thing because the Annotation class is part of okularcore and cannot contain stuff that belongs to the UI (which lives in okularpart). [remember that okularcore is a library that can be used by different clients, not by okularpart only]
- Document::updateUndoRedoAvailable() may signal "spurious" changes (for example if you undo twice there's no need to emit canRedoChanged(true) the second time).
  Maybe you can just connect signal-to-signal and let Qt do the rest, something like:
   connect( d->undoStack, SIGNAL(canUndoChanged(bool)), this, SIGNAL(canUndoChanged(bool)))
- Do we really need these new public methods? Keep in mind that once we add them they become part of the API/ABI, so it's better to avoid them at first if possible. In general, auxiliary methods that are not meant to be called outside okularcore should be put in the various ***Private classes (I'm referring
  to the three methods in Annotation and the m_textEdit member)
- Why Annotation::getNewPrivateAnnotation? Wasn't "new BlablablaAnnotationPrivate()" ok? Please also note that calling virtual methods within the constructor of the same object is generally not recommended because the class hasn't been fully initialized yet.
- In annotations_p.h you've forward-declared classes QTextEdit and QMatrix but I don't see them being used. If you want to use QMatrix, please have a look at commit aa6ed8afc0f595db1d372b81dc8db2593e7055e1
- kDebug(4700) << "new edit contents";   →   kDebug(OkularDebug) << "new edit contents";
- In document.h you're referencing class AnnotWindow. You can't, because that's part of okularpart.
- Do the *AnnotationCommand classes need to be exported? If not, please remove the OKULAR_EXPORT tag and move them to some not-exported blabla_p.h file (such as document_p.h or a dedicated undocommands_p.h)
- I don't understand "// setup actions shouldn't be undoable \ m_doc->undoStack->clear();" in page.cpp. Can you explain it please? Thank you

Undo/redo support is one of the most requested features about annotations, so thank you very much for your work!

P.S.: About the issue with PDF files, you have to be careful with them as they are "special": in non-PDF files annotations are entirely handled by Okular (rendering included), wheres in PDF files it's Poppler who draws them and saves them to PDF. (Poppler is the library we use to render and save PDF files)
When you use annotations in PDF files you need to make sure that poppler stays informed about them via the AnnotationProxy interface (so that it knows both what to draw and what to save if the user presses Save As), and you also need to preserve the Annotation::ExternallyDrawn flag, which basically tells Okular's PagePainter class not to redraw annotations above the ones drawn by Poppler. Note that this is just a hint, I haven't checked if you are already doing this.


- Fabio D'Urso


On Nov. 24, 2012, 3:09 a.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
> 
> (Updated Nov. 24, 2012, 3:09 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/annotations_p.h 221572d 
>   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 bf8b546 
> 
> 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/20121128/464b4265/attachment.html>