Review Request: This change will allow the user to see keyboard layout visually

Shaun Reich shaun.reich at kdemail.net
Sun Nov 4 23:33:17 GMT 2012 

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


i mostly went over formatting changes and the like...and i stopped halfway because the rest you can figure out i'm sure. basically, just abide by this: http://techbase.kde.org/Policies/Kdelibs_Coding_Style

which means use nice names for vars (no abbreviations), don't use getSomething(), capitalize each word except the first for vars/methods..etc.etc..


kcontrol/keyboard/kcm_add_layout_dialog.h
<http://git.reviewboard.kde.org/r/107202/#comment16638>

    we don't use getsomethings() and even if we did, it would be getSomething(). normally it'd be variant() but in this context we don't know what this is by the name.
    
    is there a better name for what it represents?
    
    and actually..why is it returning a string and accept a string called a variant? this confuses me.



kcontrol/keyboard/kcm_add_layout_dialog.h
<http://git.reviewboard.kde.org/r/107202/#comment16639>

    what's a layoutprev? layout preview? name it layoutPreview then, prev tends to mean previous anyways



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16640>

    remove that ;)



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16642>

    fix the indentation here (not your fault, but fix it anyways)



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16641>

    again, prevButton..should be previewButton if that is what it is. also, match the rest of the file when it comes to whitespace. if the rest of the file has whitespace in a connect after commas, do the same everywhere else



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16644>

    this whole function needs whitespace fixings between (){ and what not



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16645>

    please don't put this all on one line, it's horrible. split the inner stuff out into a dedicated statement..



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16643>

    remove next 2 dead lines



kcontrol/keyboard/kcm_add_layout_dialog.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16646>

    again, ws between operators e.g. = new vs =new



kcontrol/keyboard/kcm_keyboard_widget.h
<http://git.reviewboard.kde.org/r/107202/#comment16647>

    rm



kcontrol/keyboard/kcm_keyboard_widget.h
<http://git.reviewboard.kde.org/r/107202/#comment16648>

    rm



kcontrol/keyboard/kcm_keyboard_widget.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16649>

    not yours, but rm anyways. i'm going to stop mentioning this. just remove all code lines that are commented...that's what git is for.



kcontrol/keyboard/kcm_keyboard_widget.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16650>

    this line needs joined according to kdelibs style coding guide



kcontrol/keyboard/kcm_keyboard_widget.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16651>

    indent..



kcontrol/keyboard/preview/kbpreviewframe.h
<http://git.reviewboard.kde.org/r/107202/#comment16652>

    rm ws



kcontrol/keyboard/preview/kbpreviewframe.h
<http://git.reviewboard.kde.org/r/107202/#comment16653>

    ws



kcontrol/keyboard/preview/kbpreviewframe.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16654>

    don't abbreviate. what is a gr1 anyways? grid1?



kcontrol/keyboard/preview/kbpreviewframe.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16656>

    oh my, don't abbreviate that.. bkspszx? ew



kcontrol/keyboard/preview/kbpreviewframe.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16655>

    that's not going to be translatable..



kcontrol/keyboard/preview/kbpreviewframe.cpp
<http://git.reviewboard.kde.org/r/107202/#comment16657>

    again, untranslatable



kcontrol/keyboard/preview/keyaliases.h
<http://git.reviewboard.kde.org/r/107202/#comment16658>

    uh...public stuff should be at the top, private at the bottom generally. unless the original author had code like this



kcontrol/keyboard/preview/keyaliases.h
<http://git.reviewboard.kde.org/r/107202/#comment16659>

    again, no get



kcontrol/keyboard/preview/keyboardlayout.h
<http://git.reviewboard.kde.org/r/107202/#comment16661>

    ws



kcontrol/keyboard/preview/keyboardlayout.h
<http://git.reviewboard.kde.org/r/107202/#comment16660>

    ws


- Shaun Reich


On Nov. 4, 2012, 10:24 p.m., Andriy Rysin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107202/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2012, 10:24 p.m.)
> 
> 
> Review request for KDE Runtime and Shaun Reich.
> 
> 
> Description
> -------
> 
> Keyboard layout preview feature
> 
> 
> Diffs
> -----
> 
>   kcontrol/keyboard/CMakeLists.txt 29de717975d37949448a922c2297674a796a97c1 
>   kcontrol/keyboard/kcm_add_layout_dialog.h 6c6a795161d95d5203b5943ead6a7787e29ffd67 
>   kcontrol/keyboard/kcm_add_layout_dialog.cpp 34c97163d9267da34ad948347f7cc35b556fdaeb 
>   kcontrol/keyboard/kcm_add_layout_dialog.ui b73080d08d6281c4f67d079ab016a3df50821012 
>   kcontrol/keyboard/kcm_keyboard.ui 22968e8644cae700efe505fde437d332aa9514ed 
>   kcontrol/keyboard/kcm_keyboard_widget.h fa912c7f7daa497c20e3ce7ec4e40882d7d980af 
>   kcontrol/keyboard/kcm_keyboard_widget.cpp e1bd830bca8d308470dbba40e5e5a0127672798b 
>   kcontrol/keyboard/preview/KeyboardPainter.ui PRE-CREATION 
>   kcontrol/keyboard/preview/TODO PRE-CREATION 
>   kcontrol/keyboard/preview/kbpreviewframe.h PRE-CREATION 
>   kcontrol/keyboard/preview/kbpreviewframe.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keyaliases.h PRE-CREATION 
>   kcontrol/keyboard/preview/keyaliases.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keyboardlayout.h PRE-CREATION 
>   kcontrol/keyboard/preview/keyboardlayout.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keyboardpainter.h PRE-CREATION 
>   kcontrol/keyboard/preview/keyboardpainter.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keys.h PRE-CREATION 
>   kcontrol/keyboard/preview/keys.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keysym2ucs.h PRE-CREATION 
>   kcontrol/keyboard/preview/keysym2ucs.cpp PRE-CREATION 
>   kcontrol/keyboard/preview/keysymhelper.h PRE-CREATION 
>   kcontrol/keyboard/preview/keysymhelper.cpp PRE-CREATION 
>   kcontrol/keyboard/x11_helper.cpp fdadeeb851bac7511dc924a310c02f6f8a76bc84 
> 
> Diff: http://git.reviewboard.kde.org/r/107202/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Keyboard Layout Preview
>   http://git.reviewboard.kde.org/r/107202/s/813/
> 
> 
> Thanks,
> 
> Andriy Rysin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121104/030df6c6/attachment.htm>

More information about the kde-core-devel mailing list