Ideas to optimise QAbstractItemView::dataChanged

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?

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.