<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/100389/">http://git.reviewboard.kde.org/r/100389/</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;">All the changes to the profile selection widget and use of the profilemanager look great. Except for a few minor review comments, mostly about how Tp-Qt4 works.
Your changes to the plugin interface I'm less convinced by.
1) I think we've mixed up "profile" and "serviceName" here.
2) The interfaces changes weren't what I was expecting:
I was imagining,
virtual AbstractAccountUi* accountUi(const QString &connectionManager, const QString &protocol); in AbstractAccountUiPlugin changing to have a third parameter called serviceName.
This then returns a different AbstractAccountUi per service which can be different classes.
If a plugin wants to use the same AbstractAccountUi for all services and just make minor tweaks, this can be done at a plugin level.
The GabbleAccountUi class can take an extra parameter (serviceName), without the superclass changing.
This means we don't have to update /all/ the plugins with a parameter that they don't use.
Especially as I think gabble is going to be the only one with multiple services.
</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/100389/diff/1/?file=6972#file6972line46" style="color: black; font-weight: bold; text-decoration: underline;">src/KCMTelepathyAccounts/abstract-account-parameters-widget.h</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; ">typedef QMap<Tp::ProtocolParameter, QWidget*> ParametersWidgetsMap;</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">46</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">profile</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;">It's not really a profile you're passing,
a profile is a set of:
connectionManager, protocol and serviceName. I would say this is a serviceName.
Unfortunately that's quite a big change...</pre>
</div>
<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/100389/diff/1/?file=6973#file6973line119" style="color: black; font-weight: bold; text-decoration: underline;">src/KCMTelepathyAccounts/abstract-account-parameters-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; ">void AbstractAccountParametersWidget::handleParameter(const QString &parameterName,</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">119</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">kDebug</span><span class="p">()</span> <span class="o"><<</span> <span class="s">"WARNING: Field"</span> <span class="o"><<</span> <span class="n">parameterName</span> <span class="o"><<</span> <span class="s">"hidden"</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;">whilst I agree we should have a debug line here, it's not really a warning.
Having 'missing' parameters is sort of expected if a user is using an older version of the connection manager than the newest connection manager we support. It's not really a problem because we handle it.</pre>
</div>
<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/100389/diff/1/?file=6977#file6977line85" style="color: black; font-weight: bold; text-decoration: underline;">src/KCMTelepathyAccounts/account-edit-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="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">77</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">ui</span><span class="o">-></span><span class="n">iconLabel</span><span class="o">-></span><span class="n">setPixmap</span><span class="p">(</span><span class="n">KIcon</span><span class="p">(</span><span class="n"><span class="hl">info</span></span><span class="p"><span class="hl">.</span></span><span class="n">iconName</span><span class="p">()).</span><span class="n">pixmap</span><span class="p">(</span><span class="mi">32</span><span class="p">));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">85</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">ui</span><span class="o">-></span><span class="n">iconLabel</span><span class="o">-></span><span class="n">setPixmap</span><span class="p">(</span><span class="n">KIcon</span><span class="p">(</span><span class="n"><span class="hl">profile</span></span><span class="p"><span class="hl">.</span></span><span class="n"><span class="hl">data</span></span><span class="p"><span class="hl">()</span></span><span class="o"><span class="hl">-></span></span><span class="n">iconName</span><span class="p">()).</span><span class="n">pixmap</span><span class="p">(</span><span class="mi">32</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 don't need .data() here.</pre>
</div>
<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/100389/diff/1/?file=7003#file7003line196" style="color: black; font-weight: bold; text-decoration: underline;">src/add-account-assistant.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; ">void AddAccountAssistant::accept()</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">168</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">properties</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="s">"org.freedesktop.Telepathy.Account.Service"</span><span class="p">,</span> <span class="n">d</span><span class="o">-></span><span class="n">currentProfileItem</span><span class="o">-></span><span class="n">serviceName</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;">don't do this. We have nice APIs so that we don't have to write dbus paths ourselves.
account->setSericeName();
account->setEnabled</pre>
</div>
<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/100389/diff/1/?file=7004#file7004line73" style="color: black; font-weight: bold; text-decoration: underline;">src/edit-account-dialog.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; ">EditAccountDialog::EditAccountDialog(AccountItem *item, QWidget *parent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">73</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">widget</span> <span class="o">=</span> <span class="k">new</span> <span class="n">AccountEditWidget</span><span class="p">(</span><span class="n">pro<span class="hl">tocolInfo</span></span><span class="p">,</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">73</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">widget</span> <span class="o">=</span> <span class="k">new</span> <span class="n">AccountEditWidget</span><span class="p">(</span><span class="n"><span class="hl">d</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">item</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">account</span></span><span class="p"><span class="hl">().</span></span><span class="n"><span class="hl">data</span></span><span class="p"><span class="hl">()</span></span><span class="o"><span class="hl">-></span></span><span class="n">pro<span class="hl">file</span></span><span class="p"><span class="hl">()</span>,</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;">don't write ".data()->"
AccountPtr has overloaded the "->" operator, so you only need to
d->item->account()->profile();</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On January 14th, 2011, 7:59 p.m., Thomas Richard 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 Thomas Richard.</div>
<p style="color: grey;"><i>Updated Jan. 14, 2011, 7:59 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;">This change makes the accounts kcm use profiles. In the near future, distros should start shipping profiles.
This will make it possible to show 'Google talk' and 'Facebook chat' in the new accounts dialog instead of 'Jabber/XMPP/Google Talk'.
It also allows to do small modifications in the plugins, depending on the selected profile.
It can provide different default parameters for different profiles.
Last but not least, the icons used can (or must) be specified in the profile.</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;">Compiling
Adding new accounts
Editing accounts
Added the sample profile from:
http://telepathy.freedesktop.org/wiki/service-profile-v1
Everything seems to work as seen in the screenshots</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>src/KCMTelepathyAccounts/CMakeLists.txt <span style="color: grey">(1e3a282f64bc66a18765a0499492296c222e94dd)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-parameters-widget.h <span style="color: grey">(6071de379f1733b4428f4f3363a664993a3e0e15)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp <span style="color: grey">(fb29cc3fc95c3543bd2dda66d474a7ebba19c94a)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-ui.h <span style="color: grey">(a86ce6945150b5c72683ec31ca953b0128c0d3c5)</span></li>
<li>src/KCMTelepathyAccounts/abstract-account-ui.cpp <span style="color: grey">(500452b7dc85535483f8eecce0b99eed5c582132)</span></li>
<li>src/KCMTelepathyAccounts/account-edit-widget.h <span style="color: grey">(95e29238ffd83530a6005c290c1e3e8ed00d87c0)</span></li>
<li>src/KCMTelepathyAccounts/account-edit-widget.cpp <span style="color: grey">(224dab9e40c282b20db69287f8c59a4c04b84d07)</span></li>
<li>src/KCMTelepathyAccounts/connection-manager-item.h <span style="color: grey">(aa3480eeecff4be8433d5a11236c40f1ffbbbe16)</span></li>
<li>src/KCMTelepathyAccounts/connection-manager-item.cpp <span style="color: grey">(91eea62bd4e0fbf2a746424d99a025ef4d4261d3)</span></li>
<li>src/KCMTelepathyAccounts/dictionary.cpp <span style="color: grey">(2eeff58ee489fe1b95c46d9c53e83ce42afc0ae4)</span></li>
<li>src/KCMTelepathyAccounts/generic-advanced-options-widget.h <span style="color: grey">(ac5ff029ba8e86e0adf6f6a3988609d5c1ae6665)</span></li>
<li>src/KCMTelepathyAccounts/generic-advanced-options-widget.cpp <span style="color: grey">(0e39b47fbd214ef66b3af1732bf7dc0c1858a8c4)</span></li>
<li>src/KCMTelepathyAccounts/parameter-edit-model.h <span style="color: grey">(44c9d4dcc5300f0734c2c7aa12e4d0d050b48aec)</span></li>
<li>src/KCMTelepathyAccounts/parameter-edit-model.cpp <span style="color: grey">(d64fea0532f0b3a615ee69aa3236d32582c1a839)</span></li>
<li>src/KCMTelepathyAccounts/parameter-edit-widget.cpp <span style="color: grey">(34e161a48c63c02b35f4660af9826e9a09387051)</span></li>
<li>src/KCMTelepathyAccounts/parameter-item.cpp <span style="color: grey">(65e4334de625804c3277424dbca28b5f2d4dbf28)</span></li>
<li>src/KCMTelepathyAccounts/profile-item.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-item.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-list-model.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-list-model.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-select-widget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-select-widget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/profile-select-widget.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/KCMTelepathyAccounts/protocol-item.h <span style="color: grey">(759956e11e05e57ccafb2baae3314816b9122b49)</span></li>
<li>src/KCMTelepathyAccounts/protocol-item.cpp <span style="color: grey">(2a6844349ee8e16b4e8c6a9d274c77d51ffb69fb)</span></li>
<li>src/KCMTelepathyAccounts/protocol-list-model.h <span style="color: grey">(3718e8fc0b4093ab70e4528efecdf3538abb0825)</span></li>
<li>src/KCMTelepathyAccounts/protocol-list-model.cpp <span style="color: grey">(168c6c57c2db993a1d71f56678e2fbaa061aa953)</span></li>
<li>src/KCMTelepathyAccounts/protocol-select-widget.h <span style="color: grey">(ca0bd8a8468a76b89f24b477c050c39acba6aa0b)</span></li>
<li>src/KCMTelepathyAccounts/protocol-select-widget.cpp <span style="color: grey">(f53c5b7529f288efec0b9c1bd232d34b6b380a60)</span></li>
<li>src/KCMTelepathyAccounts/protocol-select-widget.ui <span style="color: grey">(d6aee67d461c574f139ebf74908acaecd193a0e2)</span></li>
<li>src/account-item.cpp <span style="color: grey">(a670a1be35bd2edb71683f918756de9edb91a1c8)</span></li>
<li>src/add-account-assistant.h <span style="color: grey">(b16d76b14ff5d871287610ce0be400188f84d3f8)</span></li>
<li>src/add-account-assistant.cpp <span style="color: grey">(69edc1171798f80ee5ac8c5c9853907ff039d76e)</span></li>
<li>src/edit-account-dialog.cpp <span style="color: grey">(979742196c880f34a5df3345b509ffb945328613)</span></li>
<li>src/kcm-telepathy-accounts.cpp <span style="color: grey">(554f6d1d719cca651f86a7df6e9700970552cec6)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100389/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/100389/s/43/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/01/14/profiles1_400x100.png" style="border: 1px black solid;" alt="" /></a>
<a href="http://git.reviewboard.kde.org/r/100389/s/44/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/01/14/profiles2_400x100.png" style="border: 1px black solid;" alt="" /></a>
<a href="http://git.reviewboard.kde.org/r/100389/s/45/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/01/14/profiles3_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>