Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix crash with undo/redo and layout multiframes, restore test
  • Loading branch information
nyalldawson committed Jan 17, 2018
1 parent 479358e commit fe48aec
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 56 deletions.
8 changes: 4 additions & 4 deletions python/core/layout/qgslayoutmultiframe.sip.in
Expand Up @@ -176,26 +176,26 @@ Returns the resize mode for the multiframe.
.. seealso:: :py:func:`setResizeMode`
%End

bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false ) const;
bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false ) const;
%Docstring
Stores the multiframe state in a DOM element.

:param parentElement: parent DOM element (e.g. 'Layout' element)
:param document: DOM document
:param context: read write context
:param ignoreFrames: set to false to avoid writing state information about child frames into DOM
:param includeFrames: set to true to write state information about child frames into DOM

.. seealso:: :py:func:`readXml`
%End

bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false );
bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false );
%Docstring
Sets the item state from a DOM element.

:param itemElement: is the DOM node corresponding to item (e.g. 'LayoutItem' element)
:param document: DOM document
:param context: read write context
:param ignoreFrames: set to false to avoid read state information about child frames from DOM
:param includeFrames: set to true to read state information about child frames from DOM

.. seealso:: :py:func:`writeXml`
%End
Expand Down
2 changes: 1 addition & 1 deletion src/app/layout/qgslayoutattributetablewidget.cpp
Expand Up @@ -394,7 +394,7 @@ void QgsLayoutAttributeTableWidget::mBackgroundColorButton_colorChanged( const Q

void QgsLayoutAttributeTableWidget::updateGuiElements()
{
if ( !mTable )
if ( !mTable || !mFrame )
{
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/layout/qgslayouthtmlwidget.cpp
Expand Up @@ -419,7 +419,7 @@ void QgsLayoutHtmlWidget::mAddFramePushButton_clicked()

void QgsLayoutHtmlWidget::setGuiElementValues()
{
if ( !mHtml )
if ( !mHtml || !mFrame )
{
return;
}
Expand Down
63 changes: 23 additions & 40 deletions src/core/layout/qgslayoutmultiframe.cpp
Expand Up @@ -453,34 +453,26 @@ int QgsLayoutMultiFrame::frameIndex( QgsLayoutFrame *frame ) const
return mFrameItems.indexOf( frame );
}

bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &doc, const QgsReadWriteContext &context, bool ignoreFrames ) const
bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &doc, const QgsReadWriteContext &context, bool includeFrames ) const
{
QDomElement element = doc.createElement( QStringLiteral( "LayoutMultiFrame" ) );
element.setAttribute( QStringLiteral( "resizeMode" ), mResizeMode );
element.setAttribute( QStringLiteral( "uuid" ), mUuid );
element.setAttribute( QStringLiteral( "type" ), type() );

#if 0 //TODO

if ( !ignoreFrames )
{
QList<QgsComposerFrame *>::const_iterator frameIt = mFrameItems.constBegin();
for ( ; frameIt != mFrameItems.constEnd(); ++frameIt )
{
( *frameIt )->writeXml( elem, doc );
}
}
#else
Q_UNUSED( ignoreFrames );
#endif

for ( QgsLayoutFrame *frame : mFrameItems )
{
if ( !frame )
continue;

QDomElement childItem = doc.createElement( QStringLiteral( "childFrame" ) );
childItem.setAttribute( QStringLiteral( "uuid" ), frame->uuid() );

if ( includeFrames )
{
frame->writeXml( childItem, doc, context );
}

element.appendChild( childItem );
}

Expand All @@ -490,7 +482,7 @@ bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &do
return true;
}

bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocument &doc, const QgsReadWriteContext &context, bool ignoreFrames )
bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocument &doc, const QgsReadWriteContext &context, bool includeFrames )
{
if ( element.nodeName() != QStringLiteral( "LayoutMultiFrame" ) )
{
Expand All @@ -504,29 +496,6 @@ bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocumen

mUuid = element.attribute( QStringLiteral( "uuid" ), QUuid::createUuid().toString() );
mResizeMode = static_cast< ResizeMode >( element.attribute( QStringLiteral( "resizeMode" ), QStringLiteral( "0" ) ).toInt() );
#if 0 //TODO
if ( !ignoreFrames )
{
deleteFrames();
}


if ( !ignoreFrames )
{
QDomNodeList frameList = itemElem.elementsByTagName( QStringLiteral( "ComposerFrame" ) );
for ( int i = 0; i < frameList.size(); ++i )
{
QDomElement frameElem = frameList.at( i ).toElement();
QgsComposerFrame *newFrame = new QgsComposerFrame( mComposition, this, 0, 0, 0, 0 );
newFrame->readXml( frameElem, doc );
addFrame( newFrame, false );
}

//TODO - think there should be a recalculateFrameSizes() call here
}
#else
Q_UNUSED( ignoreFrames );
#endif

deleteFrames();
mFrameUuids.clear();
Expand All @@ -537,8 +506,22 @@ bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocumen
if ( !elementNode.isElement() )
continue;

QString uuid = elementNode.toElement().attribute( QStringLiteral( "uuid" ) );
QDomElement frameElement = elementNode.toElement();

QString uuid = frameElement.attribute( QStringLiteral( "uuid" ) );
mFrameUuids << uuid;

if ( includeFrames )
{
QDomNodeList frameNodes = frameElement.elementsByTagName( QStringLiteral( "LayoutItem" ) );
if ( !frameNodes.isEmpty() )
{
QDomElement frameItemElement = frameNodes.at( 0 ).toElement();
std::unique_ptr< QgsLayoutFrame > newFrame = qgis::make_unique< QgsLayoutFrame >( mLayout, this );
newFrame->readXml( frameItemElement, doc, context );
addFrame( newFrame.release(), false );
}
}
}

bool result = readPropertiesFromElement( element, doc, context );
Expand Down
8 changes: 4 additions & 4 deletions src/core/layout/qgslayoutmultiframe.h
Expand Up @@ -194,20 +194,20 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
* \param parentElement parent DOM element (e.g. 'Layout' element)
* \param document DOM document
* \param context read write context
* \param ignoreFrames set to false to avoid writing state information about child frames into DOM
* \param includeFrames set to true to write state information about child frames into DOM
* \see readXml()
*/
bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false ) const;
bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false ) const;

/**
* Sets the item state from a DOM element.
* \param itemElement is the DOM node corresponding to item (e.g. 'LayoutItem' element)
* \param document DOM document
* \param context read write context
* \param ignoreFrames set to false to avoid read state information about child frames from DOM
* \param includeFrames set to true to read state information about child frames from DOM
* \see writeXml()
*/
bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false );
bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false );

/**
* Returns a list of all child frames for this multiframe.
Expand Down
4 changes: 2 additions & 2 deletions src/core/layout/qgslayoutmultiframeundocommand.cpp
Expand Up @@ -56,7 +56,7 @@ void QgsLayoutMultiFrameUndoCommand::saveState( QDomDocument &stateDoc ) const
QgsLayoutMultiFrame *item = mLayout->multiFrameByUuid( mFrameUuid );
Q_ASSERT_X( item, "QgsLayoutMultiFrameUndoCommand::saveState", "could not retrieve item for saving state" );

item->writeXml( documentElement, stateDoc, QgsReadWriteContext() );
item->writeXml( documentElement, stateDoc, QgsReadWriteContext(), true );
stateDoc.appendChild( documentElement );
}

Expand All @@ -70,7 +70,7 @@ void QgsLayoutMultiFrameUndoCommand::restoreState( QDomDocument &stateDoc )
item = recreateItem( mItemType, mLayout );
}

item->readXml( stateDoc.documentElement().firstChild().toElement(), stateDoc, QgsReadWriteContext() );
item->readXml( stateDoc.documentElement().firstChild().toElement(), stateDoc, QgsReadWriteContext(), true );
mLayout->project()->setDirty( true );
}

Expand Down
6 changes: 2 additions & 4 deletions tests/src/core/testqgslayoutmultiframe.cpp
Expand Up @@ -44,11 +44,9 @@ class TestQgsLayoutMultiFrame : public QObject
void displayName();
void frameIsEmpty(); //test if frame is empty works
void addRemovePage(); //test if page is added and removed for RepeatUntilFinished mode
#if 0 //TODO
void undoRedo(); //test that combinations of frame/multiframe undo/redo don't crash
void undoRedoRemovedFrame(); //test that undo doesn't crash with removed frames
void undoRedoRemovedFrame2();
#endif
void registry();
void deleteFrame();
void writeReadXml();
Expand Down Expand Up @@ -344,7 +342,7 @@ void TestQgsLayoutMultiFrame::addRemovePage()
mLayout->removeMultiFrame( htmlItem );
delete htmlItem;
}
#if 0//TODO

void TestQgsLayoutMultiFrame::undoRedo()
{
QgsLayoutItemHtml *htmlItem = new QgsLayoutItemHtml( mLayout );
Expand Down Expand Up @@ -513,7 +511,7 @@ void TestQgsLayoutMultiFrame::undoRedoRemovedFrame2()
htmlItem->addFrame( frame1 );

}
#endif

void TestQgsLayoutMultiFrame::registry()
{
// test QgsLayoutItemRegistry
Expand Down

0 comments on commit fe48aec

Please sign in to comment.