> 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 > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
