Review Request 128398: Fix KDescendantsProxyModel::setSourceModel(...) to reset internal data

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128398/#review97444
-----------------------------------------------------------



I'd recommend making a new unittest with simply a QStandardItemModel as source model (and switching to another one).
Use text-based comparisons to check the contents of the proxy model. You can use kextracolumnsproxymodeltest.cpp as an example.

I don't really understand kdescendantsproxymodel_smoketest.cpp either, but it seems to mostly monitor for signals, not really for actual expected outcome of the proxymodel.

- David Faure


On July 8, 2016, 2:58 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128398/
> -----------------------------------------------------------
> 
> (Updated July 8, 2016, 2:58 a.m.)
> 
> 
> Review request for KDE Frameworks, Stephen  Kelly and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> -------
> 
> KDescendantsProxyModel currently does not reset its internal data when a (new) source model is set.
> 
> Not sure the provided patch is the most correct one, but it works with the current unit tests and for the use case where this bug was hit.
> I am still confused why `KDescendantsProxyModelPrivate::synchronousMappingRefresh()` loops over `while (!m_pendingParents.isEmpty())` on calling `processPendingParents();` while `KDescendantsProxyModelPrivate::scheduleProcessPendingParents()` does not.
> Especially when the `KDescendantsProxyModelPrivate::sourceModelReset()` handler also only calls the latter.
> The sourceModelReset handler should be surely similar to what is done on setting a new source model, so the patch for now copies that code. But from what I understood by reading the code both of them should rather do a full loop perhaps?
> 
> And `m_relayouting` should get a better name now, but no idea yet what would be nice.
> 
> I have yet to grasp the proxymodeltest system to also write a matching unit test, any proposal where I should start?
> 
> 
> Diffs
> -----
> 
>   src/kdescendantsproxymodel.cpp 477cd96 
> 
> Diff: https://git.reviewboard.kde.org/r/128398/diff/
> 
> 
> Testing
> -------
> 
> Existing kitemmodels unit tests still pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160715/079cd8c9/attachment-0001.html>