Skip to content

Commit

Permalink
Fix crash when reordering composer items via drag and drop
Browse files Browse the repository at this point in the history
Caused by internal Qt bug when multiple QSortFilterProxyModels
used by widgets are attached to a parent model which calls
beginMoveRows.

Adds some tests, but none reproduce the crash. Not reproducable
on Qt5 builds.
  • Loading branch information
nyalldawson committed May 27, 2017
1 parent b0268ef commit cb33c0d
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 0 deletions.
5 changes: 5 additions & 0 deletions src/core/composer/qgscomposermodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/core/composer/qgscomposermodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ class CORE_EXPORT QgsComposerModel: public QAbstractItemModel
void rebuildSceneItemList();

friend class TestQgsComposerModel;
friend class TestQgsComposerGui;
};


Expand Down
24 changes: 24 additions & 0 deletions tests/src/core/testqgscomposermodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"
2 changes: 2 additions & 0 deletions tests/src/gui/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -141,4 +142,5 @@ ADD_QGIS_TEST(scalecombobox testqgsscalecombobox.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)

117 changes: 117 additions & 0 deletions tests/src/gui/testqgscomposergui.cpp
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit cb33c0d

Please sign in to comment.