Merge pull request #5852 from elpaso/bugfix-bookmarks-take2

[bugfix] bookmarks sorting and multiple deletion

Author: elpaso

src/app/qgsbookmarks.cpp

@@ -103,15 +103,24 @@ QgsBookmarks::QgsBookmarks( QWidget *parent )
   mQgisModel->setHeaderData( 7, Qt::Horizontal, tr( "SRID" ) );
 
   mProjectModel = new QgsProjectBookmarksTableModel( this );
-  mModel = new QgsMergedBookmarksTableModel( *mQgisModel, *mProjectModel, lstBookmarks, this );
+  mMergedModel = new QgsMergedBookmarksTableModel( *mQgisModel, *mProjectModel, lstBookmarks, this );
 
   mProxyModel = new QgsBookmarksProxyModel( this );
-  mProxyModel->setSourceModel( mModel );
+  mProxyModel->setSourceModel( mMergedModel );
 
   lstBookmarks->setModel( mProxyModel );
   lstBookmarks->setItemDelegate( new QgsDoubleSpinBoxBookmarksDelegate( this ) );
+  lstBookmarks->setSortingEnabled( true );
+  lstBookmarks->sortByColumn( 1, Qt::AscendingOrder );
 
-  connect( mModel, &QgsMergedBookmarksTableModel::layoutChanged, mProxyModel, &QgsBookmarksProxyModel::_resetModel );
+  connect( mMergedModel, &QgsMergedBookmarksTableModel::selectItem, this, [ = ]( const QModelIndex & index )
+  {
+    QModelIndex proxyIndex( mProxyModel->mapFromSource( index ) );
+    lstBookmarks->scrollTo( proxyIndex );
+    lstBookmarks->setCurrentIndex( proxyIndex );
+  } );
+
+  connect( mMergedModel, &QgsMergedBookmarksTableModel::layoutChanged, mProxyModel, &QgsBookmarksProxyModel::_resetModel );
 
   QgsSettings settings;
   lstBookmarks->header()->restoreState( settings.value( QStringLiteral( "Windows/Bookmarks/headerstate" ) ).toByteArray() );
@@ -143,7 +152,7 @@ void QgsBookmarks::saveWindowLocation()
 
 void QgsBookmarks::addClicked()
 {
-  Q_ASSERT( mModel );
+  Q_ASSERT( mMergedModel );
   Q_ASSERT( mQgisModel );
 
   QgsMapCanvas *canvas = QgisApp::instance()->mapCanvas();
@@ -176,7 +185,7 @@ void QgsBookmarks::addClicked()
   {
     mQgisModel->setSort( 0, Qt::AscendingOrder );
     mQgisModel->select();
-    QModelIndex newIdx = mProxyModel->mapFromSource( mModel->index( mQgisModel->rowCount() - 1, 1 ) );
+    QModelIndex newIdx = mProxyModel->mapFromSource( mMergedModel->index( mQgisModel->rowCount() - 1, 1 ) );
     // Edit new bookmark title
     lstBookmarks->scrollTo( newIdx );
     lstBookmarks->setCurrentIndex( newIdx );
@@ -192,16 +201,15 @@ void QgsBookmarks::addClicked()
 
 void QgsBookmarks::deleteClicked()
 {
-  QList<int> rows;
-  Q_FOREACH ( const QModelIndex &idx, lstBookmarks->selectionModel()->selectedIndexes() )
+  QItemSelection selection( mProxyModel->mapSelectionToSource( lstBookmarks->selectionModel()->selection() ) );
+  std::vector<int> rows;
+  for ( const auto &selectedIdx : selection.indexes() )
   {
-    if ( idx.column() == 1 )
-    {
-      rows << idx.row();
-    }
+    if ( selectedIdx.column() == 1 )
+      rows.push_back( selectedIdx.row() );
   }
 
-  if ( rows.isEmpty() )
+  if ( rows.size() == 0 )
     return;
 
   // make sure the user really wants to delete these bookmarks
@@ -210,12 +218,14 @@ void QgsBookmarks::deleteClicked()
        QMessageBox::Ok | QMessageBox::Cancel ) )
     return;
 
-  int i = 0;
-  Q_FOREACH ( int row, rows )
+  // Remove in reverse order to keep the merged model indexes
+  std::sort( rows.begin(), rows.end(), std::greater<int>() );
+
+  for ( const auto &row : rows )
   {
-    mModel->removeRow( row - i );
-    i++;
+    mMergedModel->removeRow( row );
   }
+  mProxyModel->_resetModel();
 }
 
 void QgsBookmarks::lstBookmarks_doubleClicked( const QModelIndex &index )
@@ -285,7 +295,7 @@ void QgsBookmarks::importFromXml()
   QDomElement docElem = doc.documentElement();
   QDomNodeList nodeList = docElem.elementsByTagName( QStringLiteral( "bookmark" ) );
 
-  Q_ASSERT( mModel );
+  Q_ASSERT( mMergedModel );
 
   QString queries;
 
@@ -354,8 +364,8 @@ void QgsBookmarks::exportToXml()
   QDomElement root = doc.createElement( QStringLiteral( "qgis_bookmarks" ) );
   doc.appendChild( root );
 
-  int rowCount = mModel->rowCount();
-  int colCount = mModel->columnCount() - 1;  // exclude virtual "In project" column
+  int rowCount = mMergedModel->rowCount();
+  int colCount = mMergedModel->columnCount() - 1;  // exclude virtual "In project" column
 
   QList<QString> headerList;
   headerList
@@ -374,7 +384,7 @@ void QgsBookmarks::exportToXml()
     root.appendChild( bookmark );
     for ( int j = 0; j < colCount; j++ )
     {
-      QModelIndex idx = mModel->index( i, j );
+      QModelIndex idx = mMergedModel->index( i, j );
       if ( idx.isValid() )
       {
         QString value = idx.data( Qt::DisplayRole ).toString();
@@ -489,9 +499,11 @@ bool QgsProjectBookmarksTableModel::setData( const QModelIndex &index, const QVa
 bool QgsProjectBookmarksTableModel::insertRows( int row, int count, const QModelIndex &parent )
 {
   Q_UNUSED( parent );
-  beginInsertRows( parent, row, row + count );
-
-  bool result = QgsProject::instance()->writeEntry( QStringLiteral( "Bookmarks" ), QStringLiteral( "/count" ), QgsProject::instance()->readNumEntry( QStringLiteral( "Bookmarks" ), QStringLiteral( "/count" ) ) + count );
+  Q_UNUSED( row );
+  // append
+  int oldCount = QgsProject::instance()->readNumEntry( QStringLiteral( "Bookmarks" ), QStringLiteral( "/count" ) );
+  beginInsertRows( parent, oldCount, oldCount + count );
+  bool result = QgsProject::instance()->writeEntry( QStringLiteral( "Bookmarks" ), QStringLiteral( "/count" ), oldCount + count );
   endInsertRows();
   return result;
 }
@@ -603,36 +615,39 @@ QVariant QgsMergedBookmarksTableModel::data( const QModelIndex &index, int role
 
 bool QgsMergedBookmarksTableModel::setData( const QModelIndex &index, const QVariant &value, int role )
 {
+  bool result = false;
   // last column triggers a move from QGIS to project bookmark
   if ( index.column() == mQgisTableModel.columnCount() )
   {
     if ( index.row() < mQgisTableModel.rowCount() )
     {
       // Move from SQLite storage to project
       moveBookmark( mQgisTableModel, mProjectTableModel, index.row() );
-      mTreeView->scrollTo( this->index( rowCount() - 1, 1 ) );
-      mTreeView->setCurrentIndex( this->index( rowCount() - 1, 1 ) );
-      mTreeView->selectionModel()->select( this->index( rowCount() - 1, 1 ), QItemSelectionModel::Rows );
+      emit selectItem( this->index( rowCount() - 1, 1 ) );
     }
     else
     {
       //Move from project to SQLite storage
       moveBookmark( mProjectTableModel, mQgisTableModel, index.row() - mQgisTableModel.rowCount() );
-      mTreeView->scrollTo( this->index( mQgisTableModel.rowCount() - 1, 1 ) );
-      mTreeView->setCurrentIndex( this->index( mQgisTableModel.rowCount() - 1, 1 ) );
-      mTreeView->selectionModel()->select( this->index( mQgisTableModel.rowCount() - 1, 1 ), QItemSelectionModel::Rows );
+      emit selectItem( this->index( mQgisTableModel.rowCount() - 1, 1 ) );
     }
-    return true;
-  }
-
-  if ( index.row() < mQgisTableModel.rowCount() )
-  {
-    return mQgisTableModel.setData( index, value, role );
+    result = true;
   }
   else
   {
-    return mProjectTableModel.setData( this->index( index.row() - mQgisTableModel.rowCount(), index.column() ), value, role );
+
+    if ( index.row() < mQgisTableModel.rowCount() )
+    {
+      result = mQgisTableModel.setData( index, value, role );
+    }
+    else
+    {
+      result = mProjectTableModel.setData( this->index( index.row() - mQgisTableModel.rowCount(), index.column() ), value, role );
+    }
   }
+  if ( result )
+    emit dataChanged( index, index );
+  return result;
 }
 
 Qt::ItemFlags QgsMergedBookmarksTableModel::flags( const QModelIndex &index ) const
@@ -659,19 +674,18 @@ bool QgsMergedBookmarksTableModel::removeRows( int row, int count, const QModelI
 {
   Q_ASSERT( count == 1 );
   bool result;
-  beginRemoveRows( parent, row, row + count );
   if ( row < mQgisTableModel.rowCount() )
   {
     QSqlTableModel *qgisModel = static_cast<QSqlTableModel *>( &mQgisTableModel );
     Q_ASSERT( qgisModel );
     result = qgisModel->removeRows( row, count, parent );
-    qgisModel->select();
+    qgisModel->select(); // This updates row count in the model!
   }
   else
   {
     result = mProjectTableModel.removeRows( row - mQgisTableModel.rowCount(), count, parent );
   }
-  endRemoveRows();
+  allLayoutChanged();
   return result;
 }
 

src/app/qgsbookmarks.h

@@ -126,6 +126,10 @@ class QgsMergedBookmarksTableModel: public QAbstractTableModel
     bool projectAvailable() const;
     void moveBookmark( QAbstractTableModel &modelFrom, QAbstractTableModel &modelTo, int row );
 
+  signals:
+
+    void selectItem( const QModelIndex &index );
+
   private slots:
     void allLayoutChanged()
     {
@@ -156,7 +160,7 @@ class APP_EXPORT QgsBookmarks : public QgsDockWidget, private Ui::QgsBookmarksBa
   private:
     QSqlTableModel *mQgisModel = nullptr;
     QgsProjectBookmarksTableModel *mProjectModel = nullptr;
-    QgsMergedBookmarksTableModel *mModel = nullptr;
+    QgsMergedBookmarksTableModel *mMergedModel = nullptr;
     QgsBookmarksProxyModel *mProxyModel = nullptr;
 
     void saveWindowLocation();

src/core/layertree/qgslayertreemodel.cpp

@@ -501,7 +501,7 @@ void QgsLayerTreeModel::refreshLayerLegend( QgsLayerTreeLayer *nodeLayer )
 
   // update children
   int oldNodeCount = rowCount( idx );
-  beginRemoveRows( idx, 0, oldNodeCount - 1 );
+  beginRemoveRows( idx, 0, std::max( oldNodeCount - 1, 0 ) );
   removeLegendFromLayer( nodeLayer );
   endRemoveRows();
 

src/gui/symbology/qgscategorizedsymbolrendererwidget.cpp

@@ -58,7 +58,7 @@ void QgsCategorizedSymbolRendererModel::setRenderer( QgsCategorizedSymbolRendere
 {
   if ( mRenderer )
   {
-    beginRemoveRows( QModelIndex(), 0, mRenderer->categories().size() - 1 );
+    beginRemoveRows( QModelIndex(), 0, std::max( mRenderer->categories().size() - 1, 0 ) );
     mRenderer = nullptr;
     endRemoveRows();
   }