Finding / removing a row from a QStandardItemModel by item data

Question

I have a QStandardItemModel with a single column (represents a list). Each item in the list has a unique integer ID stored as the QStandardItem's data (via QStandardItem::setData which I guess is into Qt::UserRole+1 by default).

Given one of these IDs, I'd like to find and remove the corresponding row from the model. Right now I'm doing this:

void NetworkManager::removeSessionFromModel (QStandardItemModel *model, int sessionId) {

    foreach (const QStandardItem *item, model->findItems("*", Qt::MatchWildcard)) {
        if (item->data() == sessionId) {
            model->removeRow(item->index().row());
            break;
        }
    }

}

It works fine, but every single line of that function makes me cringe. Is there a cleaner way to do any of this?

Mope

QStandardItemModel::setData uses Qt::EditRole by default. – Burnsides

@SilvanoCerza I'm setting it on the items, QStandardItem::setData, not QStandardItemModel. – Mope

Answer 1

You're using findItems wrong, it can already return the item you want just by passing the value you're searching for. If you call it like you're doing right now you're looping through your items at least two times, since findItems must iterate through all the items to find those that match your pattern, in your case all items match, then you iterate the returned items again to find the sessionId.

void NetworkManager::removeSessionFromModel (QStandardItemModel *model, int sessionId) {

    auto items = model->findItems(QString::number(sessionId));
    if (!items.empty()) {
        auto row = items.first()->index().row();
        model->removeRow(row);
    }
}

Alternatively you can use the match method since findItems uses that internally, so you avoid allocating the StandardItem just to get its index. Also match returns right after the number of items matching the pattern, in this case the value of sessionId, are found so it doesn't always iterate all the items; that's more efficient. Obviously if the value is not found after iterating all the items it returns an empty list.

auto start = model->index(0, 0);
auto indexes = model->match(start, Qt::UserRole + 1, QString::number(sessionId), 1, Qt::MatchExactly);
if (!indexes.empty()) {
    auto row = indexes.first().row();
    model->removeRow(row);
}

Burnsides

Hey good idea; findItems directly won't actually work in my case since it calls match with Qt::DisplayRole but the suggestion to use match with Qt::UserRole+1 is the way to go I think. I'll give this a shot today. – Mope

Works like a charm. foreach (auto index, model->match(model->index(0, 0), Qt::UserRole + 1, sessionId, 1, Qt::MatchExactly)) model->removeRow(index.row()); if you're going for brevity. The indices won't change out from under you during the loop as long as hits is set to 1. Also since the match parameter is a QVariant, I skipped QString::number. – Mope

Answer 2

How about traversing the QStandardItemModel directly? Something like this:

void NetworkManager::removeSessionFromModel (QStandardItemModel *model, int sessionId) 
{
    for (int i = 0; i < model->rowCount(); ++i)
    {
        if (model->item(i)->data() == sessionId)
        {
            model->removeRow(i);
            break;
        }
    } 
}

Not sure how QStandardItemModel behaves with random access, maybe your method is more efficient.

Edit:

Actually, there is a function to do what you want: QAbstractItemModel::match

It returns a QModelIndexList with all entries that have matching data in a given role.

void NetworkManager::removeSessionFromModel (QStandardItemModel *model, int sessionId)
{
    QModelIndexList list = model->match(model->index(0, 0), Qt::UserRole + 1, sessionId);

    if (!list.empty())
        model->removeRow(list .first().row());
}

Setting data to a specific role can be done as follows:

model->setData(model->index(row, col), QVariant(13), Qt::UserRole + 1);

Reynolds

OP is probably using Qt::DisplayRole as its sessionId value, otherwise findItems wouldn't work since the implementation calls match internally using that role. – Burnsides

Now that I think about it QStandardItem::data() by default returns the value of Qt::UserRole + 1 and OP is matching on that, and not on Qt::DisplayRole, that's probably why OP used findItems that way it did. – Burnsides

@SilvanoCerza I indeed am matching on Qt::UserRole+1; I used findItems("*", ...) just to get a list of items I could iterate over, and didn't use findItems to do the search because it uses DisplayRole. But I actually didn't realize match existed until I saw your answer. – Mope

@RickPat Sorry man, I might be shuffling answer check marks around. This Q&A is a tough one because I kinda feel like everybody's a winner so far, heh. – Mope

I went with match; I chose the other answer since I found it clearer on that point. That said, even though match is the most direct code, I find your suggestion with the int index to be the clearest to read. So that's another good option. – Mope

Answer 3

You need to get the row index from your item id.

A more effective way could be to use a QMap with the row index as value and the item id as a key.

In this case, you also need to maintain the map values every time you add/remove a row.

If you don't have 3 millions items in your list, just keep it simple and use your code. By optimize this code, you probably also add complexity and reduce maintainability, and you get is 0,05 ms instead of 0,06 ms.

In GUI code, I often have code like this : it's simple, everyone get it immediatly and it does the job. It' also fast enough.

Crabwise

Even though this isn't a direct answer, this is the best advice here. After reading it I was perfectly happy with my code, and changes just became academic at that point. – Mope

It might be better to use a QHash if you have lots of items, I suggest you read this comparison between QMap and QHash. – Burnsides