[Okular-devel] Review Request 110391: Fix for bug 319442

[Fabio D'Urso]

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


Hi Jon! The patch looks good to me. Here are a few minor notes


core/annotations.h
<http://git.reviewboard.kde.org/r/110391/#comment24127>

    Seems you forgot to remove setInplaceText() and inplaceText() here



core/annotations.cpp
<http://git.reviewboard.kde.org/r/110391/#comment24128>

    I've researched a bit if window's text attribute has ever ever actually been used. Turns out it has always been empty. These are the occurrences I've found and checked:
     - Poppler always leaves it empty (checked poppler 0.18 too, i.e. before I refactored qt4 annotation code)
     - PickPointEngine::end() sets an empty string on new annotations
     - The if condition in DocumentPrivate::performSetAnnotationContents always fails because window text is already empty
    
    It is therefore safe to pretend it has never really existed and just remove it, to minimize the number of "phantom" attributes that hunt us from the past :D



core/annotations.cpp
<http://git.reviewboard.kde.org/r/110391/#comment24129>

    Sub-Node-2 is now the first and only one :D



generators/poppler/annots.cpp
<http://git.reviewboard.kde.org/r/110391/#comment24130>

    This special handling is now useless, because we've already done the very same thing a few lines above



ui/pageviewannotator.cpp
<http://git.reviewboard.kde.org/r/110391/#comment24131>

    No need to preserve this instruction, contents is already empty by default


- Fabio D'Urso


On May 11, 2013, 7:48 p.m., Jon Mease wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110391/
> -----------------------------------------------------------
> 
> (Updated May 11, 2013, 7:48 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> Fixed bug 319442 by removing the annotation's inplaceText and window text attributes and replacing them both with the unified usage of the contents attribute.  See discussion with Fabio in bug report and https://git.reviewboard.kde.org/r/107442/.  The inplaceText and window text properties will still be read from saved XML files for backwards compatibility, however they are now used to set the annotation's contents property.
> 
> 
> This addresses bug 319442.
>     http://bugs.kde.org/show_bug.cgi?id=319442
> 
> 
> Diffs
> -----
> 
>   core/annotations.h 4e9082e 
>   core/annotations.cpp 72ca8b5 
>   core/document.cpp 2732441 
>   generators/djvu/generator_djvu.cpp bc83ed7 
>   generators/poppler/annots.cpp b7fb9f7 
>   ui/pagepainter.cpp 950be03 
>   ui/pageviewannotator.cpp 4615d1c 
> 
> Diff: http://git.reviewboard.kde.org/r/110391/diff/
> 
> 
> Testing
> -------
> 
> Tested creating inline note annotations and the bug no longer occurs.
> 
> 
> Thanks,
> 
> Jon Mease
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130513/1e5f4d6f/attachment.html>