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

Thu Mar 7 15:59:39 UTC 2013

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


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


core/annotations.h
<http://git.reviewboard.kde.org/r/107442/#comment21489>

    Can you please try moving all these protected/private methods to the private classes instead of to the public ones?


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

    We should modify this since


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

    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


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

    Add const & to the delta


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

    Do we really need to pass new and old data here? Shouldn't we be able to get one of the two from annotation?


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

    Add the @since markers


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

    Can you try to move this to the private class and not the public one? Not sure it makes sense exposing this to the external world, ah, actually you don't need it, you can just emit from the child class, will comment in there


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

    Add some docu here, not because they are not obvious but to have the @since marker


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

    @since missing


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

    const & for newContents


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

    split it into 3 lines, no need to save 2 lines in a 4000+ line file :D
    
    Same for the other one
    
    the text is missing i18n so it is translatable, probably i18nc to give a nice context to the translators


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

    const & for delta


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

    kill the "this->"


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

    nitpick, extra space at the end (or missing at the beginning :D not sure what is the file style)


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

    const & for the strings


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

    You can just do emit m_docPriv->m_parent->annotationContentsBlaBla, no need for the intermediate function call


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

    You don't really need this delete, you already gave this as parent when creating the stack so deleting the document will autodelete the stack, it doesn't hurt either of course


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

    we probably don't need this


ui/annotwindow.cpp
<http://git.reviewboard.kde.org/r/107442/#comment21506>

    You should use menu and not this as parent of the actions so they are deleted as soon as they are not needed


- Albert Astals Cid


On Feb. 2, 2013, 6:58 p.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2013, 6:58 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
> -----
> 
>   ui/pageview.cpp 60a273d 
>   ui/annotwindow.cpp c1bafb9 
>   ui/annotwindow.h f7df9f6 
>   part.rc 39c1571 
>   ui/annotationpropertiesdialog.cpp 4b02258 
>   core/page.cpp 4df58e0 
>   core/document.cpp 372af56 
>   core/document_p.h 57a3bee 
>   core/document.h 1d825e1 
>   core/annotations.h 72abdff 
>   core/annotations.cpp 49ab5bd 
> 
> 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/20130307/26e4443e/attachment-0001.html>