Review Request: API to start log/chat/calls

[Daniele Elmo Domenichelli]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20121102/1222e830/attachment.html>