----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9680 -----------------------------------------------------------
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. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.h <http://svn.reviewboard.kde.org/r/6312/#comment10821> Why 20 if you're storing 3-character extensions? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10825> Why not make this a private class method? Plus, isn't it possible to keep some of these in memory so that you do not need to do all this set up for every call (I've never used jasper so I have no idea)? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10819> No need to keep this commented out function. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10820> What are you using this variable for? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10822> strncpy here and in the other calls just to always stay on the safe side? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10823> static_cast, for consistency. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10824> Missing closing ')' - Raphael 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
