<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/114349/">http://git.reviewboard.kde.org/r/114349/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 8th, 2013, 10:28 a.m. UTC, <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;">On the ETM model change, I struggle to believe that Akonadi fetching the data then building a model only for us to then scrape the model for the data is going to be more efficient than us just loading the data.</pre>
</blockquote>
<p>On December 8th, 2013, 11:24 a.m. UTC, <b>Dan Vrátil</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;">We don't scrape the model anywhere! The model lives as long as the plugin lives, which is for the entire application lifetime (in PersonsPluginManager).
Assuming you use PersonsModel, the full list has been loaded anyway, as AkonadiAllContacts monitor has been requested, which loads and caches the whole list in memory. We actually optimize by sharing the contact from the model between AkonadiAllContacts and AkonadiContact - which means that requesting AkonadiContact is bit faster, as it does not have to query Akonadi for the contact, but just looks it up in the model.
</pre>
</blockquote>
<p>On December 8th, 2013, 11:29 a.m. UTC, <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;">"Assuming you use PersonsModel"
That's the point - we can't assume the user will have a personsmodel. For a lot of cases they won't.</pre>
</blockquote>
<p>On December 8th, 2013, 3:34 p.m. UTC, <b>Dan Vrátil</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;">Ah, I see now. I haven't realized that there's a usecase for the class outside PersonsModel.
So what about we have one Akonadi::Monitor in AkonadiAllContacts (like it's now) and one Akonadi::Monitor in AkonadiDataSource that will monitor all AkonadiContacts. AkonadiContacts would simply ignore the notification if it's for a different item. This would still solve the problem with too many monitors, while not having the model.</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;">To confirm that we're both thinking the same thing:
AkonadiAllContacts is the same
AkonadiContacts doesn't have a monitor itself.
AkonadiDataSource owns one monitor
AkonadiContact gets the monitor passed as an argument in createContatMonitor
- runs monitor->setItemMonitored(item, true) in the AkonadiContact constructor
- runs monitor->setItemMonitored(item, false) in the AkonadiContact destructor
(these are ref counted so we would never get two AkonadiContacts on the same ID, so this is safe)
AkonadiContact checks the item changed is the right one before emitting signals.
</pre>
<br />
<p>- David</p>
<br />
<p>On December 8th, 2013, 2:48 a.m. UTC, Dan Vrátil 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.</div>
<div>By Dan Vrátil.</div>
<p style="color: grey;"><i>Updated Dec. 8, 2013, 2:48 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
libkpeople
</div>
<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;">Use a single ETM instead of manually fetching all contacts and then installing a new Akonadi Monitor for each single AkonadiContact.
ETM (with EntityMimeTypeFilterModel on top) will do the CollectionFetchJobs and ItemFetchJobs and data handling for us, which only leaves us with handling of changes, which we do in slots for QAbstractItemModel's rowsInserted(), rowsRemoved() and dataChanged() signals. This is much better than having many Akonadi monitors, because for each change notification that Akonadi server generates, it has to iterate over all registered monitors and compare the notification against each monitor's filter. With lots of monitors, this can impact performance of the server. Although the AkonadiContacts with their Akonadi monitors are created on demand (so there's none at the beginning), they are not removed (probably cached somewhere in PersonsModel), so the number of active Akonadi monitors tends to grow - for instance going through all contacts in PersonViewer will leave you with one monitor for each contact fed from Akonadi.
>From Akonadi POV, having one properly configured monitor (hidden in ETM) and working with data via ETM is a better approach.</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;">Going through all contacts in PersonViewer includes all contacts from Akonadi, and the application uses only one Akonadi Monitor all the time.</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">(889e96d)</span></li>
<li>src/plugins/akonadi/akonadidatasource.h <span style="color: grey">(b09edf8)</span></li>
<li>src/plugins/akonadi/akonadidatasource.cpp <span style="color: grey">(193de78)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/114349/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>