Review Request 124282: Implement Voikko based spellchecker for Sonnet

Tue Jul 7 19:07:18 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82192
-----------------------------------------------------------


src/plugins/voikko/voikkoclient.cpp (line 28)
<https://git.reviewboard.kde.org/r/124282/#comment56578>

    here you can just use the Q_FUNC_INFO macro to get the same information (the function signature).


src/plugins/voikko/voikkoclient.cpp (line 30)
<https://git.reviewboard.kde.org/r/124282/#comment56579>

    plural is dictionaries


src/plugins/voikko/voikkoclient.cpp (line 32)
<https://git.reviewboard.kde.org/r/124282/#comment56580>

    I prefer to error out early («if (!dictionaries) return;»), less indentation and state to remember.


src/plugins/voikko/voikkodict.h (line 64)
<https://git.reviewboard.kde.org/r/124282/#comment56585>

    prefix all private members with m_


src/plugins/voikko/voikkodict.cpp (line 47)
<https://git.reviewboard.kde.org/r/124282/#comment56581>

    ALL_UPPERCASE usually means defines (because they have slightly different behavior from other variables).


src/plugins/voikko/voikkodict.cpp (line 97)
<https://git.reviewboard.kde.org/r/124282/#comment56582>

    always use braces {}


src/plugins/voikko/voikkodict.cpp (line 108)
<https://git.reviewboard.kde.org/r/124282/#comment56584>

    just return true here, and you don't need the else block.


src/plugins/voikko/voikkodict.cpp (line 125)
<https://git.reviewboard.kde.org/r/124282/#comment56586>

    if (replacements.contains(word)) {
            suggestions.append(word);
        }
        
    is much more readable


src/plugins/voikko/voikkodict.cpp (line 130)
<https://git.reviewboard.kde.org/r/124282/#comment56588>

    can't you free the string again here? then you can do an early return if wcharSuggestions is empty.
    
    (I'd also prefer avoiding hungarian notiation  -- typeVariablename.)


src/plugins/voikko/voikkodict.cpp (line 140)
<https://git.reviewboard.kde.org/r/124282/#comment56587>

    isn't this the wrong delete? It is declared as a pointer, not an array.


src/plugins/voikko/voikkodict.cpp (line 172)
<https://git.reviewboard.kde.org/r/124282/#comment56590>

    This is not very elegant.
    
    A better solution might be to split this up into two functions (fetchPersonal() that returns languageNode, and storePersonal() that takes a languageNode()), maybe?


src/plugins/voikko/voikkodict.cpp (line 180)
<https://git.reviewboard.kde.org/r/124282/#comment56589>

    error out early instead.
    
    also, QIODevice::Truncate? That will delete everything already in the file.


src/plugins/voikko/voikkodict.cpp (line 236)
<https://git.reviewboard.kde.org/r/124282/#comment56591>

    error out early


- Martin Tobias Holmedahl Sandsmark


On July 7, 2015, 2:44 p.m., Jesse Jaara wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124282/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 2:44 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: sonnet
> 
> 
> Description
> -------
> 
> # Implement Voikko based spellchecker for Sonnet
> 
> ## Description
> Implements a spell chekcing plugin based on libvoikko <http://voikko.puimula.org/>.
> Primarily for supporting highquality Finnishs spell checking, but HFST trancuders
> can be found several other languages too.
> <http://sourceforge.net/projects/hfst/files/resources/spell-transducers/>
> 
> 
> ## List of commits (oldest 1st)
> ---------------------------------------------------------------------------------------------------
> 
> Define QLoggingCategory for for voikko speller plugin
> 
> * Declared SONNET_VOIKKO QLoggingCategory
> 
> --------------------------------------------------------------------------------------------------
> 
> Implement Voikko based spellchecker (dictionary)
> 
> * All Sonnet::SpellerPlugin functions are implemented.
>    * storeReplacement() and addToPersonal() use Json based storage.
>     * File location:
>         * UNIX & OSX: QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
>         * Windows >= Vista: QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
>         * XP: QSP::GenericDataLocation/../../Aplication Data/Sonnet/Voikko-user-dictionary.json
>     * Format:
> ```JSON
> { "<languageId>": {
>     "PersonalWords": [
>         "word"
>     ],
>     "Replacements": [
>         {"bad": "eror",
>          "good": "error"}
>     ]
> }
> ```
> * Before use VoikkoDict based chekkers must be ensured to be with valid with initFailed().
>   As so the ctor is protected and only accessible from friens class VoikkoClient, which
>   does this check before returning the speller. Using an invalid speller will result in
>   null-pointer exceptions.
> 
> --------------------------------------------------------------------------------------------------
> 
> Implement Sonnet::Client for Voikko speller
> 
> * Reliability set to 50.
>   Voikko is primarily only used for Finnish at the moment, although
>   the HFST transducer-backend has added support for other languages
>   of varying quality.
>   As for Finnish (99% of use cases) the results are top quality.
> 
>   In any case the reliability should be higher than that of hunspell
>   and aspell to prevent them from kicking in for Finnish, as the
>   Finnish dictionarys for them are low-quality.
> 
> * Name is "Voikko"
> 
> --------------------------------------------------------------------------------------------------
> 
> Add in CMakeBits needed to compile Voikko speller.
> 
> * Added FindVOIKKO module
> 
> 
> Diffs
> -----
> 
>   cmake/FindVOIKKO.cmake PRE-CREATION 
>   src/plugins/CMakeLists.txt 3d24d61 
>   src/plugins/voikko/CMakeLists.txt PRE-CREATION 
>   src/plugins/voikko/voikkoclient.h PRE-CREATION 
>   src/plugins/voikko/voikkoclient.cpp PRE-CREATION 
>   src/plugins/voikko/voikkodebug.h PRE-CREATION 
>   src/plugins/voikko/voikkodebug.cpp PRE-CREATION 
>   src/plugins/voikko/voikkodict.h PRE-CREATION 
>   src/plugins/voikko/voikkodict.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124282/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jesse Jaara
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150707/246b3b89/attachment-0001.html>