QListWidget and takeItem() slow

[deca5423]

Why is takeItem() horribly slow when used on more than 20 items in a list?

I've come up with a work around but I need to clear my original lists. QListWidget::clear(), however, deletes my dynamically allocated pointers? Doesn't this go against everything in C++? So this brings me back to takeItem(), which sucks.

Example:

QListWidgetItem* item = currentList->item(0);
QString str = item->text(); //fine
currentList->clear();
str = item->text(); //crashes

QListWidgetItem* item = currentList->item(0);
QString str = item->text(); //fine
currentList->clear();
str = item->text(); //crashes

To copy to clipboard, switch view to plain text mode 

Am I doing something horribly wrong? My lists easily can contain over 1,000 items. Clear() is the only function that seems to 'clear' them fast, but my items are permanently deleted. This would be like if std::vector or std::map deleted pointers when you cleared. This is EXTREMELY frustrating.

[wysota]

I don't see any use of takeItem() in the code you pasted. Are you sure this is the right code?

[deca5423]

The original code I was using:

int pos = right_export->m_ui->lumpListView->currentRow() + 1;
 
        if ( right_export->m_ui->lumpListView->count() == 0 )
            pos = 0;
 
        int finalpos = currentList->row(list.at(0));
 
        for( int i=0; i<list.size(); i++ ) {
 
            QListWidgetItem* item = currentList->takeItem( currentList->row(list.at(i)) );
 
            right_export->m_ui->lumpListView->insertItem(pos,item);
            right_export->m_ui->lumpListView->clearSelection();
            if ( pos >= right_export->m_ui->lumpListView->count() )
                pos = right_export->m_ui->lumpListView->count()-1;
            right_export->m_ui->lumpListView->setCurrentRow(pos);
            pos++;
 
        }

int pos = right_export->m_ui->lumpListView->currentRow() + 1;

        if ( right_export->m_ui->lumpListView->count() == 0 )
            pos = 0;

        int finalpos = currentList->row(list.at(0));

        for( int i=0; i<list.size(); i++ ) {

            QListWidgetItem* item = currentList->takeItem( currentList->row(list.at(i)) );

            right_export->m_ui->lumpListView->insertItem(pos,item);
            right_export->m_ui->lumpListView->clearSelection();
            if ( pos >= right_export->m_ui->lumpListView->count() )
                pos = right_export->m_ui->lumpListView->count()-1;
            right_export->m_ui->lumpListView->setCurrentRow(pos);
            pos++;

        }

To copy to clipboard, switch view to plain text mode 

It's fine as long as I don't have more than 20 items selected. Even if I do something simple like remove all items:

while( currentList->count() )
    currentList->takeItem(0);

while( currentList->count() )
    currentList->takeItem(0);

To copy to clipboard, switch view to plain text mode 

In that example if the list is even more than 100 items, it never returns. My code wasn't incorrect. I was simply pointing out that clear() makes no sense and goes against everything else in C++.

[wysota]

My code wasn't incorrect.

Yes, it was -- see below.

I was simply pointing out that clear() makes no sense and goes against everything else in C++.

Of course it makes sense. clear() is for clearing a list and clearing a list means getting rid of all the items in the list. If clear() wasn't deleting the items then to avoid memory leaks you would need a separate list where you kept all item pointers and it would be hard to synchronize that list with other actions that may be happening in your application. So the list is so to say "embedded" iinto the widget itself. If you don't want it to be -- use the model approach in which case you won't find a clear() method in QListView but rather (if at all) in the model. There is no reason clear() should not delete the items if they are owned by the widget. That's all in conformance with every design rules (C++ or no C++) I know.

Now back to your code....

Everytime you take the item out of the list the list is updated which means that it needs to do some calculations, inform its environment about the change by emitting proper signals and finally redraw itself. This happens with every remove so if you remove 1000 items one by one, you get 1000 of those. If you do it in one go - like clear() does - you only get one update so things are approximately 1000 faster. If you insisted on using takeItem() a more proper code would be:

list->setUpdatesEnabled(false);
list->blockSignals(); // optionally
while(list->item(0)) // notice I'm not using count() which is also slow
  list->takeItem(0);
list->unblockSignals(); // optionally
list->setUpdatesEnabled(true);
list->update(); // force update

list->setUpdatesEnabled(false);
list->blockSignals(); // optionally
while(list->item(0)) // notice I'm not using count() which is also slow
  list->takeItem(0);
list->unblockSignals(); // optionally
list->setUpdatesEnabled(true);
list->update(); // force update

To copy to clipboard, switch view to plain text mode 

I'm not sure if blocking signals will not make your application crash in this particular situation so you might want to avoid it.

Another thing - I have no idea what the code you pasted is meant to do, I thing you overcomplicated it greatly. If you told us what was your intention, maybe we could help you find a simpler alternative.

[deca5423]

Thank you for taking the time to reply. I realized the code with clear() was incorrect, I meant that I didn't post the incorrect code? If that makes any sense

Blocking the signals did the trick, I don't know why I didn't think to do that. I remember trying the disabling the updates which didn't do much difference. takeItem in that messy function I used before is much faster, but still too slow with updates and signals blocked. However, I wrote my own container for QListWidget with functions addItems and removeItems. Sadly, clearing the lists using take items, throwing everything in my container, calling removeItems and addItems, and then finally repopulating the lists based on my containers is infinitely faster.

I have many lists in the program. Basically there is a left list, and a right list. You are able to select multiple items from one list and insert them into another. So all my QListWidgetItems are null parented and I think of the QListWidgets as more of containers, which is why I got frustrated when I realized clear() deleted my items.

[wysota]

In my opinion you should use the model based approach. Preferably with the same base model for all views and two sort-filter proxies (one for each view) that would only hide or show respective entries from the base model.

Here is a rough example implementation:

#include <QtGui>                                                                                                                                                                   
 
class Widget : public QWidget {
  Q_OBJECT                     
public:                        
  enum {FilterRole = Qt::UserRole+1};
  Widget() {                         
    QHBoxLayout *l = new QHBoxLayout(this);
    lview = new QListView; rview = new QListView;
    l->addWidget(lview); l->addWidget(rview);    
    QStandardItemModel *model = new QStandardItemModel(this);
    model->setColumnCount(1);                                
    for(int i=0;i<100;i++){                                  
      QStandardItem *item = new QStandardItem(QString::number(i+1));
      item->setData(true, FilterRole);
      item->setFlags(Qt::ItemIsEnabled); // disable editing
      model->appendRow(item);
    }
 
    QSortFilterProxyModel *lproxy = new QSortFilterProxyModel(this);
    lproxy->setDynamicSortFilter(true);
    lproxy->setFilterRole(FilterRole);
    lproxy->setFilterFixedString("true");
    lproxy->setSourceModel(model);
    lview->setModel(lproxy);
 
    QSortFilterProxyModel *rproxy = new QSortFilterProxyModel(this);
    rproxy->setDynamicSortFilter(true);
    rproxy->setFilterRole(FilterRole);
    rproxy->setFilterFixedString("false");
    rproxy->setSourceModel(model);
    rview->setModel(rproxy);
    connect(lview, SIGNAL(doubleClicked(QModelIndex)), SLOT(toggleItem(QModelIndex)));
    connect(rview, SIGNAL(doubleClicked(QModelIndex)), SLOT(toggleItem(QModelIndex)));
  }
public slots:
  void toggleItem(const QModelIndex &index){
    bool isInLeft = index.data(FilterRole).toBool();
    const_cast<QAbstractItemModel*>(index.model())->setData(index, !isInLeft, FilterRole);
  }
private:
  QListView *lview, *rview;
};
 
#include "main.moc"
 
int main(int argc, char **argv){
  QApplication app(argc, argv);
  Widget w;
  w.show();
  return app.exec();
}

#include <QtGui>                                                                                                                                                                   

class Widget : public QWidget {
  Q_OBJECT                     
public:                        
  enum {FilterRole = Qt::UserRole+1};
  Widget() {                         
    QHBoxLayout *l = new QHBoxLayout(this);
    lview = new QListView; rview = new QListView;
    l->addWidget(lview); l->addWidget(rview);    
    QStandardItemModel *model = new QStandardItemModel(this);
    model->setColumnCount(1);                                
    for(int i=0;i<100;i++){                                  
      QStandardItem *item = new QStandardItem(QString::number(i+1));
      item->setData(true, FilterRole);
      item->setFlags(Qt::ItemIsEnabled); // disable editing
      model->appendRow(item);
    }

    QSortFilterProxyModel *lproxy = new QSortFilterProxyModel(this);
    lproxy->setDynamicSortFilter(true);
    lproxy->setFilterRole(FilterRole);
    lproxy->setFilterFixedString("true");
    lproxy->setSourceModel(model);
    lview->setModel(lproxy);

    QSortFilterProxyModel *rproxy = new QSortFilterProxyModel(this);
    rproxy->setDynamicSortFilter(true);
    rproxy->setFilterRole(FilterRole);
    rproxy->setFilterFixedString("false");
    rproxy->setSourceModel(model);
    rview->setModel(rproxy);
    connect(lview, SIGNAL(doubleClicked(QModelIndex)), SLOT(toggleItem(QModelIndex)));
    connect(rview, SIGNAL(doubleClicked(QModelIndex)), SLOT(toggleItem(QModelIndex)));
  }
public slots:
  void toggleItem(const QModelIndex &index){
    bool isInLeft = index.data(FilterRole).toBool();
    const_cast<QAbstractItemModel*>(index.model())->setData(index, !isInLeft, FilterRole);
  }
private:
  QListView *lview, *rview;
};

#include "main.moc"

int main(int argc, char **argv){
  QApplication app(argc, argv);
  Widget w;
  w.show();
  return app.exec();
}

To copy to clipboard, switch view to plain text mode