Review Request: Chime Time Implementation

[John Layt]

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


Given there is no real functionality added, it's a bit hard to review, but you do seem to be in the right place now.  You don't seem to have paid attention to the last email I did describing how to do the ui with a combo for choosing the feedback option as it takes less space.

The reason for the "bug" of the config choice changing is because you haven't written the choice to the config file.

I have made comments on the coding style, you need to stick with the coding style used by the existing code in the file, which is usually http://techbase.kde.org/Policies/Kdelibs_Coding_Style or very similar.  You also seem to have added a lot of extra whitespace?


libs/plasmaclock/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101074/#comment2339>

    Is this stuff needed, surely it worked before so I can't see why you need to add this now?



libs/plasmaclock/clockapplet.h
<http://git.reviewboard.kde.org/r/101074/#comment2340>

    Don't put the include in the header if you are not using the class in the header, it slows things down.  Put it in the .cpp only.



libs/plasmaclock/clockapplet.h
<http://git.reviewboard.kde.org/r/101074/#comment2341>

    What does this do?  Give it a more meaningful name that explains at a glance.



libs/plasmaclock/clockapplet.h
<http://git.reviewboard.kde.org/r/101074/#comment2342>

    You shouldn't put variables in the class if a Private d-ptr is being used, put these in the Private class instead, and use the KDE naming conventions, i.e. not s_*.



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2343>

    Should be <QtGui/QRadioButton> if not already included by .ui file via moc



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2344>

    coding style, opening brace of method should be on separate line.



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2345>

    Coding style: put back the space before the opening brace



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2349>

    Code style: Also have braces around if/else even if just one line.



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2346>

    Coding style: braces belong with keywod



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2350>

    Style: put back the space before brace



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2356>

    Why have a bool for s_none when you can derive it from s_chime and s_speak?  Better yet, seeing as these are exclusive options, this should only be one variable with either a string like "chime" or "speak" or a private enum.



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2353>

    Code style: Should be space between close bracket and open brace



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2354>

    Code style: brace and else if should be on same line



libs/plasmaclock/clockapplet.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2355>

    Code style: brace and else if should be on same line



plasma/generic/applets/analog-clock/clock.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2351>

    Why remove the line?



plasma/generic/applets/analog-clock/clock.cpp
<http://git.reviewboard.kde.org/r/101074/#comment2352>

    Put back the space before brace


- John


On April 10, 2011, 1:22 p.m., Sunny Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101074/
> -----------------------------------------------------------
> 
> (Updated April 10, 2011, 1:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Presently I have developed a UI for the setting s in the "General" part. There are at present 3 radio buttons and according to the user wish, the radiobutton which is clicked would work. There is a bug since the settings gets changed when you again go and see the general settings.
> 
> 
> Diffs
> -----
> 
>   libs/plasmaclock/CMakeLists.txt 667df3c 
>   libs/plasmaclock/clockapplet.h b75f286 
>   libs/plasmaclock/clockapplet.cpp b806792 
>   libs/plasmaclock/generalConfig.ui aae25c0 
>   plasma/generic/applets/analog-clock/clock.cpp 68d0725 
> 
> Diff: http://git.reviewboard.kde.org/r/101074/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> snapshot
>   http://git.reviewboard.kde.org/r/101074/s/122/
> 
> 
> Thanks,
> 
> Sunny
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110414/1b67da36/attachment-0001.htm