----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101045/#review2490 -----------------------------------------------------------
Looks very nice. I mostly comments about code style. If it contains the same functionality as the current dialog I think you can already commit. Otherwise please bring it first on par with the current dialog. Will you also update stage and tables to use the new dialog? libs/main/KoFindBase.h <http://git.reviewboard.kde.org/r/101045/#comment2161> Please remove the empty spaces libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2164> Object* parent should be Object *parent. That is also true for the usage of & libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2162> d needs to be deleted here libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2163> There should be a blank between if and (. This should be fixed everywhere in the patch. libs/main/KoFindBase.cpp <http://git.reviewboard.kde.org/r/101045/#comment2166> How about simplifying that to d->currentMatch = (d->currentMatch + 1) % d->matches.count(); emitMatchFound(d->matches.at(c->currentMatch())); if (currentMatch == 0) { emit wrapAround(true); } libs/main/KoFindMatch.cpp <http://git.reviewboard.kde.org/r/101045/#comment2170> d needs to be deleted here libs/main/KoFindOptionSet.h <http://git.reviewboard.kde.org/r/101045/#comment2167> There should be no blanks after < and before > libs/main/KoFindOptionSet.cpp <http://git.reviewboard.kde.org/r/101045/#comment2171> d needs to be deleted here libs/main/KoFindText.h <http://git.reviewboard.kde.org/r/101045/#comment2168> Are those needed. If not please remove libs/main/KoFindText.cpp <http://git.reviewboard.kde.org/r/101045/#comment2169> d needs to be deleted here words/part/KWView.cpp <http://git.reviewboard.kde.org/r/101045/#comment2172> I had to remove this line to make it compile - Thorsten 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
