----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101045/#review2453 -----------------------------------------------------------
Hi it looks nicely coded. Just a few small issues listed below. I've not actually tried to build aand use it One question though: Don't KDE have some find toolbar already we could have resued. Okay it keeps us free of relying on KDE but on the other hand this is ui. As far as I'm concerned it can go in, but I'm not pressing ship it since i'v not actually tried it. libs/main/KoFindBase.h <http://git.reviewboard.kde.org/r/101045/#comment2113> No abbriavations please libs/main/KoFindBase.h <http://git.reviewboard.kde.org/r/101045/#comment2114> No abbriavations please libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2115> { should go to end of line above libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2116> shouldn't this do something or be abstract ? libs/main/KoFindText.cpp <http://git.reviewboard.kde.org/r/101045/#comment2117> remember { } libs/main/KoFindText.cpp <http://git.reviewboard.kde.org/r/101045/#comment2118> remember { } libs/main/KoFindToolbar.cpp <http://git.reviewboard.kde.org/r/101045/#comment2119> What, no ui file but hardcoding ?? oh well. - Casper On April 7, 2011, 12:11 p.m., Arjen Hiemstra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101045/ > ----------------------------------------------------------- > > (Updated April 7, 2011, 12:11 p.m.) > > > Review request for Calligra, Marijn Kruisselbrink, Boudewijn Rempt, Thorsten > Zachmann, and Casper Boemann. > > > Summary > ------- > > This is the diff of the branch libs-kofind_refactor-ahiemstra and master. It > encompasses the entire refactor of KoFind to a more generic structure that > can be used by different interfaces and by different document types. > > I consider this to be done API-wise, though several features are currently > missing. This review therefore is mostly a request for comments about the > current API. > > Still to be done feature-wise: > - Add support for more options (search in selection, search from cursor, > regex, etc) > - Properly implement replacing, including implementing an interface for it. > - Add support for searching through multiple text shapes to KoFindText. > - Implement a searching backend for tables. > > > Diffs > ----- > > libs/kotext/KoTextDocument.h 6d755f8 > libs/kotext/KoTextDocument.cpp 1517953 > libs/main/CMakeLists.txt a7f3077 > libs/main/KoFindBase.h PRE-CREATION > libs/main/KoFindBase.cpp PRE-CREATION > libs/main/KoFindMatch.h PRE-CREATION > libs/main/KoFindMatch.cpp PRE-CREATION > libs/main/KoFindOption.h PRE-CREATION > libs/main/KoFindOption.cpp PRE-CREATION > libs/main/KoFindOptionSet.h PRE-CREATION > libs/main/KoFindOptionSet.cpp PRE-CREATION > libs/main/KoFindText.h PRE-CREATION > libs/main/KoFindText.cpp PRE-CREATION > libs/main/KoFindToolbar.h PRE-CREATION > libs/main/KoFindToolbar.cpp PRE-CREATION > libs/main/tests/CMakeLists.txt 6f13036 > libs/main/tests/testfindmatch.h PRE-CREATION > libs/main/tests/testfindmatch.cpp PRE-CREATION > plugins/textshape/TextShape.cpp 1c8f2c5 > words/part/KWView.h a8eec16 > words/part/KWView.cpp 539e6b1 > > Diff: http://git.reviewboard.kde.org/r/101045/diff > > > Testing > ------- > > I have implemented a toolbar interface for Words, which is enabled in this > review. It works very well, though it does not yet feature all the options > the current dialog has. Furthermore, I have implemented unit tests for > KoFindMatch, the most important class in this code, though arguably at least > KoFindOption/KoFindOptionSet also could do with some unit testing. > > > Thanks, > > Arjen > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
