> On Jan. 9, 2011, 6:02 p.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt, line 2 > > <http://svn.reviewboard.kde.org/r/6312/diff/1/?file=44034#file44034line2> > > > > Please use KDE's macro_optional_find_package + macro_log_feature > > instead of find_package(), so that CMake does not immediately error out > > when Jasper is not found. > > Cristi P wrote: > based on naming, I guess that means the jasper lib won't be declared as > mandatory and code changes done so that it copes w/ the missing lib? >
Actually the name is a bit misleading: macro_optional_find_package works just like find_package, but makes it possible to pass -DWITH_foo=FALSE to CMake; macro_log_feature has a "REQUIRED" argument which makes the feature required (it just runs the CMake stuff until the end before showing which packages are missing). For more information, see http://api.kde.org/cmake/modules.html In this case, it might make more sense to use find_package (without passing REQUIRED) + macro_log_feature. > On Jan. 9, 2011, 6:02 p.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.cpp, > > line 216 > > <http://svn.reviewboard.kde.org/r/6312/diff/1/?file=44036#file44036line216> > > > > Is there a reason to put this value in a variable instead of hardcoding > > it as the code used to? > > Cristi P wrote: > because I played a bit w/ the value of it. But, also because I'm thinking > if the value can't be changed by some slider in some Yahoo - Webcam > Preferences dialog. > I guess that's one of the things windows version is doing as well. I can > put it hardcoded and just leave a comment. I think it makes more sense to leave it hardcoded for now, then. If later a GUI is added to control that, it can be made a variable again. > On Jan. 9, 2011, 6:02 p.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.h, > > line 9 > > <http://svn.reviewboard.kde.org/r/6312/diff/1/?file=44035#file44035line9> > > > > Why are you exporting those two functions? > > Cristi P wrote: > somehow the functions didn't get into libkyahoo and I always got linker > error for libkopete_yahoo. I'll change anyway into a class with some static > functions or something. If you do need to keep these functions exported, #include libkyahoo_export.h and use LIBKYAHOO_EXPORT instead of Q_DECL_EXPORT. As for creating a class with static methods, consider using a singleton so that those static functions you added to initialize the Jasper stuff can be moved to the constructor, for example. - Raphael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9586 ----------------------------------------------------------- On Jan. 9, 2011, 4:21 p.m., Cristi P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/6312/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2011, 4:21 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/protocols/yahoo/libkyahoo/CMakeLists.txt > 1212994 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.h > PRE-CREATION > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.cpp > PRE-CREATION > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamtask.cpp > 1212994 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoocontact.cpp 1212994 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.h 1212994 > trunk/KDE/kdenetwork/kopete/protocols/yahoo/yahoowebcam.cpp 1212994 > > 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
