> 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
