----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107169/#review21348 -----------------------------------------------------------
I agree that the #define PREFERRED_*_HANDLER everywhere is simply ridicolous, but to be honest I'm not really sure if I like this patch. Just check how many ensure/create methods are in Tp::Account, are you planning to add one method for each of them? http://telepathy.freedesktop.org/doc/telepathy-qt/a00040.html Moreover you are adding one for rfb, are you going to add one for each tube channel possible? The QDateTime should be the moment when the user requests the channel to start, so I'm not sure if it's good not allowing to pass it. Finally, the log viewer is not really a handler, so I don't like it very much in there. I'd rather have an API to retrieve the available handlers and the preferred handler for a specific channel type, and just call the ensure/create channels as we do now. And I would like to have the preferred handlers configurable and stored somewhere. I think that George K. started doing something about this some time ago, but I might be wrong... For the log-viewer an openLogViewer method seems ok, but not in the KTp::ChannelHandler namespace. Perhaps KTp::openLogViewer() is enough - Daniele Elmo Domenichelli On Nov. 2, 2012, 12:24 p.m., Dan Vrátil wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107169/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2012, 12:24 p.m.) > > > Review request for Telepathy. > > > Description > ------- > > I think it's silly to have #define PREFERRED_*_HANDLER everywhere where we > want to handle "start chat/call/..." actions. If we ever want/have to change > the strings, this is the best way to forget about some hidden #define > somewhere and break things. > > Also looking at the ensure*Call, createStreamTube - there are these magic > keywords, like "audio" or "rfb" (really, how is "rfb" related to "desktop > sharing"?) and the useless QDateTime::currentDateTime() being passed > everywhere. > > Finally, because I have problems with remembering KInvocationTool classname, > it also has openLogViewer() method (IIRC David already criticized hardcoding > the ktp-log-viewer name and arguments list everywhere we want to support > logviewer). > > All the methods in the API proposed in this patch only have two arguments: > Tp::AccountPtr and Tp::ContactPtr (and filename in case of filetransfer) - it > looks much cleaner and pretty and it nicely takes care of the magic strings > and #defines that I don't like. > > If you happen to like this proposal, I'll post reviews for porting all > components to the API. > > > Diffs > ----- > > KTp/CMakeLists.txt ae65dc7 > KTp/channel-handlers.h PRE-CREATION > KTp/channel-handlers.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/107169/diff/ > > > Testing > ------- > > > Thanks, > > Dan Vrátil > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
