Ideas to optimise QAbstractItemView::dataChanged

[VRonin]

As highlighted in this topic, when topLeft!=bottomRight the view triggers a complete repaint.

We are putting some ideas on the table to make it better

[Christian Ehrlicher]

If someone has a good idea how to improve the default implementation in QAbstractItemModel - I'm open for suggestions

[kshegunov]

No ABI breaking suggestions?

[Christian Ehrlicher]

No, nothing for Qt6 - at least not for now.
A dataChanged(QVector<QModelIndex>...) would maybe ok but it would not make anything better then doing it manually.

[VRonin]

While I do realise in general it's difficult to make it better, concrete subclasses should be able to optimise it. Take QTableView for example, computing the rect for the items to update for that class should be easy but it still relies on the default implementation

[kshegunov]

That's what I was thinking along, however the main issue I see is that when you give the view a range (from, to), it doesn't know which indices are actually updated. So to take your question to the extreme - assume flavor 1 and assume you pass it QModelIndex(0, 0) and QModelIndex(maxIndex, maxIndex), then the view would "think" that all the items are changed, when only the two edges are touched. Still I do believe a small optimization in the default implementation could be made:

Instead of taking viewport->update() one could calculate the region corresponding to the range of model indices and pass viewport->update(region). However if this'd really mean something performance-wise has to be tested. Furthermore it's not quite clear if the viewport is actually going to respect the dirty region ...

Or at least a union of the items' visual rects could be done, if using a QRegion isn't feasible.

[kshegunov]

I was thinking something like this:

} else {
    d->updateEditorData(topLeft, bottomRight);
    if (isVisible() && !d->delayedPendingLayout)  {
        QModelIndex parent = topLeft.parent();

        QRect dirty;
        for (qint32 i = topLeft.row(), maxRow = bottomRight.row(); i < maxRow; i++)  {
            for (qint32 j = topLeft.column(), maxColumn = bottomRight.column(); j < maxColumn; j++)  {
                dirty |= visualRect(d->model->index(i, j, parent));
            }
        }

        dirty &= d->viewport->rect();
        if (!dirty.isEmpty())
            d->viewport->update(dirty);
    }
}

[VRonin]

@kshegunov said in QAbstractItemModel::dataChanged what would you prefer?:

assume flavor 1 and assume you pass it QModelIndex(0, 0) and QModelIndex(maxIndex, maxIndex), then the view would "think" that all the items are changed

Yes and I have some code that relies on that and it's even documented:
http://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged

If the items are of the same parent, the affected ones are those between topLeft and bottomRight inclusive. If the items do not have the same parent, the behavior is undefined.

the problem is that if you have a 1000x1000 table and pass index(0,0),index(1,1) it will still update 1000000 items instead of 4. This should be easily optimisable.

[kshegunov]

@VRonin said in QAbstractItemModel::dataChanged what would you prefer?:

This should be easily optimisable.

See above code and please comment!

[VRonin]

@kshegunov Sorry, you probably ninja'd your code while I was typing.

Looks good and it wasn't such a big change in code. If imagine the same index(0,0),index(1,1) and the editor is open in index(0,1) you still are updating a cell too many but it's already a huge improvement over the default.

QTableView can optimise it further with

d->updateEditorData(topLeft, bottomRight);
    if (isVisible() && !d->delayedPendingLayout)  {
const QRect dirty = (visualRect(topLeft) | visualRect(bottomRight)) & d->viewport->rect();
if (!dirty.isEmpty())
            d->viewport->update(dirty);
}

[kshegunov]

@VRonin said in QAbstractItemModel::dataChanged what would you prefer?:

QTableView can optimise it further

Indeed, however the QAbstractItemView doesn't know the exact layout of the items (yet), so that's why the looping over. There's one catch though, I fear that visualRect might be rather costly, so I don't know if anything is really gained by my snippet ...

[VRonin]

@kshegunov said in QAbstractItemModel::dataChanged what would you prefer?:

I fear that visualRect might be rather costly, so I don't know if anything is really gained by my snippet ...

worst case is index(0, 0), index(rowCount(), columnCount()) where (assuming repaint implies evaluating more or less the same as visualRect) you end up doubling the calls. You could have something like:

} else {
    d->updateEditorData(topLeft, bottomRight);
    if (isVisible() && !d->delayedPendingLayout)  {
        const QModelIndex parent = topLeft.parent();
        const int cellsToUpdate = (bottomRight.row()-topLeft.row()+1)*(bottomRight.column()-topLeft.column()+1);
        const int totalCells = d->model->rowCount(parent )*d->model->columnCount(parent );
        if(cellsToUpdate > totalCells/2){
            d->viewport->update();
        } else{
                QRect dirty;
                for (int i = topLeft.row(), maxRow = bottomRight.row(); i < maxRow; ++i)  {
                    for (int j = topLeft.column(), maxColumn = bottomRight.column(); j < maxColumn; ++j)  {
                        dirty |= visualRect(d->model->index(i, j, parent));
                    }
                }

                dirty &= d->viewport->rect();
                if (!dirty.isEmpty())
                    d->viewport->update(dirty);
            }
    }
}

I still like your direct approach more though as:

• paint is probably orders of magnitude more costy than visualRect (not the same cost as I assumed above)
• depending how many items are inside or outside the viewport you might still end up gaining performance with your method

[kshegunov]

@VRonin said in QAbstractItemModel::dataChanged what would you prefer?:

You could have something like

That's an interesting twist, note however that no part of your index range may be visible in actuality, so if it were me I'd stick to just looping over and getting the visualRect. ;)
You can however check the edges before that and hope that a leaf (i.e. at the level of parent) of the model is displayed flat (as in the case of the QTableView. I.e. possible update of your suggestion to include some "heuristics":

} else {
    d->updateEditorData(topLeft, bottomRight);
    if (isVisible() && !d->delayedPendingLayout)  {
        const QRect viewportRect = d->viewport->rect(); 
        const QModelIndex parent = topLeft.parent();

        // If dirty is 1/4th or more of the viewport rect, just trigger a full update
        QRect dirty = (visualRect(topLeft) | visualRect(bottomRight)) & viewportRect;
        if(dirty.width() * dirty.height() * 4 > viewportRect.width() * viewportRect.height())  {
            d->viewport->update();
        }
        else  { // Just fall back to iterating over the model indices
                QRect dirty;
                for (int i = topLeft.row(), maxRow = bottomRight.row(); i < maxRow; ++i)  {
                    for (int j = topLeft.column(), maxColumn = bottomRight.column(); j < maxColumn; ++j)  {
                        dirty |= visualRect(d->model->index(i, j, parent));
                    }
                }

                dirty &= viewportRect;
                if (!dirty.isEmpty())
                    d->viewport->update(dirty);
        }
    }
}

[VRonin]

While we are at it: now that a lot of models actually send the roles vector, we could replace Q_UNUSED(roles) with something like

if(!roles.isEmpty() && std::all_of(roles.begin(),roles.end(),[](int role)->bool{return role>=Qt::UserRole}))
return;

[kshegunov]

I don't think that's too wise, there may be user roles that pertain to the display as well ... or am I misinterpreting the field?

[VRonin]

@kshegunov said in QAbstractItemModel::dataChanged what would you prefer?:

there may be user roles that pertain to the display as well

I thought the point of reserving every role before Qt::UserRole was Qt telling us it can do lots of (even undocumented) stuff with the first roles but they never use anything after Qt::UserRole

[kshegunov]

True, but correct me if I'm wrong, the point of user roles is when you need to pass some data to custom views where the role might have significance to the view display.

[kshegunov]

This thread is going way off topic, but let's roll with it a bit more ...
Can we use something like Qt::NeedsVisualUpdate role and somehow batch up the actual updates, compressing the paint events or something along those lines; deferring them to the last possible moment so to speak?

[VRonin]

@kshegunov said in QAbstractItemModel::dataChanged what would you prefer?:

to pass some data to custom views

Exactly, so you need to reimplement QAbstractItemView::dataChanged anyway. no problem with my code

This thread is going way off topic, but let's roll with it a bit more ...

Forked

[kshegunov]

@VRonin said in Ideas to optimise QAbstractItemView::dataChanged:

Forked

Yeah, thanks. We wrote a novel judging by the number of notifications I got. :)

Exactly, so you need to reimplement QAbstractItemView::dataChanged anyway. no problem with my code

Okay, this is my bad then.