----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9586 -----------------------------------------------------------
The idea of not relying on an external process for this sounds good (it'd be nice if you could look up our bugzilla and see if there's any bug report related to this). The code still needs some work IMO. The files you have added look "un-Qt-ish": you could turn that code into a class for a start, and the coding style there does not match the rest of Kopete. Can you work on these issues and submit a new patch? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt <http://svn.reviewboard.kde.org/r/6312/#comment10569> No need to add this comment. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt <http://svn.reviewboard.kde.org/r/6312/#comment10570> 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. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/CMakeLists.txt <http://svn.reviewboard.kde.org/r/6312/#comment10571> Style nitpick: either remote the space from both sides inside the ()'s, or leave both there. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.h <http://svn.reviewboard.kde.org/r/6312/#comment10572> Missing copyright header. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.h <http://svn.reviewboard.kde.org/r/6312/#comment10575> Why are you exporting those two functions? trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10573> No need to keep this commented out block. trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcam_img_format.cpp <http://svn.reviewboard.kde.org/r/6312/#comment10574> Is there a reason to put this value in a variable instead of hardcoding it as the code used to? - Raphael 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
