<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/102193/">http://git.reviewboard.kde.org/r/102193/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 3rd, 2011, 12:06 p.m., <b>David Edmundson</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/102193/diff/1/?file=30664#file30664line423" style="color: black; font-weight: bold; text-decoration: underline;">lib/chat-widget.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 ChatWidget::onKeyPressed(QKeyEvent* e)</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">391</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">ChatWidget</span><span class="o">::</span><span class="n">toggleSelection</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;">As I read this, you're not really toggling a selection. Only removing it.
So rename this. removeActiveSelection?
Also this is all very weird:
If I'm reading this correctly
If the sender is the chatArea
clear the sendMessageBox
if the sender is the sendMessageBox
clear the chatArea's search.
Why not just connect the signal from the two sources to two different slots?
For bonus points, make these slots in the relevant classes, rather than here.</pre>
</blockquote>
<p>On August 4th, 2011, 12:06 a.m., <b>Jan Gerrit Marker</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;">This is the main point of the idea to make the two widgets (chatArea and sendMessageBox) behave like one:
If chatArea changes its selection then sendMessageBox needs to clear its selection in order to have only one selection.
I named it toggleSelection() as it changes the widget which has a selection. I'm open to name suggestions...
The slots are in the ChatWidget class as I thought that it would be better to let the ChatWidget control the chatArea and the sendMessageBox if I want to create the user experience of using one widget.</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;">Ok, I see what it's doing now. That makes a lot of sense.
I would still rather see this as two slots (no matter which class this code is in), and then a sensible comment (just copy and paste what you just typed above) where you make the connections to explain it. It makes things a /lot/ simpler, and is less prone to break if anyone edits either the sendMessageBox or the search. </pre>
<br />
<p>- David</p>
<br />
<p>On August 3rd, 2011, 10:45 a.m., Jan Gerrit Marker 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 Jan Gerrit Marker.</div>
<p style="color: grey;"><i>Updated Aug. 3, 2011, 10:45 a.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;">The patch was created with the purpose to enable text copying by pressing Ctrl+C. This was more complicated than I thought so I tried to implement a new concept of handling the keys:
My goal was to let the line edit and the view behave like there were one widget regarding text selection. If text in the line edit is marked there is no text marked in the view and vice versa. In order to achieve this I let the widget which contains both handle the key events by connecting to a signal in the two classes which get's emitted when a key is pressed.</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;">I tested it for a day and there were no problems.</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>lib/adium-theme-view.h <span style="color: grey">(0507128)</span></li>
<li>lib/adium-theme-view.cpp <span style="color: grey">(0eb1090)</span></li>
<li>lib/chat-text-edit.h <span style="color: grey">(c086010)</span></li>
<li>lib/chat-text-edit.cpp <span style="color: grey">(ea96034)</span></li>
<li>lib/chat-widget.h <span style="color: grey">(06e98a1)</span></li>
<li>lib/chat-widget.cpp <span style="color: grey">(534ee1c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102193/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>