Review Request 123654: Improve performance of KFileItem and KDirSortFilterProxyModel

[Milian Wolff]

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


generally +1 from my side except for the open question for the case of displayName != fileName. and probably someone else with more knowledge of what's going on should give the final +2.


autotests/kdirsortfilterproxymodel_benchmark.cpp (line 45)
<https://git.reviewboard.kde.org/r/123654/#comment54849>

    should this be part of the benchmark? can't you set it once and switch between sort columns? i.e. I'd expect this call to be outside the QBENCHMARK scope



src/core/kfileitem.cpp (line 1115)
<https://git.reviewboard.kde.org/r/123654/#comment54850>

    what about this comment, it seems there are cases where the filename is different from the display name sometimes and the old code always used the filename. is that comment obsolete? or simply not applicable to the current directory which always has fileName() == m_strName ?



src/filewidgets/kdirsortfilterproxymodel.cpp (line 48)
<https://git.reviewboard.kde.org/r/123654/#comment54851>

    is this check required, i.e. shouldn't the collator code in Qt handle it and do a no-op if nothing changes?


- Milian Wolff


On May 6, 2015, 2:35 a.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123654/
> -----------------------------------------------------------
> 
> (Updated May 6, 2015, 2:35 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> Reduces some operations in KDirSortFilterProxyModel.
> Removes usage of QUrl::fileName() in KFileItem::isHidden(), it comprised 75% of the time spent running the benchmark in here.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 010074d 
>   autotests/kdirsortfilterproxymodel_benchmark.cpp PRE-CREATION 
>   src/core/kfileitem.cpp 344ac67 
>   src/filewidgets/kdirsortfilterproxymodel.cpp 22ac025 
> 
> Diff: https://git.reviewboard.kde.org/r/123654/diff/
> 
> 
> Testing
> -------
> 
> Everything seems to be working still, including tests.
> 
> There was a comment about trash:/ triggering an assert, but I couldn't reproduce.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150506/f71bf36f/attachment.html>