Model indexing fix in AlbumWidget::addPictures (chapter 4)

[ziembla]

(This was originally submitted to the 1st edition's codebase PacktPublishing/Mastering-Qt-5#10 but also applies here)

Thanks for a great book, but while reading it I stumbled upon a problem I might even have a solution for.

When running Debug binary of gallery-desktop from chapter 4 the program shuts down on adding pictures, just after closing the "Open File" dialog, with a message:

ASSERT: "proxyIndex.model() == this" in file itemmodels\qidentityproxymodel.cpp, line 259

The program runs just fine in Release.

The offending line is Qt sources is https://github.com/qt/qtbase/blob/4eda22ea0db1fc571ae9f44a68825056e6245548/src/corelib/itemmodels/qidentityproxymodel.cpp#L259

As I digged down, the problem seems to lay in AlbumWidget::addPictures. The lastModelIndex is an index into PictureModel but it is used later to setCurrentIndex where ThumbnailProxyModel is expected.

The simplest fix might be just adding a line converting the QModelIndex (below).

Works on my machine ;-)

My environment is Qt 5.12.1 on MinGW 64bit but it seems to be platform agnostic.

[GuillaumeLazar]

Hi @ziembla, thanks for reading the book and taking the time to create a PR for a potential bug.

Unfortunately, I can't reproduce the bug. No issues on a common desktop configuration. (Qt 5.12.2 / Linux Ubuntu 16.04.6 LTS / gcc 5.4.0 / debug mode).

Could you list all the steps to reproduce the bug? Did you actually add a picture, or did you click on the cancel button of the open file dialog? etc.

It's also really strange that it's work for you in release but not in debug. Do you have any differences in the 'build/run configuration' of the Debug and Release mode?

[ziembla]

Thanks for reply, unfortunatelly only now I am able to catch up.

I might have suspected the problem is not so obvious when the official sample code "just seems not to work"...

Now, as I said, the program collapses just after the "Add pictures" dialog is closed and yes, I added a picture and that was the first try with freshly-initialized database. I narrowed the problem down to the ui->thumbnailListView->setCurrentIndex(lastModelIndex) call at the end of AlbumWidget::addPictures().

It may not be so surprising that only Debug build is affected, as it is an assert failing and I suppose they're just stripped away in Release?

Before writing the PR I've confirmed the problem on two machines, still, they were two Windows x64 machines, both with Qt 5.12.1. One was Win7, the second Win10.

The only linux box I was able to put my hands on was an ubuntu 18.04 one with Qt 5.9.5. The problem didn's show up there, so surprisingly perhaps it is platform-specific?

I was able to isolate the problem to the following snippet that can be pasted to main.cpp in an empty Qt project:

class MyModel : public QAbstractListModel {
public:
    int rowCount(const QModelIndex&) const override { return 17; }
    QVariant data(const QModelIndex &index, int) const override { return QString("data@") + QString('0' + index.row()); }
};

class MyProxyModel : public QIdentityProxyModel {
public:
    QVariant data(const QModelIndex &index, int role) const override { return QIdentityProxyModel::data(index, role); }
    void setSourceModel(QAbstractItemModel *sourceModel) override { QIdentityProxyModel::setSourceModel(sourceModel); }
};

MyModel myModel;
MyProxyModel myProxyModel;
myProxyModel.setSourceModel(&myModel);

QListView listView(&w);
listView.setModel(&myProxyModel);

//INFO 1: no problem here
listView.setCurrentIndex(myProxyModel.index(7, 0));
qDebug() << "index from proxy model" << listView.currentIndex();

//INFO 2: breaks on windows debug with the following message
//        ASSERT: "proxyIndex.model() == this" in file itemmodels\qidentityproxymodel.cpp, line 259
listView.setCurrentIndex(myModel.index(8, 0));
qDebug() << "index from source model" << listView.currentIndex();

class OtherModel : public QAbstractListModel {
public:
    int rowCount(const QModelIndex&) const override { return 27; }
    QVariant data(const QModelIndex &index, int) const override { return QString("other@") + QString('0' + index.row()); }
};

OtherModel otherModel;

//INFO 3: breaks as in 2 if 2 is commented-out
listView.setCurrentIndex(otherModel.index(9, 0));
qDebug() << "index from other model" << listView.currentIndex();

The results are as follows

linux

Release/indexing-problem...
index from proxy model QModelIndex(7,0,0x0,QIdentityProxyModel(0x7ffd7c8265f0))
index from source model QModelIndex(8,0,0x0,QAbstractListModel(0x7ffd7c8265e0))
index from other model QModelIndex(9,0,0x0,QAbstractListModel(0x7ffd7c826600))

Debug/indexing-problem...
index from proxy model QModelIndex(7,0,0x0,QIdentityProxyModel(0x7fff834a68e0))
index from source model QModelIndex(8,0,0x0,QAbstractListModel(0x7fff834a68d0))
index from other model QModelIndex(9,0,0x0,QAbstractListModel(0x7fff834a68f0))

windows

Release\release\indexing-problem.exe...
index from proxy model QModelIndex(7,0,0x0,QIdentityProxyModel(0x61fc70))
index from source model QModelIndex(8,0,0x0,QAbstractListModel(0x61fc60))
index from other model QModelIndex(9,0,0x0,QAbstractListModel(0x61fc80))

Debug\debug\indexing-problem.exe...
index from proxy model QModelIndex(7,0,0x0,QIdentityProxyModel(0x76fb50))
ASSERT: "proxyIndex.model() == this" in file itemmodels\qidentityproxymodel.cpp, line 259
15:41:15: The program has unexpectedly finished.

So it seems that the problem it that there is a check to verify whether the index being set is connected to the same model that is set as listView's source model (in QIdentityProxyModel::mapToSource). But only windows debug binary seems to perform the check as far as I was able to confirm.

That looks more like Qt's bug/inconsistency and I may be able to try to file an issue on that but even so the book's code should benefit from the conversion I suggest.

[soga-denken]

Hi,the same issue happened to me.
The pull request fixed the issue in my box. Thanks.

[GuillaumeLazar]

@ziembla thanks for your deep analysis and this PR.
@soga-denken thank you for the confirmation message.

PR merged! 🎉