> On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > Some general remarks on your new class's coding style: there are many > > unnecessary empty lines, and it's better for maintainability that member > > variables are prefixed with 'm' or 'm_' so that they can be easily > > distinguished from normal, method variables. Some other comments follow. > > Cristi P wrote: > using m_ is also my style of coding. I just chose to follow the > conventions in the file I worked on. > At least I tried to do that. > Now, look at: > * webcamtask.h -> no m_ prefix > * yahoocontact.h -> worse: *both* m_ and without prefix member variables. > > So... what should it be? I would suggest - leave them as they are... >
Unfortunately the default style for those files is... ugly :) I'd suggest starting using some sane coding style with the files you're adding, and in the future fixing the other files. > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 133 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line133> > > > > static_cast, for consistency. > > Cristi P wrote: > for such a thing, gcc gives a deprecated warning. so, no, leaving those > as they are. > Actually, the warning's because it should be a const_cast, not a static_cast. - Raphael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9680 ----------------------------------------------------------- On Jan. 15, 2011, 11:13 a.m., Cristi P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/6312/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2011, 11:13 a.m.) > > > Review request for Kopete. > > > Summary > ------- > > I saw that for yahoo - webcam images are converted by running an external > program. Personally I am not ok with this since running an external program > every 0.X seconds is a bit expensive. Also, not to mention using disk files > (at least pushing directly into the program and reading its output would have > been better). > Also - I saw that inviteWebcam action was checking for presence of jasper > program - but requestWebcam should also have done that. Not to mention that > if a yahoo contact just invites you to see his cam, you might get a feedback > of missing program after accepting to invitation. Not necessarily a big deal, > though. > > Note that this will mean that having jasper lib at compile time will get to > be mandatory - and needs to also be added as dependency by package managers. > > > Diffs > ----- > > trunk/KDE/kdenetwork/kopete/CMakeLists.txt 1214536 > trunk/KDE/kdenetwork/kopete/config-kopete.h.cmake 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/CMakeLists.txt 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt > 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.h > PRE-CREATION > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp > PRE-CREATION > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamtask.cpp > 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoocontact.cpp 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.h 1214536 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.cpp 1214536 > > Diff: http://svn.reviewboard.kde.org/r/6312/diff > > > Testing > ------- > > compiled and ran it. Communicating with another linux kopete. Tried both > directions of seeing the cam. > > > Thanks, > > Cristi > >
_______________________________________________ kopete-devel mailing list kopete-devel@kde.org https://mail.kde.org/mailman/listinfo/kopete-devel
