----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100687/#review2257 -----------------------------------------------------------
Ship it! Whoa, that's some pretty high-quality code we have here, congratulations. I pointed out some issues I found out here and there, but it's gold quality to me - except for the test issue. Marking as "Ship it!" given that is still something out of the scope of the main code. CMakeLists.txt <http://git.reviewboard.kde.org/r/100687/#comment1912> Was about to yell "PKGCONFIG!!" when I saw your comment, so ok, if you really mean it :) otherwise, try and draft a quick cmake find. libqtf/CMakeLists.txt <http://git.reviewboard.kde.org/r/100687/#comment1914> If those are meant to stay, you need a cmake module for that (I know, I feel your pain :( ) libqtf/CMakeLists.txt <http://git.reviewboard.kde.org/r/100687/#comment1913> LDFLAGS->LIBRARIES: typo? libqtf/qtf.cpp <http://git.reviewboard.kde.org/r/100687/#comment1915> Watch out for correct indentation libtelepathy-kde-call/callchannelhandler.h <http://git.reviewboard.kde.org/r/100687/#comment1917> Nitpicking: you should change to 2010-2011 instead of changing the year completely when talking about copyrights libtelepathy-kde-call/callchannelhandler.h <http://git.reviewboard.kde.org/r/100687/#comment1918> code style: const * d; Also, you'd be better off calling this class Private, given that it is declared inside the scope of its parent class. If you want to preserve the naming, you could move it outside. libtelepathy-kde-call/callcontenthandler.h <http://git.reviewboard.kde.org/r/100687/#comment1919> Try and use /** instead of /*! when writing docs libtelepathy-kde-call/callcontenthandler.cpp <http://git.reviewboard.kde.org/r/100687/#comment1920> code style: Q_FOREACH ( libtelepathy-kde-call/callcontenthandler.cpp <http://git.reviewboard.kde.org/r/100687/#comment1921> Danger ahead: be sure to notice that sender() might be invalid when connecting a slot to this signal. libtelepathy-kde-call/callcontenthandler.cpp <http://git.reviewboard.kde.org/r/100687/#comment1922> Code style: avoid using brackets in case: libtelepathy-kde-call/sinkcontrollers.h <http://git.reviewboard.kde.org/r/100687/#comment1923> Try and file an upstream bug, it definitely might have its place up there. libtelepathy-kde-call/sinkcontrollers.h <http://git.reviewboard.kde.org/r/100687/#comment1924> Why is the destructor protected? libtelepathy-kde-call/sinkcontrollers.h <http://git.reviewboard.kde.org/r/100687/#comment1925> Missing destructor libtelepathy-kde-call/sinkcontrollers.cpp <http://git.reviewboard.kde.org/r/100687/#comment1926> You'd be better off in using Q_DECLARE_PUBLIC, which gives inheritance goodness to the q pointer as well. libtelepathy-kde-call/sinkcontrollers.cpp <http://git.reviewboard.kde.org/r/100687/#comment1927> You could think about switching to QReadWriteLock instead of QMutex here libtelepathy-kde-call/sinkcontrollers.cpp <http://git.reviewboard.kde.org/r/100687/#comment1928> elegance points awarded libtelepathy-kde-call/sinkcontrollers_p.h <http://git.reviewboard.kde.org/r/100687/#comment1929> As I told you before, maybe a QReadWriteLock would be more handy? (that's just as a personal preference, and not a real issue) libtelepathy-kde-call/tests/CMakeLists.txt <http://git.reviewboard.kde.org/r/100687/#comment1930> That's probably the only real issue I see here: this class should be moved indeed to be an autotest. - Dario On Feb. 19, 2011, 10:20 p.m., George Kiagiadakis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100687/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2011, 10:20 p.m.) > > > Review request for Telepathy. > > > Summary > ------- > > This is a complete refactoring of libtelepathy-kde-call, the library that > handles audio/video streaming using telepathy-farstream. > > The new design is based on the new Call.DRAFT spec and telepathy-farstream, > which are way cleaner than their predecessors, StreamedMediaChannel and > telepathy-farsight. This inherently makes the design of this library cleaner > too. In addition to this, the GStreamer sources and sinks have also been > redesigned to handle correctly possible race conditions (i.e. do correct > synchronization between the main thread and the gstreamer streaming threads > and also between telepathy-qt4 and telepathy-glib) and also allow dynamic > switching of source devices. > > http://quickgit.kde.org/?p=clones/telepathy-call-ui/gkiagia/telepathy-call-ui.git&a=shortlog&h=refs/heads/call-2 > > > Diffs > ----- > > CMakeLists.txt 59582bfa98fd738e50a5efbb39d03802e9f0e25c > libqtf/CMakeLists.txt PRE-CREATION > libqtf/qtf.h PRE-CREATION > libqtf/qtf.cpp PRE-CREATION > libtelepathy-kde-call/CMakeLists.txt > d6a39b8d182f457909a78bf9a02a0d0efbf1aceb > libtelepathy-kde-call/callchannelhandler.h > 94c27c18a1c37f590232acd9c9a77a969f85bedd > libtelepathy-kde-call/callchannelhandler.cpp > 05aeff6040bb741a61b08ff1520e002487903406 > libtelepathy-kde-call/callchannelhandler_p.h > 5a1ce85bee81ef0e3c088e22845537e9c60c62f0 > libtelepathy-kde-call/callcontenthandler.h PRE-CREATION > libtelepathy-kde-call/callcontenthandler.cpp PRE-CREATION > libtelepathy-kde-call/callcontenthandler_p.h PRE-CREATION > libtelepathy-kde-call/callparticipant.h > 5ac2ad7d69e5465f18f25de0c86b8333ee632af3 > libtelepathy-kde-call/callparticipant.cpp > 9a683935d1212262671b6bab071b179b70dacb5b > libtelepathy-kde-call/sinkcontrollers.h PRE-CREATION > libtelepathy-kde-call/sinkcontrollers.cpp PRE-CREATION > libtelepathy-kde-call/sinkcontrollers_p.h PRE-CREATION > libtelepathy-kde-call/sourcecontrollers.h PRE-CREATION > libtelepathy-kde-call/sourcecontrollers.cpp PRE-CREATION > libtelepathy-kde-call/sourcecontrollers_p.h PRE-CREATION > libtelepathy-kde-call/tests/CMakeLists.txt PRE-CREATION > libtelepathy-kde-call/tests/sourcetest.cpp PRE-CREATION > libtelepathy-kde-call/volumecontroller.h PRE-CREATION > libtelepathy-kde-call/volumecontroller.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/100687/diff > > > Testing > ------- > > I have successfully done audio calls with call-ui <-> empathy and call-ui <-> > echo bot, using a modified version of the GUI that will be in another review > request later, once it is completed. Video calls and some other features are > not tested, as the GUI still needs some work to support them. > > > Thanks, > > George > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
