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

[Jon Mease]

> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > Tried it looks pretty cool :) Some minor comments inline
> > 
> > You will need to rebase your patch, there's a small conflict with a change i did in document.cp

Thanks a lot for your feedback! 
These issues will be addressed in version 6 of the patch


> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > core/annotations.h, line 676
> > <http://git.reviewboard.kde.org/r/107442/diff/5/?file=111627#file111627line676>
> >
> >     Can you please try moving all these protected/private methods to the private classes instead of to the public ones?

Good idea, this actually cleaned up the design a bit


> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > core/document.h, line 405
> > <http://git.reviewboard.kde.org/r/107442/diff/5/?file=111629#file111629line405>
> >
> >     I'd prefer if we passed the newprops instead of the old ones in here so if you can fix editPageAnnotationContents to only need the new ones too, the three functions will be getting the "new data" we want to apply. Making it easier if someone (like the Okular Active guys) wants to use these functions

I modified this method to not require the old properties by adding a map to the DocumentPrivate class to keep track of the old properties.  The new properties are extracted from the annotation passed to the method.


> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > core/document.h, line 428
> > <http://git.reviewboard.kde.org/r/107442/diff/5/?file=111629#file111629line428>
> >
> >     Do we really need to pass new and old data here? Shouldn't we be able to get one of the two from annotation?

Modified this method to only input the new contents, cursor position, and anchor position by adding corresponding maps to the DocumentPrivate class to keep track of the old quantities.


> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > core/document.cpp, line 2248
> > <http://git.reviewboard.kde.org/r/107442/diff/5/?file=111630#file111630line2248>
> >
> >     You can just do emit m_docPriv->m_parent->annotationContentsBlaBla, no need for the intermediate function call

Thanks for the tip.  Looks like emitting another class's signal is equivalent to calling a protected method on the class so I needed to make EditAnnotationContentsCommand a friend of Document to make this work.


> On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote:
> > core/document.cpp, line 2126
> > <http://git.reviewboard.kde.org/r/107442/diff/5/?file=111630#file111630line2126>
> >
> >     nitpick, extra space at the end (or missing at the beginning :D not sure what is the file style)

Also did some general parenthesis spacing cleanup of this section


- Jon


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


On March 12, 2013, 1:24 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:24 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.  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/20130312/2bdf14ff/attachment-0001.html>