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

[Casper Boemann]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110407/023b8603/attachment.htm>