<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/100199/">http://git.reviewboard.kde.org/r/100199/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 10th, 2010, 2:58 a.m., <b>Daniele Elmo Domenichelli</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/100199/diff/1/?file=4967#file4967line35" style="color: black; font-weight: bold; text-decoration: underline;">src/textchannelapprover.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">TextChannelApprover::TextChannelApprover(const Tp::TextChannelPtr & channel, QObject *parent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_FOREACH</span><span class="p">(</span><span class="k">const</span> <span class="n">Tp</span><span class="o">::</span><span class="n">ReceivedMessage</span> <span class="o">&</span> <span class="n">msg</span><span class="p">,</span> <span class="n">channel</span><span class="o">-></span><span class="n">messageQueue</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">36</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">onMessageReceived</span><span class="p">(</span><span class="n">msg</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">39</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">channel</span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">messageReceived</span><span class="p">(</span><span class="n">Tp</span><span class="o">::</span><span class="n">ReceivedMessage</span><span class="p">)),</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SLOT</span><span class="p">(</span><span class="n">onMessageReceived</span><span class="p">(</span><span class="n">Tp</span><span class="o">::</span><span class="n">ReceivedMessage</span><span class="p">)));</span></pre></td>
</tr>
</tbody>
</table>
<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 might be wrong here...
If I understand correctly, you show a popup for each message in the channel message queue. Then you show more popups when a new message is received.
Isn't the chat handler supposed to show just ONE notification when someone starts a chat with you (maybe just showing the first message or appending the text of the following messages)? I would hate having several notification asking me to accept the same chat.
Also what happens if the channel is started in status ChannelChatStateComposing? Actually I'm not sure whether this can happen and how we are supposed to handle it. Should we wait for the first message to show the accept channel popup? But then what happens if the other side stops writing and doesn't send any message?</pre>
</blockquote>
<p>On December 10th, 2010, 9:44 a.m., <b>George Kiagiadakis</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 show a popup for each message, yes. This is what empathy is doing and so far I like it. I think kopete also does that. Note that popups normally group together and autohide after a while, so I think it is fine from a usability pov. And clicking respond on any of them will make them all disappear.
Starting a chat in composing state should not happen imho. If the CM signals it, I would consider this a CM bug. It is just wrong to show anything to the user if there is still a chance that this channel will not receive any messages.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If I may interject here, showing just one popup and appending the messages there is IMHO better. Think about it in this way - you get four or five messages in rather rapid succession, so only one will be shown and the rest will stay hidden. Now if the last one would be shown, you would see the fourth (fifth) message without seeing the previous ones, which is not right. It works better for displaying the first message and hiding the rest, but then why even show the rest when they will autohide all together (because of the rapid succession, they'll all have almost the same timeout). Therefore, it's much better to display just one popup and append the messages there.</pre>
<br />
<p>- Martin</p>
<br />
<p>On December 9th, 2010, 8:27 p.m., George Kiagiadakis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/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 George Kiagiadakis.</div>
<p style="color: grey;"><i>Updated 2010-12-09 20:27:51</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;">This is the initial implementation of the approver. It can approve p2p text chats only, for now. It is build as a kded module. When a new message arrives, it shows a knotify popup, it plays a sound and shows a KStatusNotifierItem flashing in the tray.
gitweb url:
http://gitweb.kde.org/clones/telepathy-approver/gkiagia/telepathy-approver.git/shortlog/refs/heads/implementation
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Tested with empathy as the chat handler. Currently the "respond" button on the popup doesn't seem to work, but I blame knotify's crappiness for that. Clicking on the flashing icon works perfectly, though.
</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>CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/approverdaemon.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/approverdaemon.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/channelapprover.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/channelapprover.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/dispatchoperation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/dispatchoperation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/handlewithcaller.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/handlewithcaller.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/telepathy_kde_approver.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/telepathy_kde_approver.notifyrc <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/textchannelapprover.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/textchannelapprover.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/tpkdeapprovermodule.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100199/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>