> On March 30, 2011, 2:52 p.m., Dario Freddi wrote: > > CMakeLists.txt, line 13 > > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9527#file9527line13> > > > > 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. > > George Kiagiadakis wrote: > I'd prefer if upstream (you in this case?) added a > TelepathyQt4*Config.cmake file :)
Will do :) > On March 30, 2011, 2:52 p.m., Dario Freddi wrote: > > libqtf/CMakeLists.txt, lines 4-5 > > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9528#file9528line4> > > > > If those are meant to stay, you need a cmake module for that (I know, I > > feel your pain :( ) > > George Kiagiadakis wrote: > Yeah, those are to stay. It's quite a PITA to do this in cmake because > the dependencies actually are: > > * telepathy-farstream > * farstream > * gstreamer > * telepathy-glib > * dbus-glib > * libxml2 > * gio > * gobject > * glib > * telepathy-qt4-yell-farstream > > I don't think pkgconfig is bad in this case, because it really solves a > problem here and since this code will not work on any other platform than > unix/x11 at the moment (requires Qt glib event loop integration), there is no > point in arguing that there will be build problems on windows/mac (which is > the main reason for not using pkgconfig, right?) Ok, but if that is the case, please require pkgconfig when building to prevent failures > On March 30, 2011, 2:52 p.m., Dario Freddi wrote: > > libtelepathy-kde-call/callchannelhandler.h, line 51 > > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9532#file9532line51> > > > > 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. > > George Kiagiadakis wrote: > You sure about T const * d? Isn't it T * const d; ? At least that's what > it says here: http://techbase.kde.org/Policies/Library_Code_Policy#D-Pointers > > Or maybe you are referring to the missing space? In this case, I'll fix > it. > > As for the name, this class is not declared inside the scope of this > class, see the forward declaration above. I'd prefer to keep it that way, > since it's a standalone QObject. Sorrysorrysorry - I messed up: I was obviously pointing the missing space: T * const d; > On March 30, 2011, 2:52 p.m., Dario Freddi wrote: > > libtelepathy-kde-call/callcontenthandler.cpp, line 49 > > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9536#file9536line49> > > > > Danger ahead: be sure to notice that sender() might be invalid when > > connecting a slot to this signal. > > George Kiagiadakis wrote: > You mean because I call deleteLater() afterwards? Yes, sender might be > invalid if the slot is connected with Qt::QueuedConnection or from another > thread, but in this case, this object is private and it is only used with > Qt::DirectConenction here, so it's ok. I am aware of it. Ok - I didn't realize it was not meant to be exported, my fault > On March 30, 2011, 2:52 p.m., Dario Freddi wrote: > > libtelepathy-kde-call/tests/CMakeLists.txt, lines 1-2 > > <http://git.reviewboard.kde.org/r/100687/diff/1/?file=9546#file9546line1> > > > > That's probably the only real issue I see here: this class should be > > moved indeed to be an autotest. > > George Kiagiadakis wrote: > The problem here is that there is nothing to check programatically in > this test. The check is done by listening if audio is stopped and resumed > correctly, so this test needs to run manually. I added it basically for > experimenting with gstreamer, because I wasn't sure if the trick I did would > work as expected. Ok, in this case go and merge! (and mark the review as submitted, too!) - Dario ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100687/#review2257 ----------------------------------------------------------- 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
