Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix crash in composer on ungrouping after group move.
Closes #11371.

Adds support for undo/redo grouping/ungrouping operations.

Enable pending test for the crash (now passing) and add many more
undo/redo related ones (including signals testing).

Includes a new QgsGroupUngroupItemsCommand class
and its SIP bindings.
  • Loading branch information
strk committed Jun 17, 2016
1 parent b380a1d commit df8dca2
Show file tree
Hide file tree
Showing 11 changed files with 537 additions and 29 deletions.
2 changes: 2 additions & 0 deletions python/core/composer/qgscomposition.sip
Expand Up @@ -830,6 +830,8 @@ class QgsComposition : QGraphicsScene
void composerArrowAdded( QgsComposerArrow* arrow );
/** Is emitted when a new composer html has been added to the view*/
void composerHtmlFrameAdded( QgsComposerHtml* html, QgsComposerFrame* frame );
/** Is emitted when a new item group has been added to the view*/
void composerItemGroupAdded( QgsComposerItemGroup* group );
/** Is emitted when new composer label has been added to the view*/
void composerLabelAdded( QgsComposerLabel* label );
/** Is emitted when new composer map has been added to the view*/
Expand Down
43 changes: 43 additions & 0 deletions python/core/composer/qgsgroupungroupitemscommand.sip
@@ -0,0 +1,43 @@
/** A composer command class for grouping / ungrouping composer items.
*
* If mState == Ungrouped, the command owns the group item
*/
class QgsGroupUngroupItemsCommand: QObject, QUndoCommand
{
%TypeHeaderCode
#include "qgsgroupungroupitemscommand.h"
%End

public:

/** Command kind, and state */
enum State
{
Grouped = 0,
Ungrouped
};

/** Create a group or ungroup command
*
* @param s command kind (@see State)
* @param item the group item being created or ungrouped
* @param c the composition including this group
* @param text command label
* @param parent parent command, if any
*
*/
QgsGroupUngroupItemsCommand( State s, QgsComposerItemGroup* item, QgsComposition* c, const QString& text, QUndoCommand* parent = nullptr );
~QgsGroupUngroupItemsCommand();

void redo();
void undo();

signals:
/** Signals addition of an item (the group) */
void itemAdded( QgsComposerItem* item );
/** Signals removal of an item (the group) */
void itemRemoved( QgsComposerItem* item );

};


1 change: 1 addition & 0 deletions python/core/core.sip
Expand Up @@ -158,6 +158,7 @@
%Include auth/qgsauthmethod.sip

%Include composer/qgsaddremoveitemcommand.sip
%Include composer/qgsgroupungroupitemscommand.sip
%Include composer/qgsaddremovemultiframecommand.sip
%Include composer/qgsatlascomposition.sip
%Include composer/qgscomposerarrow.sip
Expand Down
2 changes: 2 additions & 0 deletions src/core/CMakeLists.txt
Expand Up @@ -253,6 +253,7 @@ SET(QGIS_CORE_SRCS
composer/qgscomposerutils.cpp
composer/qgscomposition.cpp
composer/qgsdoubleboxscalebarstyle.cpp
composer/qgsgroupungroupitemscommand.cpp
composer/qgslegendmodel.cpp
composer/qgsnumericscalebarstyle.cpp
composer/qgspaperitem.cpp
Expand Down Expand Up @@ -510,6 +511,7 @@ SET(QGIS_CORE_MOC_HDRS
composer/qgscomposertablev2.h
composer/qgscomposertexttable.h
composer/qgscomposition.h
composer/qgsgroupungroupitemscommand.h
composer/qgslegendmodel.h
composer/qgspaperitem.h

Expand Down
4 changes: 4 additions & 0 deletions src/core/composer/qgsaddremoveitemcommand.cpp
Expand Up @@ -36,6 +36,7 @@ QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand()

void QgsAddRemoveItemCommand::redo()
{
QUndoCommand::redo(); // call redo() on all childs
if ( mFirstRun )
{
mFirstRun = false;
Expand All @@ -46,6 +47,7 @@ void QgsAddRemoveItemCommand::redo()

void QgsAddRemoveItemCommand::undo()
{
QUndoCommand::undo(); // call undo() on all childs, in reverse order
if ( mFirstRun )
{
mFirstRun = false;
Expand All @@ -58,6 +60,7 @@ void QgsAddRemoveItemCommand::switchState()
{
if ( mState == Added )
{
// Remove
if ( mComposition )
{
mComposition->itemsModel()->setItemRemoved( mItem );
Expand All @@ -68,6 +71,7 @@ void QgsAddRemoveItemCommand::switchState()
}
else //Removed
{
// Add
if ( mComposition )
{
mComposition->itemsModel()->setItemRestored( mItem );
Expand Down
2 changes: 1 addition & 1 deletion src/core/composer/qgscomposeritemgroup.cpp
Expand Up @@ -36,7 +36,7 @@ QgsComposerItemGroup::~QgsComposerItemGroup()
//loop through group members and remove them from the scene
Q_FOREACH ( QgsComposerItem* item, mItems )
{
if ( !item )
if ( !item || item->isRemoved() )
continue;

//inform model that we are about to remove an item from the scene
Expand Down
66 changes: 53 additions & 13 deletions src/core/composer/qgscomposition.cpp
Expand Up @@ -33,6 +33,7 @@
#include "qgscomposerattributetablev2.h"
#include "qgsaddremovemultiframecommand.h"
#include "qgscomposermultiframecommand.h"
#include "qgsgroupungroupitemscommand.h"
#include "qgspaintenginehack.h"
#include "qgspaperitem.h"
#include "qgsproject.h"
Expand Down Expand Up @@ -1515,6 +1516,10 @@ void QgsComposition::addItemsFromXML( const QDomElement& elem, const QDomDocumen
QgsComposerItemGroup *newGroup = new QgsComposerItemGroup( this );
newGroup->readXML( groupElem, doc );
addItem( newGroup );
if ( addUndoCommands )
{
pushAddRemoveCommand( newGroup, tr( "Group added" ) );
}
}

//Since this function adds items grouped by type, and each item is added to end of
Expand Down Expand Up @@ -1940,14 +1945,28 @@ QgsComposerItemGroup *QgsComposition::groupItems( QList<QgsComposerItem *> items
}

QgsComposerItemGroup* itemGroup = new QgsComposerItemGroup( this );
QgsDebugMsg( QString( "itemgroup created with %1 items (%2 to be added)" ) .arg( itemGroup->items().size() ).arg( items.size() ) );

QList<QgsComposerItem*>::iterator itemIter = items.begin();
for ( ; itemIter != items.end(); ++itemIter )
{
itemGroup->addItem( *itemIter );
QgsDebugMsg( QString( "itemgroup now has %1" )
.arg( itemGroup->items().size() ) );
}

addItem( itemGroup );

QgsGroupUngroupItemsCommand* c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Grouped, itemGroup, this, tr( "Items grouped" ) );
QObject::connect( c, SIGNAL( itemRemoved( QgsComposerItem* ) ), this, SIGNAL( itemRemoved( QgsComposerItem* ) ) );
QObject::connect( c, SIGNAL( itemAdded( QgsComposerItem* ) ), this, SLOT( sendItemAddedSignal( QgsComposerItem* ) ) );

undoStack()->push( c );
QgsProject::instance()->setDirty( true );
//QgsDebugMsg( QString( "itemgroup after pushAddRemove has %1" ) .arg( itemGroup->items().size() ) );

emit composerItemGroupAdded( itemGroup );

return itemGroup;
}

Expand All @@ -1959,6 +1978,17 @@ QList<QgsComposerItem *> QgsComposition::ungroupItems( QgsComposerItemGroup* gro
return ungroupedItems;
}

// group ownership transferred to QgsGroupUngroupItemsCommand
// Call this before removing group items so it can keep note
// of contents
QgsGroupUngroupItemsCommand* c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Ungrouped, group, this, tr( "Items ungrouped" ) );
QObject::connect( c, SIGNAL( itemRemoved( QgsComposerItem* ) ), this, SIGNAL( itemRemoved( QgsComposerItem* ) ) );
QObject::connect( c, SIGNAL( itemAdded( QgsComposerItem* ) ), this, SLOT( sendItemAddedSignal( QgsComposerItem* ) ) );

undoStack()->push( c );
QgsProject::instance()->setDirty( true );


QSet<QgsComposerItem*> groupedItems = group->items();
QSet<QgsComposerItem*>::iterator itemIt = groupedItems.begin();
for ( ; itemIt != groupedItems.end(); ++itemIt )
Expand All @@ -1967,10 +1997,9 @@ QList<QgsComposerItem *> QgsComposition::ungroupItems( QgsComposerItemGroup* gro
}

group->removeItems();
removeComposerItem( group, false, false );

emit itemRemoved( group );
delete( group );
// note: emits itemRemoved
removeComposerItem( group, false, false );

return ungroupedItems;
}
Expand Down Expand Up @@ -2542,6 +2571,7 @@ void QgsComposition::addComposerTableFrame( QgsComposerAttributeTableV2 *table,
emit composerTableFrameAdded( table, frame );
}

/* public */
void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool createCommand, const bool removeGroupItems )
{
QgsComposerMap* map = dynamic_cast<QgsComposerMap *>( item );
Expand All @@ -2550,25 +2580,36 @@ void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool creat
{
mItemsModel->setItemRemoved( item );
removeItem( item );
emit itemRemoved( item );

QgsDebugMsg( QString( "removeComposerItem called, createCommand:%1 removeGroupItems:%2" )
.arg( createCommand ).arg( removeGroupItems ) );

QgsComposerItemGroup* itemGroup = dynamic_cast<QgsComposerItemGroup*>( item );
if ( itemGroup && removeGroupItems )
{
//add add/remove item command for every item in the group
QUndoCommand* parentCommand = new QUndoCommand( tr( "Remove item group" ) );
QgsDebugMsg( QString( "itemGroup && removeGroupItems" ) );

// Takes ownership of itemGroup
QgsAddRemoveItemCommand* parentCommand = new QgsAddRemoveItemCommand(
QgsAddRemoveItemCommand::Removed, itemGroup, this,
tr( "Remove item group" ) );
connectAddRemoveCommandSignals( parentCommand );

//add add/remove item command for every item in the group
QSet<QgsComposerItem*> groupedItems = itemGroup->items();
QgsDebugMsg( QString( "itemGroup contains %1 items" ) .arg( groupedItems.size() ) );
QSet<QgsComposerItem*>::iterator it = groupedItems.begin();
for ( ; it != groupedItems.end(); ++it )
{
mItemsModel->setItemRemoved( *it );
removeItem( *it );
QgsAddRemoveItemCommand* subcommand = new QgsAddRemoveItemCommand( QgsAddRemoveItemCommand::Removed, *it, this, "", parentCommand );
connectAddRemoveCommandSignals( subcommand );
emit itemRemoved( *it );
}

undoStack()->push( parentCommand );
emit itemRemoved( itemGroup );
delete itemGroup;
}
else
{
Expand All @@ -2580,19 +2621,13 @@ void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool creat
{
multiFrame = static_cast<QgsComposerFrame*>( item )->multiFrame();
item->beginItemCommand( tr( "Frame deleted" ) );
emit itemRemoved( item );
item->endItemCommand();
}
else
{
emit itemRemoved( item );
pushAddRemoveCommand( item, tr( "Item deleted" ), QgsAddRemoveItemCommand::Removed );
}
}
else
{
emit itemRemoved( item );
}

//check if there are frames left. If not, remove the multi frame
if ( frameItem && multiFrame )
Expand Down Expand Up @@ -2715,6 +2750,11 @@ void QgsComposition::sendItemAddedSignal( QgsComposerItem* item )
emit selectedItemChanged( frame );
return;
}
QgsComposerItemGroup* group = dynamic_cast<QgsComposerItemGroup*>( item );
if ( group )
{
emit composerItemGroupAdded( group );
}
}

void QgsComposition::updatePaperItems()
Expand Down
2 changes: 2 additions & 0 deletions src/core/composer/qgscomposition.h
Expand Up @@ -1061,6 +1061,8 @@ class CORE_EXPORT QgsComposition : public QGraphicsScene
void composerArrowAdded( QgsComposerArrow* arrow );
/** Is emitted when a new composer html has been added to the view*/
void composerHtmlFrameAdded( QgsComposerHtml* html, QgsComposerFrame* frame );
/** Is emitted when a new item group has been added to the view*/
void composerItemGroupAdded( QgsComposerItemGroup* group );
/** Is emitted when new composer label has been added to the view*/
void composerLabelAdded( QgsComposerLabel* label );
/** Is emitted when new composer map has been added to the view*/
Expand Down
96 changes: 96 additions & 0 deletions src/core/composer/qgsgroupungroupitemscommand.cpp
@@ -0,0 +1,96 @@
/***************************************************************************
qgsgroupungroupitemscommand.cpp
---------------------------
begin : 2016-06-09
copyright : (C) 2016 by Sandro Santilli
email : strk at kbt dot io
***************************************************************************/

/***************************************************************************
* *
* 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 "qgsgroupungroupitemscommand.h"
#include "qgscomposeritem.h"
#include "qgscomposeritemgroup.h"
#include "qgscomposition.h"
#include "qgsproject.h"
#include "qgscomposermodel.h"
#include "qgslogger.h"

QgsGroupUngroupItemsCommand::QgsGroupUngroupItemsCommand( State s, QgsComposerItemGroup* item, QgsComposition* c, const QString& text, QUndoCommand* parent ):
QUndoCommand( text, parent ), mGroup( item ), mComposition( c ), mState( s ), mFirstRun( true )
{
mItems = mGroup->items();
}

QgsGroupUngroupItemsCommand::~QgsGroupUngroupItemsCommand()
{
if ( mState == Ungrouped )
{
//command class stores the item if ungrouped from the composition
delete mGroup;
}
}

void QgsGroupUngroupItemsCommand::redo()
{
if ( mFirstRun )
{
mFirstRun = false;
return;
}
switchState();
}

void QgsGroupUngroupItemsCommand::undo()
{
if ( mFirstRun )
{
mFirstRun = false;
return;
}
switchState();
}

void QgsGroupUngroupItemsCommand::switchState()
{
if ( mState == Grouped )
{
// ungroup
if ( mComposition )
{
// This is probably redundant
mComposition->itemsModel()->setItemRemoved( mGroup );
mComposition->removeItem( mGroup );
}
mGroup->removeItems();
emit itemRemoved( mGroup );
mState = Ungrouped;
}
else //Ungrouped
{
// group
if ( mComposition )
{
//delete mGroup; mGroup = new QgsComposerItemGroup( mCompoiser );
QSet<QgsComposerItem*>::iterator itemIter = mItems.begin();
for ( ; itemIter != mItems.end(); ++itemIter )
{
mGroup->addItem( *itemIter );
QgsDebugMsg( QString( "itemgroup now has %1" ) .arg( mGroup->items().size() ) );
}
// Add the group
mComposition->itemsModel()->setItemRestored( mGroup );
mComposition->addItem( mGroup );
}
mState = Grouped;
emit itemAdded( mGroup );
}
QgsProject::instance()->setDirty( true );
}

0 comments on commit df8dca2

Please sign in to comment.