> On Jan. 18, 2011, 6:03 p.m., Frank Schaefer wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoocontact.cpp, lines 632-638 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44237#file44237line632> > > > > Keep the return path, if libjasper is not present. > > Cristi P wrote: > Please tell me the reason to do that since you made me mark jasper as > mandatory. > I would have understand to put a message in there (it crossed my mind) if > I would have left, as I really wanted and wrote the code, the logic of the > JasPer library being mandatory or not, ONLY in CMakeLists.txt. >
See above. > On Jan. 18, 2011, 6:03 p.m., Frank Schaefer wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.cpp, lines 29-30 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44239#file44239line29> > > > > #ifdef HAVE_JASPER > > #include "webcamimgformat.h" > > #endif > > Cristi P wrote: > same question from me as somewhere else... Same answer as above... > On Jan. 18, 2011, 6:03 p.m., Frank Schaefer wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamtask.cpp, lines > > 23-24 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44236#file44236line23> > > > > #ifdef HAVE_JASPER > > #include "webcamimgformat.h" > > #endif > > Cristi P wrote: > I thought I already said my opinion about leaving the webcamimgformat.h + > .cpp in there but change only the function implementation. Please see my > comment from Jan 10. > > Also, why would I still add the #ifdef HAVE_JASPER if we made it > mandatory? > Waiting for feedback on both things. I've read your comments, but I have a different opinion. Creating instances of objects which then do nothing is no clean solution. If you don't need/use code, just don't compile it in. Looking at the changes I suggested you will see, that this is easy to do. in fact it will slightly simplify the patch. Concerning #ifdef HAVE_JASPER and making libjasper mandatory: you already answered your own question. ;) Some more work would be necessary before we can make it optional. Anyway, there is no need to make it optional as to the DISABLE_VIDEOSUPPORT switch. Not sure if there a other reason for doing this (I remember people crying when we made libv4l mandatory...). So I'm leaving it up to you to either remove the whole HAVE_JASPER (+ CMake-files) stuff or do it the right way. - Frank ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9645 ----------------------------------------------------------- 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
