<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#file4967line79" 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; ">void TextChannelApprover::onMessageReceived(const Tp::ReceivedMessage & msg)</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">79</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">notification</span><span class="o">-></span><span class="n">setActions</span><span class="p">(</span><span class="n">QStringList</span><span class="p">()</span> <span class="o"><<</span> <span class="n">i18n</span><span class="p">(</span><span class="s">"Respond"</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">80</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">notification</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">activated</span><span class="p">()),</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">channelAccepted</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;">Maybe the respond button doesn't work because the Respond button is for some reason not considered the default action by knotify. Maybe action1Activated() or activated(unsigned int action) will work...
Also an "Ignore" button should be added here :)</pre>
</blockquote>
<p>On December 10th, 2010, 10:17 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 tried all signals, none of them worked.
About the ignore button... I left it out for the moment because this is a strange case that I am not sure how to handle. If I simply close the channel, the CM will signal it again, with the same messages (according to the spec). To avoid that I have two choices, either confirm all messages and then close the channel or use the Destroyable interface to destroy it. Even if I do that though, if the remote user continues typing, I will receive a new channel. So, there is effectively no way to ignore the chat, if the other user continues sending messages, they will popup. The question is purely a usability one: what would a user expect from the ignore button? I guess I can confirm all the messages and close the chat, just as a shortcut to avoid opening the handler and then close it, but I'm not sure if that's what the user expects.</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;">TpQt4 documentation says: "Approvers wishing to reject channels should call the ChannelDispatchOperation::claim() method, then (if it succeeds) close the channels in any way they see fit."
But then if you just call requestClose() you may be able to drop the messages in the queue at that point, but if you receive another message from the same user, you probably get a new channel to approve.
You might implement the AbstractClientHandler interface to drop all the messages but that's not probably what a user expect.
I'd expect the same behaviour that I would like to see when my status is "Busy":
- No further notification/popup
- No chat opening (so the channel is not supposed to be approved)
- A way to open the chat later with my favourite chat-handler and read all the history
So, an idea that just came to my mind... The text channel should just be accepted, but there should be a method to set the chat handler in passive mode: just accept the message (message delivery status should be accepted but not read), no notification, no chat ui, just a status icon.
(So probably a "block user" button would be more useful than an "ignore" button)
</pre>
<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#file4967line118" 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; ">void TextChannelApprover::onMessageReceived(const Tp::ReceivedMessage & msg)</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">118</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QVariant</span> <span class="n">numMessages</span> <span class="o">=</span> <span class="n">m_notifierItem</span><span class="o">-></span><span class="n">property</span><span class="p">(</span><span class="s">"approver_new_messages_count"</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">119</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">numMessages</span> <span class="o">=</span> <span class="n">QVariant</span><span class="p">(</span><span class="n">numMessages</span><span class="p">.</span><span class="n">toUInt</span><span class="p">()</span> <span class="o">+</span> <span class="mi">1</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">120</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_notifierItem</span><span class="o">-></span><span class="n">setProperty</span><span class="p">(</span><span class="s">"approver_new_messages_count"</span><span class="p">,</span> <span class="n">numMessages</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;">Is this decreased somewhere? If this is static maybe it should count the number of channels to approve, not the number of messages...</pre>
</blockquote>
<p>On December 10th, 2010, 10:07 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;">Hum, good catch, I am not decreasing this iirc. I will fix it.
Why count the number of channels? What should it say then? "You have 2 new channels to approve"? I think the user should not know anything about channels.</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;">I'd say "You have <n> incoming conversations". Then, if you want to show also the number of messages, you can add n lines containing something like "<contact name>: <x> messages"
That's because I see no reason in just a sum of the messages from a random number of conversations in the tooltip. You should show the number of conversations and/or the number of message for each of them.</pre>
<br />
<p>- Daniele Elmo</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>