> On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > Some general remarks on your new class's coding style: there are many > > unnecessary empty lines, and it's better for maintainability that member > > variables are prefixed with 'm' or 'm_' so that they can be easily > > distinguished from normal, method variables. Some other comments follow.
using m_ is also my style of coding. I just chose to follow the conventions in the file I worked on. At least I tried to do that. Now, look at: * webcamtask.h -> no m_ prefix * yahoocontact.h -> worse: *both* m_ and without prefix member variables. So... what should it be? I would suggest - leave them as they are... > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.h, > > line 53 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44234#file44234line53> > > > > Why 20 if you're storing 3-character extensions? trying to be on the safer side if, in the future, some format text would be longer than 3 characters and the coder would forget to update that size in the header file. I'll fix that although compared to other memory consumptions around, that is nothing :) > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 104 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line104> > > > > No need to keep this commented out function. I'll remove that for you. *but*, I generally am appreciating such small commented debug lines - they make it easy for me, as a developer, to not go and read some damn big manual to see how to activate some debugging. That is, if I was me committing, I would have kept that comment. > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 107 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line107> > > > > What are you using this variable for? Search "formats". To display with kDebug what type of intermediary conversion image was used. Because the code tries a couple. > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 113 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line113> > > > > strncpy here and in the other calls just to always stay on the safe > > side? done. code looks uglier. that is a bit picky in this case :-) > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 133 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line133> > > > > static_cast, for consistency. for such a thing, gcc gives a deprecated warning. so, no, leaving those as they are. > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 154 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line154> > > > > Missing closing ')' yeah - thanks for the observation, didn't test that. removed anyway, as per some other reviewer. > On Jan. 23, 2011, 4:29 a.m., Raphael Kubo da Costa wrote: > > trunk/KDE/kdenetwork/kopete/protocols/yahoo/libkyahoo/webcamimgformat.cpp, > > line 39 > > <http://svn.reviewboard.kde.org/r/6312/diff/3/?file=44235#file44235line39> > > > > Why not make this a private class method? > > > > Plus, isn't it possible to keep some of these in memory so that you do > > not need to do all this set up for every call (I've never used jasper so I > > have no idea)? a) why bother to export a function into a header if it ain't using anything from the object itself? Also, I would have to include the jasper library into webcamimgformat.h due to 2 types being used. Why do that, if it can stay like that, that is - real implementation being hidden? b) to keep what in memory? I have never used jasper myself either, btw. The only thing that comes into my mind that we might want to try to keep in memory, is (and if jasper knows that) the buffer for his internal intermediate conversion. Of course, adding some code somewhere... (where?) to release that buffer once no one will use it for some longer period. So, I suggest to keep it like this. - Cristi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6312/#review9680 ----------------------------------------------------------- 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
