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/#review5957
-----------------------------------------------------------


A few minor errors to correct.
Next time, when moving around big portions of code, explain why and where you've moved it so you can make our reviews quicker and easier to do


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

    move the "*" to the left of the function( ChatWindow *createWindow) and add a doxygen line explaining what this does.



config/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101886/#comment5265>

    This is just a mental problem of mine, but would you mind keeping files in alphabetical order?



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

    sure you need these two methods to be virtual?



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

    sure this is supposed to be virtual?



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

    move "*" to the left of the member



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

    Keep the constructor on top of the C++ file



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

    Don't kill off our devs



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

    the ":" on a new line



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

    every member you initialize is to be set on a new line


- Francesco


On July 28, 2011, 1:28 p.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101886/
> -----------------------------------------------------------
> 
> (Updated July 28, 2011, 1:28 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 71c2503 
>   config/CMakeLists.txt f624431 
>   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 
> 
> 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/20110823/ceee21be/attachment-0001.html>