Ideas to optimise QAbstractItemView::dataChanged

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.

VRonin

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

Okay, this is my bad then.

I see what you mean. if a view only reimplemented paintEvent it might break due to this change. fair point probably a note for future releses only then

To summarise where we are, the current "idea" of QAbstractItemView::dataChanged is:

void QAbstractItemView::dataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles)
{
    Q_UNUSED(roles)
    // Single item changed
    Q_D(QAbstractItemView);
    if (topLeft == bottomRight && topLeft.isValid()) {
        const QEditorInfo &editorInfo = d->editorForIndex(topLeft);
        //we don't update the edit data if it is static
        if (!editorInfo.isStatic && editorInfo.widget) {
            QAbstractItemDelegate *delegate = d->delegateForIndex(topLeft);
            if (delegate) {
                delegate->setEditorData(editorInfo.widget.data(), topLeft);
            }
        }
        if (isVisible() && !d->delayedPendingLayout) {
            // otherwise the items will be update later anyway
            update(topLeft);
        }
    } else {
        d->updateEditorData(topLeft, bottomRight);
        if (isVisible() && !d->delayedPendingLayout)  {
            const QRect viewportRect = d->viewport->rect(); 
            const QModelIndex parent = topLeft.parent();
            const QRect cornerRect = visualRect(topLeft) | visualRect(bottomRight);
            // If dirty is 1/4th or more of the viewport rect, just trigger a full update
            const QRect cornerRectViewPort = cornerRect & viewportRect;
            if(cornerRectViewPort.width() * cornerRectViewPort.height() * 4 > viewportRect.width() * viewportRect.height())  {
                d->viewport->update();
            }
            else  { // Just fall back to iterating over the model indices
                    QRect dirty(cornerRect);
                    const int maxRow = bottomRight.row();
                    for (int i = topLeft.row(); i < maxRow; ++i)  {
                        const int maxColumn = bottomRight.column() - ((i==maxRow-1) ? 1:0);
                        for (int j = topLeft.column()+ ((i==topLeft.row()) ? 1:0); j < maxColumn; ++j)  {
                            dirty |= visualRect(d->model->index(i, j, parent));
                        }
                    }
                    dirty &= viewportRect;
                    if (!dirty.isEmpty())
                        d->viewport->update(dirty);
            }
        }
    }
#ifndef QT_NO_ACCESSIBILITY
    if (QAccessible::isActive()) {
        QAccessibleTableModelChangeEvent accessibleEvent(this, QAccessibleTableModelChangeEvent::DataChanged);
        accessibleEvent.setFirstRow(topLeft.row());
        accessibleEvent.setFirstColumn(topLeft.column());
        accessibleEvent.setLastRow(bottomRight.row());
        accessibleEvent.setLastColumn(bottomRight.column());
        QAccessible::updateAccessibility(&accessibleEvent);
    }
#endif
    d->updateGeometry();
}

EDIT:

Avoid calculating visualRect for the corners twice in the iteration case

kshegunov

@Christian-Ehrlicher
Even though it's not strictly the model API we were discussing, have you any thoughts on the proposed snippet? would it be worth changing like this?