Merge lp:~chihchun/unity-scopes-shell/lp1535377 into lp:unity-scopes-shell

Proposed by Rex Tsai
Status: Rejected
Rejected by: Paweł Stołowski
Proposed branch: lp:~chihchun/unity-scopes-shell/lp1535377
Merge into: lp:unity-scopes-shell
Diff against target: 15 lines (+3/-2)
1 file modified
src/Unity/resultsmodel.cpp (+3/-2)
To merge this branch: bzr merge lp:~chihchun/unity-scopes-shell/lp1535377
Reviewer Review Type Date Requested Status
Paweł Stołowski Disapprove
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+285123@code.launchpad.net

Commit message

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

The new result could be larger then current result list, moving the results to a bigger then qlist size will result null elements, which cause oldResultsMap.rebuild failure.

Description of the change

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Thanks for looking into this. As discussed on IRC though, I think that while your fix likely avoids the crash, it probably hides the real problem at the same time, plus I think that getting rid of "row + (row > oldPos ? 1 : 0)" in the beginMoveRows notification for the qml view will cause subtle issues.

It's expected and absolutely fine that the size of new results list is different than current list; as the new results list is iterated in the 0..results.count() loop, new (previously unseen) results are inserted into m_lists (so it grows) and it's guaranteed that any moves happen only in the 0..row range, which is valid for m_results list. That's the theory at least, I cannot exclude a bug in this implementation.

I'm currently working on a branch which has a bunch of stress tests for model updates (it wasn't part of the existing code, because I started working on them just recently for some other changes in that area) and am trying to come up with a case that would fail with existing implementation, giving me the opportunity to better understand what happens.

review: Abstain
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Rejecting as I think the other MP is the proper fix. My MP is currently in silo 80 and confirmed by Rex to fix the issue.

review: Disapprove

Unmerged revisions

282. By Rex Tsai

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

The new result could be larger then current result list,
moving the results to a bigger then qlist size will result
null elements, which cause oldResultsMap.rebuild failure.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Unity/resultsmodel.cpp'
2--- src/Unity/resultsmodel.cpp 2015-11-30 09:23:32 +0000
3+++ src/Unity/resultsmodel.cpp 2016-02-04 21:38:34 +0000
4@@ -110,8 +110,9 @@
5 if (hadBefore) {
6 if (row != oldPos) {
7 // move row
8- beginMoveRows(QModelIndex(), oldPos, oldPos, QModelIndex(), row + (row > oldPos ? 1 : 0));
9- m_results.move(oldPos, row);
10+ int pos = (row >= m_results.count()) ? (m_results.count() - 1) : row;
11+ beginMoveRows(QModelIndex(), oldPos, oldPos, QModelIndex(), pos);
12+ m_results.move(oldPos, pos);
13 oldResultsMap.rebuild(m_results);
14 endMoveRows();
15 }

Subscribers

People subscribed via source and target branches

to all changes: