Review Request: spellchecking changes

C. Boemann cbr at boemann.dk
Mon Dec 31 12:02:55 GMT 2012 


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/KoTextBlockData.cpp, line 87
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103020#file103020line87>
> >
> >     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.

Well in most cases (like) here i need a &T so i can modify it. However value() gives me a const T


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutArea_paint.cpp, line 752
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103023#file103023line752>
> >
> >     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,...)

both yes and no, since this is drawig for the specialized case of spellings. other kinds of markings might be drawn in a completely different way


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutArea_paint.cpp, line 754
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103023#file103023line754>
> >
> >     why not painter->save/painter->restore?

 Because this is faster. save/restore does the same thing but for every setting in the painter. Possibly this is done in a slower way than save/resotre can do it but overall i'm fairly sure this is faster. And a lot of internal qt code makes a similar trade when few things needs to be saved and restored


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutArea_paint.cpp, lines 756-760
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103023#file103023line756>
> >
> >     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.

i guess but this is controlled from the plugin


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutArea_paint.cpp, line 783
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103023#file103023line783>
> >
> >     Are we sure marIt->endX is always correct here?

I'm sure :)


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > plugins/textediting/spellcheck/SpellCheck.cpp, line 127
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103032#file103032line127>
> >
> >     Still valid?

yes still valid - i've not touched this part - but not sure it a relevant idea. it makes it less responsive


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > plugins/textshape/TextTool.cpp, line 1633
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103038#file103038line1633>
> >
> >     Has this anything to do with the spell-checking stuff, or is it a bug fix, which should be back-ported also?
> >

Yes it's a fix and should probably be backported, though i'm not sure it will not have regressions without the rest


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutRootAreaProvider.h, line 58
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103024#file103024line58>
> >
> >     Perhaps add a comment that the default implementation does nothing?
> >     And why not pure virtual like the others?

I've made it pure virtual now, so no comment added


> On Dec. 31, 2012, 10 a.m., Pierre Stirnweiss wrote:
> > libs/textlayout/KoTextLayoutArea_paint.cpp, lines 776-781
> > <http://git.reviewboard.kde.org/r/108022/diff/1/?file=103023#file103023line776>
> >
> >     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.

I've restructed it a little bit


- C.


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


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/a26f76c8/attachment.htm>

More information about the calligra-devel mailing list