<table><tr><td style="">dvratil requested changes to this revision.<br />dvratil added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D5667" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Looks good, just some minor details.</p>

<p>Regarding the parent collection being passed separately to itemAdded, I believe that's because of the semantics of the slot. When item is created, it technically does not belong to a collection yet (because the Resource did not store it there remotely yet).</p>

<p>Preferably you should pass the collection to filterItem() as a second argument, I'm not sure if you can rely on Item::parentCollection() in the itemAdded slot. You can use Item::parentCollection() in slotItemChanged() safely though.</p>

<p>For the itemMonitor, you should ensure via ItemFetchScope that the remote identifiers and ancestor are retrieved (the ancestor Retrieval policy should correspond with the the policy of the Resource's ChabgeRecorder ancestor policy).</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R206 KMail</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D5667" rel="noreferrer">https://phabricator.kde.org/D5667</a></div></div><br /><div><strong>To: </strong>mkoller, dvratil, mlaurent<br /><strong>Cc: </strong>KDE PIM, dvasin, ach, winterz, vkrause, mlaurent, knauss, dvratil<br /></div>