----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9736 -----------------------------------------------------------
Oops. While taking a last look at everything here before applying, it occurred to me that we shouldn't make Jasper a hard dependency as it currently is -- since building the Yahoo protocol itself is optional (eg -DWITH_YAHOO=OFF can be passed to CMake), it should be required only when the Yahoo protocol is being built. Thus, the checks can be made more similar to the ones already available in protocols/CMakeLists.txt, for example. Can you please change so I can commit it? One last thing: please use tabs when coding (it's the only entry in Kopete's HACKING document ;) -- I've changed your patch locally to use tabs where you used spaces, but remember that when working on your next patches. - Raphael On Jan. 26, 2011, 10:42 p.m., Cristi P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/6312/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2011, 10:42 p.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 1217129 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/CMakeLists.txt 1217129 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt > 1217129 > 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 > 1217129 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoocontact.cpp 1217129 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.h 1217129 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.cpp 1217129 > > 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
