Review Request 110260: Plugin to set status "Away" when screen gets locked.

[Martin Klapetek]

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


Very nice, thanks a lot for this! Couple comments below :)


config/telepathy-kded-config.cpp
<http://git.reviewboard.kde.org/r/110260/#comment23750>

    I wonder if we should cache this i18n string so the translators don't have to translate it 3x (not your fault, I'll probably do it in a separate patch).



config/telepathy-kded-config.ui
<http://git.reviewboard.kde.org/r/110260/#comment23748>

    Is this needed?



config/telepathy-kded-config.ui
<http://git.reviewboard.kde.org/r/110260/#comment23749>

    I don't think we need another groupbox, especially for a single option; put it under the "Auto away" groupbox
    
    I'd suggest to put another horizontal line below the "Not available" in autoaway and put single checkbox there "[] Set my status to away when the screen is locked" and the input box below



lockingscreenaway.h
<http://git.reviewboard.kde.org/r/110260/#comment23743>

    Needs updating



lockingscreenaway.h
<http://git.reviewboard.kde.org/r/110260/#comment23742>

    I didn't write this :) Add yourself here (you can leave me as the original author, but basically it's your code)



lockingscreenaway.cpp
<http://git.reviewboard.kde.org/r/110260/#comment23744>

    The coding style in our code puts the pointer sign to the variable name, ie. QObject *parent



lockingscreenaway.cpp
<http://git.reviewboard.kde.org/r/110260/#comment23745>

    I think more proper name would be screen-lock-away, probably same for the class name ("Locking screen" sounds weird)



lockingscreenaway.cpp
<http://git.reviewboard.kde.org/r/110260/#comment23746>

    Coding style of if blocks is
    
    if (cond) {
    
    } else {
    
    }



lockingscreenaway.cpp
<http://git.reviewboard.kde.org/r/110260/#comment23747>

    Remove if not needed


- Martin Klapetek


On May 1, 2013, 8:37 p.m., Lucas Betschart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110260/
> -----------------------------------------------------------
> 
> (Updated May 1, 2013, 8:37 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Plugin to set status "Away" when screen gets locked.
> Forked from autoaway.h/cpp.
> 
> 
> This addresses bug https://bugs.kde.org/show_bug.cgi?id=290998.
>     http://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=290998
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt a339495773875954b668875a28c074f7ac7f733f 
>   config/telepathy-kded-config.h b1bf626e0fbb9fc0705d95c9d190be5023ace876 
>   config/telepathy-kded-config.cpp 50c176d583aafdeedcf70442d58336afb5db3c19 
>   config/telepathy-kded-config.ui 54ebc542d7d04c14c7122d2ca3c2e0533b2c3b21 
>   lockingscreenaway.h PRE-CREATION 
>   lockingscreenaway.cpp PRE-CREATION 
>   telepathy-module.h 748437d6268fce49ef276ab996d18cbcb93da025 
>   telepathy-module.cpp d3f223a8391e232c74a5a03b9277f6c518c4d2ed 
> 
> Diff: http://git.reviewboard.kde.org/r/110260/diff/
> 
> 
> Testing
> -------
> 
> Manually by myself.
> 
> 
> Thanks,
> 
> Lucas Betschart
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130501/0faa9103/attachment-0001.html>