Ideas to optimise QAbstractItemView::dataChanged



  • 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


  • Moderators

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


  • Qt Champions 2017

    No ABI breaking suggestions?


  • Moderators

    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.



  • 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


  • Qt Champions 2017

    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.


  • Qt Champions 2017

    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);
        }
    }


  • @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.


  • Qt Champions 2017

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

    This should be easily optimisable.

    See above code and please comment!



  • @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);
    }
    

  • Qt Champions 2017

    @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 ...



  • @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

  • Qt Champions 2017

    @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);
            }
        }
    }
    


  • 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;
    

  • Qt Champions 2017

    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?



  • @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


  • Qt Champions 2017

    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.


  • Qt Champions 2017

    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?



  • @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


  • Qt Champions 2017

    @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.



  • @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


  • Qt Champions 2017

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


  • Qt Champions 2017

    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.


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.