<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/111308/">http://git.reviewboard.kde.org/r/111308/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Watch out for personitem.cpp, looks like you have my patch applied.</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/111308/diff/2/?file=166886#file166886line68" style="color: black; font-weight: bold; text-decoration: underline;">src/persondata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">PersonData::PersonData(QObject *parent)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">PersonDataPtr PersonData::createFromUri(const QUrl& uri)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">67</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">PersonData</span><span class="o">::</span><span class="n">PersonData</span><span class="p">(</span><span class="n">QObject</span> <span class="o">*</span><span class="n">parent</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">67</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">PersonDataPtr</span> <span class="n">PersonData</span><span class="o">::</span><span class="n">createFromUri</span><span class="p">(</span><span class="k">const</span> <span class="n">QUrl</span><span class="o">&</span> <span class="n">uri</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;">& uri -> &uri</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/111308/diff/2/?file=166886#file166886line77" style="color: black; font-weight: bold; text-decoration: underline;">src/persondata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">PersonData::PersonData(const QString &uri, QObject *parent)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">PersonDataPtr PersonData::createFromContactId(const QString& contactId)</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">PersonData</span><span class="o">::</span><span class="n">PersonData</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">uri</span><span class="p">,</span> <span class="n">QObject</span> <span class="o">*</span><span class="n">parent</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">74</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">PersonDataPtr</span> <span class="n">PersonData</span><span class="o">::</span><span class="n">createFromContactId</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span><span class="o">&</span> <span class="n">contactId</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;">& contacItd -> &contactId</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/111308/diff/2/?file=166886#file166886line142" style="color: black; font-weight: bold; text-decoration: underline;">src/persondata.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void PersonData::setContactId(const QString &id)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void PersonData::loadContact(const QString &id)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">131</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">uri</span> <span class="o">=</span> <span class="n">uri</span><span class="p">;</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">127</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">uri</span> <span class="o">=</span> <span class="n">uri</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;">I know it's the original code, but I think this could also use "if (d->uri == uri) { return; }</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/111308/diff/2/?file=166898#file166898line175" style="color: black; font-weight: bold; text-decoration: underline;">src/widgets/plugins/mergecontactswidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 MergeContactsWidget::onMergeButtonPressed()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">175</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_model</span><span class="o">-></span><span class="n">createPersonFromIndexes</span><span class="p">(</span><span class="n">indexToMergeConverted</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">175</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1"><span class="hl">// </span> m_model->createPersonFromIndexes(indexToMergeConverted);</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;">This shouldn't be part of this patch (it's fixed in my branch)</pre>
</div>
<br />
<p>- Martin</p>
<br />
<p>On June 30th, 2013, 10:33 p.m. UTC, David Edmundson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy, Aleix Pol Gonzalez and Vishesh Handa.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated June 30, 2013, 10:33 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;">Disallow changing URI of PersonData after creation
Currently setUri and setContactId are public which would cause a lot of bugs if changed.
Instead replace with two clear static methods; loadFromUri and loadFromContactId.
In practice they were never used more than once.
A declarative wrapper using QDeclarativeParserStatus provides the same functionality
in QML. A wrapper is needed anyway as it means we can now put more complex data types in PersonData.
bugs that used to exist which are no longer relevant:
- every single plugin for PersonsViewer does not respond to uriChanged
- personsactionsmodel did not respond to uriChanged()
- contactId would return the last user set with setContactId not the current contacts ID (which is misleading/wrong anyway, as a person can have multiple IDs)
- isPerson property was not updated when changed
- PeronDetailsView kept an invalid empty PersonData on construction with a lifespan of the parent.. for no reason.
Given we want people to write plugins, we need to make it easier.
PersonData code is also simpler as it doesn't have to be careful about wiping old resources in setUri.
Downside is you can't have a PersonData on the stack. Bit of a effort for the unit tests.. but not in the real world I don't think.
I believe someone (vishesh?) also suggested this at the Pineda sprint. </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/abstractpersonplugin.h <span style="color: grey">(60508b693fd6406b77f4622bff4c4419fb13c50e)</span></li>
<li>src/abstractpersonplugin.cpp <span style="color: grey">(3a41f3bcff668994de70d77c77fd5a4a061dadde)</span></li>
<li>src/autotests/tests/persondatatests.cpp <span style="color: grey">(623fb5f752cca6d98aa261a72a34b740d74b92e8)</span></li>
<li>src/declarative/CMakeLists.txt <span style="color: grey">(9b33b85429c65ce832f37edecebf2ecdf467c476)</span></li>
<li>src/declarative/declarativepersondata.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/declarative/declarativepersondata.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/declarative/peopleqmlplugin.cpp <span style="color: grey">(c991064e011f5115683adb90955c0e3a5dbd156c)</span></li>
<li>src/examples/personwidget.cpp <span style="color: grey">(bc2eaf2805891a201fbbf8cb20420764652e34c7)</span></li>
<li>src/personactionsmodel.cpp <span style="color: grey">(458006213442bcd24bb08216594aed1deb7fb171)</span></li>
<li>src/persondata.h <span style="color: grey">(177f2451c5c432d3cb6add454716bd292540bb7f)</span></li>
<li>src/persondata.cpp <span style="color: grey">(2341bd4f4215bcc2a671f75a118c488e870c98ac)</span></li>
<li>src/personitem.h <span style="color: grey">(91fcd3cc6558625622d17b02805cc4f04382c51d)</span></li>
<li>src/personitem.cpp <span style="color: grey">(6086e41afbd028d830c6a3771dad9fd69d3627cc)</span></li>
<li>src/personpluginmanager.h <span style="color: grey">(38f13816485e69a4a2acab45b73c978d6448be1a)</span></li>
<li>src/personpluginmanager.cpp <span style="color: grey">(487f1bbde1cf122b4c475d8a33bc7514bc9bcc43)</span></li>
<li>src/personsmodel.h <span style="color: grey">(4b6a829d5c3b62e7f057fd15c821460b1e8c5df3)</span></li>
<li>src/personsmodel.cpp <span style="color: grey">(813d0afd6add7d8c33a9d08729ce9c5acadc421f)</span></li>
<li>src/plugins/core/emailplugin.h <span style="color: grey">(ebffe6451b0a8cd74d934227e1f771643acab402)</span></li>
<li>src/plugins/core/emailplugin.cpp <span style="color: grey">(83babd4aa71627d6172794a6878a15b0738ad15e)</span></li>
<li>src/resourcewatcherservice.h <span style="color: grey">(94210ef7ac754eebd324c3d55c391378e1dc07d9)</span></li>
<li>src/resourcewatcherservice.cpp <span style="color: grey">(1342c99a597a3203c17aeb187ec2740af1e0ed00)</span></li>
<li>src/widgets/persondetailsview.cpp <span style="color: grey">(f655eb2ec0a16f00239b679263354632a6183fee)</span></li>
<li>src/widgets/plugins/mergecontactswidget.cpp <span style="color: grey">(51e11dc8d2457297336616e6ee211493b0f5fa08)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111308/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>