Merge pull request #4640 from nyalldawson/fix_drag_crash

Fix crash when reordering composer items via drag and drop

Author: nyalldawson

src/core/composer/qgscomposermodel.cpp

@@ -958,10 +958,15 @@ QgsComposerProxyModel::QgsComposerProxyModel( QgsComposition *composition, QObje
   if ( mComposition )
     setSourceModel( mComposition->itemsModel() );
 
+  // WARNING: the below code triggers a Qt bug (tested on Qt 4.8, can't reproduce on Qt5) whenever the QgsComposerModel source model
+  // calls beginInsertRows - since it's non functional anyway it's now disabled
+  // PLEASE verify that the Qt issue is fixed before reenabling
+#if 0
   // TODO doesn't seem to work correctly - not updated when item changes
   setDynamicSortFilter( true );
   setSortLocaleAware( true );
   sort( QgsComposerModel::ItemId );
+#endif
 }
 
 bool QgsComposerProxyModel::lessThan( const QModelIndex &left, const QModelIndex &right ) const

src/core/composer/qgscomposermodel.h

@@ -289,6 +289,7 @@ class CORE_EXPORT QgsComposerModel: public QAbstractItemModel
     void rebuildSceneItemList();
 
     friend class TestQgsComposerModel;
+    friend class TestQgsComposerGui;
 };
 
 

tests/src/core/testqgscomposermodel.cpp

@@ -62,6 +62,8 @@ class TestQgsComposerModel : public QObject
     void reorderToTopWithRemoved(); //test reordering to top with removed items
     void reorderToBottomWithRemoved(); //test reordering to bottom with removed items
 
+    void proxyCrash();
+
   private:
     QgsComposition *mComposition;
     QgsMapSettings *mMapSettings;
@@ -610,5 +612,27 @@ void TestQgsComposerModel::reorderToBottomWithRemoved()
   QCOMPARE( mComposition->itemsModel()->mItemsInScene.at( 1 ), mItem2 );
 }
 
+void TestQgsComposerModel::proxyCrash()
+{
+  // test for a possible crash when using QgsComposerProxyModel and reordering items
+  QgsMapSettings ms;
+  QgsComposition* composition = new QgsComposition( ms );
+
+  // create a proxy - it's not used, but will be watching...
+  QgsComposerProxyModel* proxy = new QgsComposerProxyModel( composition );
+  Q_UNUSED( proxy );
+
+  // add some items to composition
+  QgsComposerLabel* item1 = new QgsComposerLabel( composition );
+  composition->addItem( item1 );
+  QgsComposerLabel* item2 = new QgsComposerLabel( composition );
+  composition->addItem( item2 );
+  QgsComposerLabel* item3 = new QgsComposerLabel( composition );
+  composition->addItem( item3 );
+
+  // reorder items - expect no crash!
+  composition->itemsModel()->reorderItemUp( item1 );
+}
+
 QTEST_MAIN( TestQgsComposerModel )
 #include "testqgscomposermodel.moc"

tests/src/gui/CMakeLists.txt

@@ -16,6 +16,7 @@ INCLUDE_DIRECTORIES(${CMAKE_CURRENT_SOURCE_DIR}
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/gui/raster
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core/auth
+  ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core/composer
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core/geometry
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core/raster
   ${CMAKE_CURRENT_SOURCE_DIR}/../../../src/core/symbology-ng
@@ -142,4 +143,5 @@ ADD_QGIS_TEST(scalerangewidget testqgsscalerangewidget.cpp)
 ADD_QGIS_TEST(spinbox testqgsspinbox.cpp)
 ADD_QGIS_TEST(sqlcomposerdialog testqgssqlcomposerdialog.cpp)
 ADD_QGIS_TEST(filedownloader testqgsfiledownloader.cpp)
+ADD_QGIS_TEST(composergui testqgscomposergui.cpp)
 

tests/src/gui/testqgscomposergui.cpp

@@ -0,0 +1,117 @@
+/***************************************************************************
+    testqgscomposergui.cpp
+     ----------------------
+    Date                 : May 2017
+    Copyright            : (C) 2017 Nyall Dawson
+    Email                : nyall dot dawson at gmail dot com
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+
+
+
+#include <QtTest/QtTest>
+
+#include "qgsmapsettings.h"
+#include "qgscomposition.h"
+#include "qgscomposerlabel.h"
+#include "qgscomposermap.h"
+#include "qgscomposermodel.h"
+#include "qgscomposeritemcombobox.h"
+
+#include <QApplication>
+#include <QMainWindow>
+
+class TestQgsComposerGui: public QObject
+{
+    Q_OBJECT
+  private slots:
+    void initTestCase(); // will be called before the first testfunction is executed.
+    void cleanupTestCase(); // will be called after the last testfunction was executed.
+    void init(); // will be called before each testfunction is executed.
+    void cleanup(); // will be called after every testfunction.
+
+    void testProxyCrash();
+
+  private:
+
+};
+
+void TestQgsComposerGui::initTestCase()
+{
+}
+
+void TestQgsComposerGui::cleanupTestCase()
+{
+}
+
+void TestQgsComposerGui::init()
+{
+}
+
+void TestQgsComposerGui::cleanup()
+{
+}
+
+void TestQgsComposerGui::testProxyCrash()
+{
+  // test for a possible crash when using QgsComposerProxyModel and reordering items
+  QgsMapSettings ms;
+  QgsComposition* composition = new QgsComposition( ms );
+
+  // create a composer item combobox
+  QgsComposerItemComboBox* cb = new QgsComposerItemComboBox( nullptr, composition );
+  QgsComposerItemComboBox* cb2 = new QgsComposerItemComboBox( nullptr, composition );
+  QgsComposerItemComboBox* cb3 = new QgsComposerItemComboBox( nullptr, composition );
+  QgsComposerItemComboBox* cb4 = new QgsComposerItemComboBox( nullptr, composition );
+
+  cb->show();
+  cb2->show();
+
+  // add some items to composition
+  QgsComposerMap* item1 = new QgsComposerMap( composition );
+  composition->addItem( item1 );
+  QgsComposerLabel* item2 = new QgsComposerLabel( composition );
+  composition->addItem( item2 );
+  QgsComposerMap* item3 = new QgsComposerMap( composition );
+  composition->addItem( item3 );
+
+  QCOMPARE( cb->count(), 3 );
+  cb->setItemType( QgsComposerItem::ComposerMap );
+  QCOMPARE( cb->count(), 2 );
+
+  cb4->setItemType( QgsComposerItem::ComposerLabel );
+
+  cb->setItem( item1 );
+  QCOMPARE( cb->currentItem(), item1 );
+  cb2->setItem( item3 );
+  QCOMPARE( cb2->currentItem(), item3 );
+  cb3->setItem( item2 );
+  QCOMPARE( cb3->currentItem(), item2 );
+
+  // reorder items - expect no crash!
+  // we do this by calling the private members, in order to simulate what
+  // happens when a drag and drop reorder occurs
+  composition->itemsModel()->mItemZList.removeOne( item1 );
+  composition->itemsModel()->mItemZList.insert( 1, item1 );
+  composition->itemsModel()->rebuildSceneItemList();
+
+  QCOMPARE( cb->currentItem(), item1 );
+  QCOMPARE( cb2->currentItem(), item3 );
+
+  composition->itemsModel()->mItemZList.removeOne( item1 );
+  composition->itemsModel()->mItemZList.insert( 0, item1 );
+  composition->itemsModel()->rebuildSceneItemList();
+
+  delete composition;
+}
+
+
+QTEST_MAIN( TestQgsComposerGui )
+#include "testqgscomposergui.moc"