Bug 352369 - KSelectionProxyModel bug
Summary: KSelectionProxyModel bug
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kitemmodels
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR crash
Target Milestone: ---
Assignee: Stephen Kelly
URL: https://github.com/milabs/QtSampleMod...
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-06 22:56 UTC by Ilya Matveychikov
Modified: 2016-03-28 05:13 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Matveychikov 2015-09-06 22:56:44 UTC
ASSERT: "m_mappedFirstChildren.isEmpty()" in file
> kitemmodels/kselectionproxymodel.cpp



Reproducible: Always

Steps to Reproduce:
$ git clone https://github.com/milabs/QtSampleModel
$ cd QtSampleModel
$ git checkout treemodel
$ qmake && make

1) run TreeModel
2) select "B"
3) type "c" in edit, then backspace
4) type "f" in edit, then backspace
5) type "x" in edit -> CRASH

Actual Results:  
Also, looks like that model doesn't operate with QTableView​ ​correctly. The patch shows the problem​ ​place:

@@ -2446,7 +2446,7 @@ int KSelectionProxyModel::columnCount(const QModelIndex &index) const
             // it's actually 0 ,but must return what the source model says, even if we
             // have no rows or columns.
 #if QT_VERSION >= 0x040700
-            || d->m_rootIndexList.isEmpty()
+//            || d->m_rootIndexList.isEmpty()
 #endif
        ) {
         return 0;
Comment 1 Albert Astals Cid 2015-10-05 09:24:39 UTC
Does that patch actually fix the assert for you? I still seem to get the assert with or without the change.
Comment 2 Ilya Matveychikov 2015-10-05 09:50:16 UTC
It fixes QTableView issue not the crash itself.
Comment 3 Ilya Matveychikov 2015-10-05 09:50:37 UTC
(In reply to Albert Astals Cid from comment #1)
> Does that patch actually fix the assert for you? I still seem to get the
> assert with or without the change.

It fixes QTableView issue not the crash itself.
Comment 4 David Faure 2015-12-06 09:36:19 UTC
Git commit b2687816be334552c0dadceb3f562f71e8b2f596 by David Faure.
Committed on 06/12/2015 at 09:35.
Pushed by dfaure into branch 'master'.

Fix KSelectionProxyModel usage in QTableView

The proxy would initially say columnCount==0 and would then omit to notify
about the insertion of columns. Simpler to just always have columns, even
when we have 0 rows.
CCMAIL: steveire@gmail.com

M  +60   -15   autotests/kselectionproxymodeltest.cpp
M  +1    -0    autotests/test_model_helpers.h
M  +1    -10   src/kselectionproxymodel.cpp

http://commits.kde.org/kitemmodels/b2687816be334552c0dadceb3f562f71e8b2f596
Comment 5 David Faure 2015-12-06 09:56:57 UTC
Back to the crash: step 3 can be removed, and the crash still happens. The description can be changed to:

1) run TreeModel
2) select "B"
3) type "f" in edit
4) type backspace
5) type "x" in edit -> CRASH

Now with more details, after my investigation:

1) run TreeModel. It sets up the following models:
   tree model <- KRecursiveFilterProxyModel (shown by treeview) <- KSelectionProxyModel (shown by listview and tableview)
2) select "B" in the treeview (-> selection proxy shows its children)
3) type "f" in the lineedit. This changes the filtering, and hides "B" so instead "E" becomes selected (because E took B's place). The selection proxy reacts, but not correctly. It should show E's children (F+G). Instead it shows "F, E" in the listview and "F" in the tableview.
4) press backspace (then things get worse, the listview shows F,G,B,E,H and the tableview shows the correct "F,G")
5) type "x" in edit -> CRASH. But really the misbehaviour started at step 3, the crash is just a consequence.

Is it even correct for E to be selected instead of B when the filtering changes at step 3? I thought the item selection model used persistent indexes, so it should deselect, not select "whatever is now at row 1", no? At least this seems to be how it works in kmail's messagelist...
Comment 6 David Faure 2015-12-06 10:20:57 UTC
Hmm, testing in proxymodeltestapp shows that in "single selection mode", both QSortFilterProxyModel and KRecursiveFilterProxyModel lead to another row being selected instead of the one which was just removed (so this is likely a QItemSelectionModel behaviour).
In Extended selection mode, this doesn't happen.
Comment 7 David Faure 2015-12-06 10:30:21 UTC
Confirmed, 
    tree->setSelectionMode(QAbstractItemView::ExtendedSelection);
is a working workaround in QtSampleModel/TreeModelView.cpp. (Obviously that's just a workaround, not a fix).
Comment 8 Stephen Kelly 2016-03-28 05:13:05 UTC
Git commit 0cf75268b00decc0ecbe75ba905d7bc556dcca26 by Stephen Kelly.
Committed on 28/03/2016 at 05:09.
Pushed by skelly into branch 'master'.

KSPM: Recreate mapping on removal only if needed

Previously, the model would create a mapping for the wrong index while
processing removal.  Constrain the re-creation of the mapping by
determining at AboutToBe-time whether to re-create it.  This is
important because the mapping contains QPersistentModelIndexes to the
source model, and when post-processing the removal, that is already
invalidated.

M  +6    -1    src/kselectionproxymodel.cpp

http://commits.kde.org/kitemmodels/0cf75268b00decc0ecbe75ba905d7bc556dcca26