Review Request: Refactor of KoFind to a general interface for searching

[Thorsten Zachmann]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110408/3dc3a4cb/attachment.htm>