<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 14th, 2011, 9:19 p.m., <b>David Edmundson</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;">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>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) I was doubting about that one but you're right. I'll change those.
2) Sounds good to me. I updated all plugins already though. (was done in like 5 minutes). Shall i revert those changes and edit AbstractAccountUiPlugin instead?</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 14th, 2011, 9:19 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/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="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>
</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 suggest we leave it there until we make a release. I already found a parameter that get's hidden in the Gabble plugin that shouldn't be hidden. The other option would be to write an automated test case..</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 14th, 2011, 9:19 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/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="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>
</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;">setServiceName and setEnabled are both pending operation. Can i do them at once? Otherwise it'd be chain of pending set operations. Doesn't look any neater to me.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 14th, 2011, 9:19 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/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="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>
</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;">Good to know. Thanks</pre>
<br />
<p>- Thomas</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>