<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/100775/">http://git.reviewboard.kde.org/r/100775/</a>
</td>
</tr>
</table>
<br />
<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 like what it's doing. It's a very clever idea in terms of usability.
I have a few comments with regards to one line of code.</pre>
<br />
<div>
<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/100775/diff/1/?file=10383#file10383line44" style="color: black; font-weight: bold; text-decoration: underline;">haze/skype-main-options-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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QDir</span> <span class="o">*</span><span class="n">home</span> <span class="o">=</span> <span class="k">new</span> <span class="n">QDir</span><span class="p">(</span><span class="n">QString</span><span class="p">(</span><span class="s">"%1/.Skype"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">QDir</span><span class="o">::</span><span class="n">homePath</span><span class="p">()));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If you ever write "new" you should check it is deleted or has a parent. In this case it gets leaked (bad!).
You may as well keep the QDir on the stack. Putting things on the stack is faster, and they get automatically 'deleted' when the function finishes.
i.e
QDir home(QString(%1/.Skype).arg(QDir::homePath())));
You only need to write "new FooBar" if you need the lifespan of the object to last longer than the method.
----
"home" isn't the best name for this variable, you're not in the home directory. You're in a Skype config folder.
----
Also is this clearer to read, rather than using string manipulation?:
QDir skypeConfigDir(QDir::home().filePath(".Skype");
</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On March 1st, 2011, 7:18 p.m., Florian Reinhard 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 Florian Reinhard.</div>
<p style="color: grey;"><i>Updated March 1, 2011, 7:18 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;">* add autocomplete for skype account names based on foldernames in $HOME/.Skype/</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;">* works with two accounts in ~/.Skype/
* does nothing if there is no ~/.Skype/ </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>haze/skype-main-options-widget.cpp <span style="color: grey">(572804f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100775/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>