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

[Lasath Fernando]

> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.cpp, line 73
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26782#file26782line73>
> >
> >     set the ui constructor on a new line

Not sure why, but okay.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/main-window.h, line 2
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26784#file26784line2>
> >
> >     lol, don't kill off old devs :P just add your credentials

I didn't actually 'kill off' your credentials. I moved them to appearance-config.h because I moved pretty much all your work there. 

I created a new file with the same name and now git thinks I just changed your name


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/main-window.cpp, line 2
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26785#file26785line2>
> >
> >     same here with your credentials

same as above


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > app/telepathy-chat-ui.h, lines 52-56
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26776#file26776line52>
> >
> >     if this enum is to be public, could you put it on top of the class (before the constructor) please?

done.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/appearance-config.h, line 67
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26779#file26779line67>
> >
> >     watch out here
> >

fixed.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.h, line 36
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26781#file26781line36>
> >
> >     QWodget *parent = 0 -> cooler. keep the * next to the var name
> 
> David Edmundson wrote:
>     Source: http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Whitespace

fixed.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.h, line 42
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26781#file26781line42>
> >
> >     could you add a var name explaining what's to be passes to this function?

done.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.h, line 47
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26781#file26781line47>
> >
> >     same as above

did the same as other classes, and called it 'e'.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.cpp, line 69
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26782#file26782line69>
> >
> >     remember to "sync()" your config file after finished writing

fixed.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.cpp, lines 80-81
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26782#file26782line80>
> >
> >     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

fixed.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/behavior-config.cpp, line 89
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26782#file26782line89>
> >
> >     do you still need this?

vaporised.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/main-window.cpp, lines 35-36
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26785#file26785line35>
> >
> >     keep the * next to the var. On a new line start the constructors for KCModule,

okay


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/main-window.cpp, line 152
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26785#file26785line152>
> >
> >     add this before entering the { in the constructor. (after the KCModule initializer)

fixed.


> On July 13, 2011, 12:52 p.m., Francesco Nwokeka wrote:
> > config/main-window.cpp, line 174
> > <http://git.reviewboard.kde.org/r/101886/diff/2/?file=26785#file26785line174>
> >
> >     same as above

fixed.


- Lasath


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


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/20110728/3be39677/attachment-0001.htm