Review Request: Create an option for the behaviour of new tabs

[Francesco Nwokeka]

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



app/telepathy-chat-ui.h
<http://git.reviewboard.kde.org/r/101886/#comment4103>

    if this enum is to be public, could you put it on top of the class (before the constructor) please?



config/appearance-config.h
<http://git.reviewboard.kde.org/r/101886/#comment4105>

    watch out here
    



config/behavior-config.h
<http://git.reviewboard.kde.org/r/101886/#comment4106>

    QWodget *parent = 0 -> cooler. keep the * next to the var name



config/behavior-config.h
<http://git.reviewboard.kde.org/r/101886/#comment4107>

    could you add a var name explaining what's to be passes to this function?



config/behavior-config.h
<http://git.reviewboard.kde.org/r/101886/#comment4108>

    same as above



config/behavior-config.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4110>

    remember to "sync()" your config file after finished writing



config/behavior-config.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4111>

    set the ui constructor on a new line



config/behavior-config.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4114>

    what's this for? if you don't have to output anything don't put the debug there. I find it useless



config/behavior-config.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4116>

    the "m_" is used for private members. This var is used only in this funciton, so take the "m_" away".
    Plus you don't need to test the existance of the QButtonGroup with the Q_ASSERT here.
    
    As this button group is used wouldn't it be better to set it in the ui? this way it gets deleted on the deletion of the settings widget



config/behavior-config.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4117>

    do you still need this?



config/main-window.h
<http://git.reviewboard.kde.org/r/101886/#comment4118>

    lol, don't kill off old devs :P just add your credentials



config/main-window.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4119>

    same here with your credentials



config/main-window.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4120>

    keep the * next to the var. On a new line start the constructors for KCModule, 



config/main-window.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4121>

    add this before entering the { in the constructor. (after the KCModule initializer)



config/main-window.cpp
<http://git.reviewboard.kde.org/r/101886/#comment4122>

    same as above


- Francesco


On July 12, 2011, 3:02 p.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101886/
> -----------------------------------------------------------
> 
> (Updated July 12, 2011, 3:02 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This was rather a big refractor (for me at least), and this is the first time I created any new classes. So I'm not sure I did quite a few things right.
> 
> I created a new tab in the KCM, called Behaviour. I think we'll probably have to change the labels on the radiobuttons to something that makes more sense. 
> 
> I also included "../app/telepathy-chat-ui.h" because I needed an enum from it. I don't know if there's anything against doing that or not.
> 
> I also am not sure I used a QButtonGroup correctly. 
> 
> Also, I noticed the other class didn't use a d-pointer, (even though a KCM is supposed to be like a library file) so I didn't. 
> 
> There are many more things I was doubtful at the time, but I can't possibly remember now (I shouldn't leave writing review requests till 2AM). Hopefully, everything makes sense in the diff.
> 
> And as always, my code is in my scratch repo:
> http://quickgit.kde.org/?p=clones%2Ftelepathy-chat-handler%2Ffernando%2FdetachableTabs.git&a=shortlog&h=refs/heads/refractored_tabs
> 
> 
> Diffs
> -----
> 
>   app/telepathy-chat-ui.h f3cab67 
>   app/telepathy-chat-ui.cpp fe0fb9b 
>   config/CMakeLists.txt 48252a7 
>   config/appearance-config.h PRE-CREATION 
>   config/appearance-config.cpp PRE-CREATION 
>   config/behavior-config.h PRE-CREATION 
>   config/behavior-config.cpp PRE-CREATION 
>   config/behaviorconfig.ui PRE-CREATION 
>   config/main-window.h e95bf93 
>   config/main-window.cpp 3a79da4 
>   config/main.cpp c47f493 
> 
> Diff: http://git.reviewboard.kde.org/r/101886/diff
> 
> 
> Testing
> -------
> 
> Changed a (well, the only) setting, and the program changed behaviour as expected. 
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/101886/s/198/
> 
> 
> Thanks,
> 
> Lasath
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110713/6faee910/attachment-0001.htm