> On Nov. 2, 2012, 1:42 p.m., Daniele Elmo Domenichelli wrote: > > 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
It's not so much about recreating all of the methods in account, it's about recreating starting _our_ text chat, _our_ calls etc, where we want to set our handler. I think it's fairly sensible to do it for everything where KTp ships the service at the other end. If you wanted an arbitrary streamtube to something, like kbattleship, you wouldn't use this. You'd be using the basic account->ensureStreamTube(.......), and (IMHO) that's fine. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107169/#review21348 ----------------------------------------------------------- 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
