[Okular-devel] Review Request: table selection tool - semi-automatic dividers

[Albert Astals Cid]

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



core/page.h
<http://git.reviewboard.kde.org/r/102788/#comment6311>

    You should document here that the ownership of the list contents belongs to the caller and he is the one that has to take care of freeing the memory



core/page.cpp
<http://git.reviewboard.kde.org/r/102788/#comment6312>

    You are leaking lots of memory here from the old list you never free properly



core/textpage.h
<http://git.reviewboard.kde.org/r/102788/#comment6313>

    Same, you should document here that the ownership is transferred to the caller and it is the caller that has to free the memory



ui/pageview.h
<http://git.reviewboard.kde.org/r/102788/#comment6315>

    Do not add a bool here, otherwise one can get things like
    selectionClear(false)
    that are a bit ugly. Please add an enum 
    ClearAllSelection
    ClearOnlyDividers
    or something like that



ui/pageview.h
<http://git.reviewboard.kde.org/r/102788/#comment6314>

    use () instead of (void)



ui/pageview.cpp
<http://git.reviewboard.kde.org/r/102788/#comment6316>

    Use foreach here



ui/pageview.cpp
<http://git.reviewboard.kde.org/r/102788/#comment6317>

    words returns memory that was new'ed and you never delete it


- Albert Astals Cid


On Oct. 6, 2011, 3:41 a.m., Jiri Baum wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102788/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2011, 3:41 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> Automatic row and column dividers for the table selection tool, as per the bug description.
> 
> The table selection tool as per bug #279859 requires the user to manually indicate all the column and row dividers. However, in many situations it's trivial for the tool to detect the rows and columns automatically. It can't get it right every time, but it can get it right often and some of the remaining times it can at least get close. As long as the automatically-detected column and row dividers are easy to clear, they will save more time than they will cost.
> 
> Dividers are placed in any place where they will not intersect any letter and the gap is not zero-width. A special case is made for selections spanning multiple pages to avoid placing dividers that cannot be seen (instead of a divider in the gap between pages, two dividers are placed, one each side).
> 
> 
> This addresses bug 283440.
>     http://bugs.kde.org/show_bug.cgi?id=283440
> 
> 
> Diffs
> -----
> 
>   core/page.h 67bcf97 
>   core/page.cpp eaab64e 
>   core/textpage.h 2bdc09d 
>   core/textpage.cpp 7096e90 
>   ui/pageview.h cd88b99 
>   ui/pageview.cpp 25da571 
> 
> Diff: http://git.reviewboard.kde.org/r/102788/diff/diff
> 
> 
> Testing
> -------
> 
> Works for me...
> 
> 
> Thanks,
> 
> Jiri Baum
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20111012/ffd31635/attachment-0001.html>