Ideas to optimise QAbstractItemView::dataChanged

[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?

[kshegunov]

After some code-mining the above loop should be equivalent to:

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)  {
        d->viewport->update(visualRect(d->model->index(i, j, parent)));
    }
}

The updates are compressed trough QWidgetBackingStore::markDirty. I know the docs mention this but I couldn't find it in QCoreApplication::compressEvent so I went spelunking, albeit rather short lived.