Custom listModel does not notify the view

Question

I have my custom list model where I put data which should be displayed on the QML view. But for some kind of reason the view in QML sometimes is updated normally, sometimes with previous data and sometimes updating is not performed.

Here is the function where I fill the model - this function is called from some other thread.

void MyScreen::fillListModel()
{
    const QString SEPARATOR = " ";   
    myListModel->resetModel();

    for (int i = 0; i < MAX_ROWS; ++i)
    {
        QString key = QString::fromUtf16(MyData::getParameterKey(i).c_str());
        QString val = QString::fromUtf16(MyData::getParameterVal(i).c_str());       
        myListModel->addItem(key + SEPARATOR + val);
    }
}

Implementation of model reset:

void BrowsingModelBase::resetModel()
{
    beginResetModel();
    m_items.clear();
    endResetModel();
}

Implementation of addItem():

void BrowsingModelBase::addItem(const BrowsingItemModelBase &item)
{
    int count = m_items.size();
    beginInsertRows(QModelIndex(), count, count);
    m_items.append(item);
    endInsertRows();
}

Finally my QML file:

MyScreen {

    Column {
        id: myFlowList
        y: 110
        x: 220

        ListView {
            height:1000
            spacing: 35;

            model: myListModelRoot.myListModel

            delegate: Text {
                text: text1
            }
        }
    }
}

The strange thing is that after loop with line

myListModel->addItem(key + SEPARATOR + val);

when I print logs with data from myListModel it is filled with proper data, but the view is usually updated with previous data. Is it possible that data change signal is stuck somewhere? Any idea what is the solution?

Does the `BrowsingItemModelBase::BrowsingItemModelBase(const QString &)` constructor exist? The `fillListModel` is written as if there was such a constructor. — Kuba hasn't forgotten Monica, Aug 19 '15 at 15:53
`QString::fromUtf16` called on, presumably, `std::string::c_str()` is perhaps a bug? — Kuba hasn't forgotten Monica, Aug 19 '15 at 16:01

score 5 · Answer 1 · edited May 23 '17 at 12:17

Assuming that you call your model's methods from another thread, with the model being fundamentally not thread-safe, you have two options:

Make certain methods of your model thread-safe, or
Explicitly invoke the methods in a thread-safe fashion.

But first, you'd gain a bit of performance by adding all the items at once, as a unit. That way, the model will emit only one signal for all of the rows, instead of one signal per row. The views will appreciate it very much.

class BrowsingModelBase {
  ...
};
Q_DECLARE_METATYPE(QList<BrowsingItemModelBase>)

void BrowsingModelBase::addItems(const QList<BrowsingItemModelBase> & items)
{
  beginInsertRows(QModelIndex(), m_items.size(), m_items.size() + items.size() - 1);
  m_items.append(items);
  endInsertRows();
}

You should probably also have a method called clear instead of resetModel, since to reset a model has a much more general meaning: "change it so much that it's not worth emitting individual change signals". To reset a model does not mean "clear it"! Thus:

void BrowsingModelBase::clear()
{
  beginResetModel();
  m_items.clear();
  endResetModel();
}

Finally, following the 2nd approach of safely invoking the model's methods, fillListModel becomes as follows. See this answer for discussion of postTo.

template <typename F>
void postTo(QObject * obj, F && fun) {
  if (obj->thread() != QThread::currentThread()) {
    QObject signalSource;
    QObject::connect(&signalSource, &QObject::destroyed, obj, std::forward<F>(fun));
  } else 
    fun();
}

void MyScreen::fillListModel()
{
  auto separator = QStringLiteral(" ");
  QList<BrowserItemModelBase> items;
  for (int i = 0; i < MAX_ROWS; ++i) {
    auto key = QString::fromUtf16(MyData::getParameterKey(i).c_str());
    auto val = QString::fromUtf16(MyData::getParameterVal(i).c_str());
    items << BrowserItemModelBase(key + separator + val);
  }
  postTo(myListModel, [this, items]{ 
    myListModel->clear();
    myListModel->addItems(items);
  });
}

Alternatively, following the first approach, you can make the clear and addItems methods thread-safe:

/// This method is thread-safe.
void BrowsingModelBase::addItems(const QList<BrowsingItemModelBase> & items)
{
  postTo(this, [this, items]{
    beginInsertRows(QModelIndex(), m_items.size(), m_items.size() + items.size() - 1);
    m_items.append(items);
    endInsertRows();
  });
}

/// This method is thread-safe.
void BrowsingModelBase::clear()
{
  postTo(this, [this]{
    beginResetModel();
    m_items.clear();
    endResetModel();
  });
}

You then need no changes to fillListModel, except to make it use addItems:

void MyScreen::fillListModel()
{
  auto separator = QStringLiteral(" ");
  myListModel->clear();
  QList<BrowserItemModelBase> items;
  for (int i = 0; i < MAX_ROWS; ++i) {
    auto key = QString::fromUtf16(MyData::getParameterKey(i).c_str());
    auto val = QString::fromUtf16(MyData::getParameterVal(i).c_str());
    items << BrowserItemModelBase(key + separator + val);
  }
  myListModel->addItems(items);
}

score 1 · Accepted Answer · answered Aug 19 '15 at 06:25

The problem is most likely the fact that you are calling the fillListModel() method not from the main GUI thread. You can update the model from other threads but the beginResetModel();, endResetModel();, beginInsertRows(QModelIndex(), count, count);... methods have to be called in the main GUI thread.

One way to call these method in GUI thread (maybe not the most efficient) is to :

Create signals for each method you want to call:

  signals:
    //these signals are emitted from worker thread
    void requestBeginResetModel();
    void requestEndResetModel();

Create slots that will actually call the methods:

  private slots:
    //these slots execute the model reset operations in main thread
    void callBeginResetModel();
    void callEndResetModel();

Connect the signals and the slots:

  //connect the appropriate signals
  connect(this, SIGNAL(requestBeginResetModel()),
          this, SLOT(callBeginResetModel()));
  connect(this, SIGNAL(requestEndResetModel()),
          this, SLOT(callEndResetModel()));

Your reset model will then be:

void BrowsingModelBase::resetModel()
{
    emit requestBeginResetModel();
    m_items.clear();
    emit requestEndResetModel();
}

Finally the slots are implemented as:

void ObjectModel::callBeginResetModel()
{
  beginResetModel();
}

void ObjectModel::callEndResetModel()
{
  endResetModel();
}

Note that you will have to do the same for row insert methods as well. Or alternatively you could fill you model in the resetModel() method in between emitted signals.

This is going in the right direction, but very unwise. `resetModel` **must be treated as a unit** - the entirety of it should run in the thread that own the model. Doing it any other way without ensuring that a lot of other things are thread-safe too is just asking for undefined behavior. — Kuba hasn't forgotten Monica, Aug 19 '15 at 15:38
I'll be more blunt: this answer is wrong. To anyone thinking of using the code here as-is: DON'T. Not only does it promote undefined behavior, but the whole Qt 4 proxy signals idea is horribly verbose and backwards in modern times. — Kuba hasn't forgotten Monica, Aug 19 '15 at 16:08

Custom listModel does not notify the view

2 Answers2

Linked