Review Request: spellchecking changes

Mon Dec 31 10:00:53 GMT 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108022/#review24301
-----------------------------------------------------------

Overall looks good (as far as I can tell).
Seems a bit odd that functionality provided by a plug-in (spelling) is painted in the underlying lib, but I guess there is no other way.

libs/kotext/KoTextBlockData.h
<http://git.reviewboard.kde.org/r/108022/#comment18605>

    Could you move that between the TabLineData stuff and the constr. Also a bit of text explaining what/why/how would be great (a bit like for the TabLineData). API doc can be a life saver later on.

libs/kotext/KoTextBlockData.h
<http://git.reviewboard.kde.org/r/108022/#comment18606>

    Also there some comments?

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18607>

    Shouldn't markupRanges.value(type) be used instead of markupRanges[type]?
    According to Qt's documentation, the [] operator silently inserts a item in the map if none exists.

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18609>

    idem

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18610>

    idem

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18611>

    idem

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18612>

    idem

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18613>

    idem

libs/kotext/KoTextBlockData.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18614>

    idem

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18615>

    Theoretically, you could still commit before the New Year ;)

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18616>

    Perhaps rephrase for a more generic sense (marking could be for other things than spell-checking AFAICS).
    Perhaps: Finally let's paint our own markings (eg. spelling,...)

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18617>

    why not painter->save/painter->restore?

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18618>

    Shouldn't there be a check if spelling plug-in is present/active?

    Perhaps insert a "TODO: make this configurable", so it is easier to find if/when we want to make it so.

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18619>

    This formula would look better with an explanation ;)

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18620>

    a () around the test would make it more legible.

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18621>

    I am not sure, but can't the logic be a bit improved here? It seems odd to assign x1 in the above line, and then check if the values in the MarkupRange are correct, eventually assigning x1 again.

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18622>

    Are we sure marIt->endX is always correct here?

libs/textlayout/KoTextLayoutArea_paint.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18623>

    Same as both issues above

libs/textlayout/KoTextLayoutRootAreaProvider.h
<http://git.reviewboard.kde.org/r/108022/#comment18624>

    Perhaps add a comment that the default implementation does nothing?
    And why not pure virtual like the others?

plugins/textediting/spellcheck/SpellCheck.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18625>

    Still valid?

plugins/textshape/TextTool.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18629>

    Has this anything to do with the spell-checking stuff, or is it a bug fix, which should be back-ported also?

plugins/textshape/TextTool.cpp
<http://git.reviewboard.kde.org/r/108022/#comment18628>

    Forgotten?

- Pierre Stirnweiss

On Dec. 30, 2012, 1:51 p.m., C. Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108022/
> -----------------------------------------------------------
> 
> (Updated Dec. 30, 2012, 1:51 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Change the way we draw markings of speeling errors. Instead of using the qt way we draw it ourselve
> This has the following benefits:
>   - we don't have to relayout after finding spelling errors
>   - we can have more than one type of marking (spelling, grammar, etc)
> 
> This commit also changes the way we ask for spellchecking to be performed. The old way had some
> shortcommings that could lead to text not being checked at all
> 
> 
> Diffs
> -----
> 
>   libs/kotext/KoTextBlockData.h 05db499 
>   libs/kotext/KoTextBlockData.cpp b1d7d5c 
>   libs/kotext/KoTextEditingPlugin.h 7b7b9dd 
>   libs/textlayout/KoTextLayoutArea.h 94c52ec 
>   libs/textlayout/KoTextLayoutArea_paint.cpp 2fc2131 
>   libs/textlayout/KoTextLayoutRootAreaProvider.h d8c0614 
>   libs/textlayout/KoTextLayoutRootAreaProvider.cpp 7d27944 
>   plugins/textediting/autocorrection/Autocorrect.h 2a84506 
>   plugins/textediting/autocorrection/Autocorrect.cpp e136216 
>   plugins/textediting/changecase/Changecase.h 8433be1 
>   plugins/textediting/changecase/Changecase.cpp 936957c 
>   plugins/textediting/spellcheck/CMakeLists.txt a4dedef 
>   plugins/textediting/spellcheck/SpellCheck.h ee2f7bf 
>   plugins/textediting/spellcheck/SpellCheck.cpp 4da7e7d 
>   plugins/textediting/thesaurus/Thesaurus.h c5de5b0 
>   plugins/textediting/thesaurus/Thesaurus.cpp b10a69d 
>   plugins/textshape/SimpleRootAreaProvider.h 8daf2f8 
>   plugins/textshape/SimpleRootAreaProvider.cpp 1f3b0fe 
>   plugins/textshape/TextTool.h a8a525d 
>   plugins/textshape/TextTool.cpp 148806c 
>   words/part/KWRootAreaProvider.h 1908b11 
>   words/part/KWRootAreaProvider.cpp 9e28045 
> 
> Diff: http://git.reviewboard.kde.org/r/108022/diff/
> 
> 
> Testing
> -------
> 
> played around with it
> 
> 
> Thanks,
> 
> C. Boemann
> 
>

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