<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/102804/">http://git.reviewboard.kde.org/r/102804/</a>
</td>
</tr>
</table>
<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/102804/diff/3/?file=38471#file38471line130" style="color: black; font-weight: bold; text-decoration: underline;">telepathy-module.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">TelepathyModule::~TelepathyModule()</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">130</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">m_pluginStack</span><span class="p">.</span><span class="n">top</span><span class="p">()</span> <span class="o">!=</span> <span class="n">plugin</span><span class="p">)</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;">You've not really addressed any of my comments here, and this code is somewhat crucial.
At least add a check for double insert. It can easily happen as you don't always remove directly when one becomes inactive.
just a simple
if (active) {
if (m_pluginStack.contains(m_plugin) {
return;
}
I'm not sure what would happen if you it's in the stack twice, but it's not ideal.
I'd like to see this loop tidied up too (as suggested last review), but I'm happy to let that slide till afterwards.
</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On October 9th, 2011, 6:23 p.m., Martin Klapetek 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 Martin Klapetek.</div>
<p style="color: grey;"><i>Updated Oct. 9, 2011, 6:23 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;">Quite a big rewrite of the KDED module. The main changes are:
1/ Use stacking system for plugins
--each plugin has its own priority, if the priority is higher than what's currently in stack, the plugin is placed on top, if it's lower, it's placed below (maintaining the order by priorities)
2/ Plugins now use this sort-of API
--when any plugin wants to set a presence, it signals active(true); which is connected to the module's core where it checks the stack and either uses that plugin's presence or places it somewhere down the stack
3/ Plugins all have common base class
4/ 'Now listening...' aka mpris plugin was gifted with some refactoring as well</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;">1/ Works
2/ Works
3/ Works too, obviously
4/ Still not with configurable message, but works</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">(143afef)</span></li>
<li>autoaway.h <span style="color: grey">(a95f49a)</span></li>
<li>autoaway.cpp <span style="color: grey">(812ba97)</span></li>
<li>global-presence.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>global-presence.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>telepathy-kded-module-plugin.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>telepathy-kded-module-plugin.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>telepathy-module.h <span style="color: grey">(815ee38)</span></li>
<li>telepathy-module.cpp <span style="color: grey">(174cecf)</span></li>
<li>telepathy-mpris.h <span style="color: grey">(5dd596f)</span></li>
<li>telepathy-mpris.cpp <span style="color: grey">(b4920d8)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102804/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>