D24723: Fix slideshow crashing in invalidate()
davidre created this revision. davidre added a reviewer: Plasma. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidre requested review of this revision. REVISION SUMMARY QSortFilterProxyModel uses std::stable_sort internally which requires that the comparison function generates a strict weak ordering. Returning true or false randomly didn't fullfil this requirement causing a crash in some calls to invalidate. To keep the random order consistent a vector of row indices is used which records the current random order. BUG: 413018 FIXED-IN: 5.17.1 TEST PLAN To reproduce the bug use a slideshow in random order with few pictures and a small time intervall. REPOSITORY R120 Plasma Workspace BRANCH Plasma/5.17 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidre updated this revision to Diff 68115. davidre added a comment. Need rebuild here, rowCount could have changed REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24723?vs=68114&id=68115 BRANCH Plasma/5.17 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidre updated this revision to Diff 68117. davidre added a comment. Simplify the logic. In configmode we don't need the random order. We just use the order of the underlying model. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24723?vs=68115&id=68117 BRANCH Plasma/5.17 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
broulik added inline comments.
INLINE COMMENTS
> slidefiltermodel.cpp:51
> +if(sourceModel) {
> +connect(sourceModel, &QAbstractItemModel::rowsInserted, this,
> &SlideFilterModel::invalidate);
> +connect(sourceModel, &QAbstractItemModel::rowsRemoved, this,
> &SlideFilterModel::invalidate);
Perhaps only `invalidate` if order is actually set to random?
I am actually not sure we need to invalidate at all when inserting items,
unless you want to shuffle every time that happens? Perhaps all we need to do
is shrink/grow the random vector when new items are added?
> slidefiltermodel.cpp:121
> +{
> +m_randomOrder.resize(sourceModel()->rowCount());
> +std::iota(m_randomOrder.begin(), m_randomOrder.end(), 0);
Can `sourceModel()` be null here?
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D24723
To: davidre, #plasma
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidre added inline comments.
INLINE COMMENTS
> slidefiltermodel.cpp:51
> +if(sourceModel) {
> +connect(sourceModel, &QAbstractItemModel::rowsInserted, this,
> &SlideFilterModel::invalidate);
> +connect(sourceModel, &QAbstractItemModel::rowsRemoved, this,
> &SlideFilterModel::invalidate);
Good point. No need to invalidate in the other cases.
Maybe. It depends where images are added in the model? Maybe we would end up
showing pictures again because the indexes have shifted? Or the new images are
always shown at the end because they get appended? Either way I think that*'s
not to bad if the order is set to Random. The result could also be the result
of a random permutation.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D24723
To: davidre, #plasma
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidedmundson added a comment. Other than Kai's comments, ship it. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24723 To: davidre, #plasma Cc: davidedmundson, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidre updated this revision to Diff 68302. davidre marked 3 inline comments as done. davidre added a comment. Like this? I also changed the condition in image.cpp otherwise the order wouldn't have been random on the first run with these changes. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24723?vs=68117&id=68302 BRANCH Plasma/5.17 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/image.cpp wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma Cc: davidedmundson, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
davidre updated this revision to Diff 68304. davidre added a comment. Build the new order only after setting the source model. In invalidate just shuffling is enough. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24723?vs=68302&id=68304 BRANCH Plasma/5.17 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/image.cpp wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma Cc: davidedmundson, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D24723: Fix slideshow crashing in invalidate()
This revision was automatically updated to reflect the committed changes. Closed by commit R120:a1cf305ffb21: Fix slideshow crashing in invalidate() (authored by davidre). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24723?vs=68304&id=68407 REVISION DETAIL https://phabricator.kde.org/D24723 AFFECTED FILES wallpapers/image/image.cpp wallpapers/image/slidefiltermodel.cpp wallpapers/image/slidefiltermodel.h To: davidre, #plasma, broulik Cc: davidedmundson, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
