> On Jan. 10, 2011, 7:19 p.m., Frank Schaefer wrote: > > When disabling of the video part is cleaned up / fixed, you can simplify > > your patch a lot: > > > > Just do > > > > if ( NOT DISABLE_VIDEOSUPPORT ) > > find_package(Jasper) > > macro_log_feature(...) > > endif > > > > in CMakeLists.txt and skip (do not include) webcamimgformat.* > > > > > > Cristi P wrote: > Yes, the CMakeLists.txt file could be changed like that, at that moment. > As for including webcamimgformat.*... it could as well remain the same, since > function implementations are covered by some #ifdef HAVE_JASPER. > > Have you checked the 2nd patch? Want other changes in it? > > > Frank Schaefer wrote: > Excluding the whole files from building is the cleaner solution. There > are already to many #ifdefs in code.
I would agree with you, generally. But let's see this particular case: * I would have to add some #ifdef HAVE_JASPER in at least 2 files (to not include the webcamimgformat.h file). And then add some more #ifdef around the calls to WebcamImgFormat::for/fromYahoo. What if (theoretically) more files would like to use those conversion functions? Why just not let the WebcamImgFormat behave itself ok based on having the JasPer lib included? So - to me, it looks better this way. If you really really want me to add other #ifdef to cope w/ absence of webcamimgformat* files, in the CMakeList.txt files and in the 2 .cpp files I can do that, but let me know quickly, I'd like to get this included so I can work on other fixes! - Cristi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9607 ----------------------------------------------------------- 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
