[Okular-devel] Review Request: table selection tool - new feature

[Jiri Baum]

> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > part.rc, line 71
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32142#file32142line71>
> >
> >     Please do not write tbl, use table

OK


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.h, line 60
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32143#file32143line60>
> >
> >     Same here

OK


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.h, line 159
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32143#file32143line159>
> >
> >     selectionRect should be const here, and by having a quick look at the function seems the function itself may be const too?
> >     
> >     Also please do not pass int b, use a proper enum for that

OK, done. Sorry about that int b, didn't want to add #includes to the headers without knowing what the dependency graph looks like...


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.h, line 219
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32143#file32143line219>
> >
> >     Same here

Done.


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2921
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line2921>
> >
> >     No two methods in the same line please

OK.


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 115
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line115>
> >
> >     no capital as first letter, tbl -> table and probably better if you add "selection" somewhere in the variable name

Done.


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 168
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line168>
> >
> >     tbl->table

Done.


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2229
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line2229>
> >
> >     You need to use if ( d->document->isAllowed( Okular::AllowCopy ) ) somewhere here

Yes, I use it on line 2223.


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 1772
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line1772>
> >
> >     Nitpicking (as i know almost none of our code does it) but it would be cool if you added const to the variables you know won't change (like selectionRect), makes it easier to read for the new person reading it

Added here and to the six ints where I'm calculating distances from edges...


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 1258
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line1258>
> >
> >     Have not tried, but does this survive zooming in/out of the document?

Ah, no it doesn't. Neither does the selection tool, from which I was copying, but I guess zooming in/out while holding down the mouse button is not a common use case.

How would you recommend I fix this, please? I'm not particularly familiar with the internals of okular, so I've been mostly copying from the Selection Tool, and it has the same behaviour...


> On Aug. 29, 2011, 9:28 a.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2805
> > <http://git.reviewboard.kde.org/r/102358/diff/1/?file=32144#file32144line2805>
> >
> >     spacing is a bit odd here, either remove the first space or add one after bb

Ah, space added.


- Jiri


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


On Aug. 18, 2011, 5:34 a.m., Jiri Baum wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102358/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2011, 5:34 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Summary
> -------
> 
> Table selection tool, as per the bug description.
> 
> Usage: from the menu, select Tools | Table Selection Tool (or use the shortcut Ctrl-5). Use the mouse to select the whole table (in a manner similar to the existing Selection Tool), then click near the edges of the table to add and remove row and column dividers; the table is automatically copied to the clipboard after each change. When ready, paste into another document (spreadsheet, word processor, etc). Press Esc to clear the selection.
> 
> The patch also fixes handling of Esc key, so that it's not consumed by the closeFindBar KAction when the FindBar is closed. Previously this bug was non-obvious since the rectangular selection tool can in any case be cancelled by releasing the mouse button, then pressing Esc; cancelling it without releasing the mouse button was presumably not a common use-case.
> 
> 
> This addresses bug 279859.
>     http://bugs.kde.org/show_bug.cgi?id=279859
> 
> 
> Diffs
> -----
> 
>   AUTHORS 55b672e 
>   aboutdata.h f9c5896 
>   part.h a36031b 
>   part.cpp e6b80a5 
>   part.rc 928168d 
>   ui/pageview.h cd88b99 
>   ui/pageview.cpp 25da571 
> 
> Diff: http://git.reviewboard.kde.org/r/102358/diff
> 
> 
> Testing
> -------
> 
> It works for me, even under demo conditions...
> 
> 
> Thanks,
> 
> Jiri
> 
>

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