<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107169/">http://git.reviewboard.kde.org/r/107169/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 2nd, 2012, 1:42 p.m., <b>Daniele Elmo Domenichelli</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
<br />
<p>- David</p>
<br />
<p>On November 2nd, 2012, 12:24 p.m., Dan Vrátil wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By Dan Vrátil.</div>
<p style="color: grey;"><i>Updated Nov. 2, 2012, 12:24 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>KTp/CMakeLists.txt <span style="color: grey">(ae65dc7)</span></li>
<li>KTp/channel-handlers.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>KTp/channel-handlers.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107169/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>