Skip to content

Commit

Permalink
Merge map layer style undo commands that are created in short time (P…
Browse files Browse the repository at this point in the history
…R 3277)

This makes the undo stack grow slower (especially when typing or repeatedly
changing a value) and preserves memory.
  • Loading branch information
wonder-sk authored Jul 7, 2016
1 parent 9b102c2 commit fd446cf
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 6 deletions.
28 changes: 23 additions & 5 deletions src/app/qgslayerstylingwidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -449,19 +449,18 @@ void QgsLayerStylingWidget::pushUndoItem( const QString &name )
QDomElement rootNode = doc.createElement( "qgis" );
doc.appendChild( rootNode );
mCurrentLayer->writeStyle( rootNode, doc, errorMsg );
mCurrentLayer->undoStackStyles()->beginMacro( name );
mCurrentLayer->undoStackStyles()->push( new QgsMapLayerStyleCommand( mCurrentLayer, rootNode, mLastStyleXml ) );
mCurrentLayer->undoStackStyles()->endMacro();
mCurrentLayer->undoStackStyles()->push( new QgsMapLayerStyleCommand( mCurrentLayer, name, rootNode, mLastStyleXml ) );
// Override the last style on the stack
mLastStyleXml = rootNode.cloneNode();
}


QgsMapLayerStyleCommand::QgsMapLayerStyleCommand( QgsMapLayer *layer, const QDomNode &current, const QDomNode &last )
: QUndoCommand()
QgsMapLayerStyleCommand::QgsMapLayerStyleCommand( QgsMapLayer *layer, const QString& text, const QDomNode &current, const QDomNode &last )
: QUndoCommand( text )
, mLayer( layer )
, mXml( current )
, mLastState( last )
, mTime( QTime::currentTime() )
{
}

Expand All @@ -479,6 +478,25 @@ void QgsMapLayerStyleCommand::redo()
mLayer->triggerRepaint();
}

bool QgsMapLayerStyleCommand::mergeWith( const QUndoCommand* other )
{
if ( other->id() != id() ) // make sure other is also an QgsMapLayerStyleCommand
return false;

const QgsMapLayerStyleCommand* otherCmd = static_cast<const QgsMapLayerStyleCommand*>( other );
if ( otherCmd->mLayer != mLayer )
return false; // should never happen though...

// only merge commands if they are created shortly after each other
// (e.g. user keeps modifying one property)
if ( mTime.msecsTo( otherCmd->mTime ) > 3000 )
return false;

mXml = otherCmd->mXml;
mTime = otherCmd->mTime;
return true;
}

QIcon QgsLayerStyleManagerWidgetFactory::icon() const
{
return QgsApplication::getThemeIcon( "propertyicons/stylepreset.svg" );
Expand Down
11 changes: 10 additions & 1 deletion src/app/qgslayerstylingwidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,24 @@ class APP_EXPORT QgsLayerStyleManagerWidgetFactory : public QgsMapLayerConfigWid
class APP_EXPORT QgsMapLayerStyleCommand : public QUndoCommand
{
public:
QgsMapLayerStyleCommand( QgsMapLayer* layer, const QDomNode& current, const QDomNode& last );
QgsMapLayerStyleCommand( QgsMapLayer* layer, const QString& text, const QDomNode& current, const QDomNode& last );

/** Return unique ID for this kind of undo command.
* Currently we do not have a central registry of undo command IDs, so it is a random magic number.
*/
virtual int id() const override { return 0xbeef; }

virtual void undo() override;
virtual void redo() override;

/** Try to merge with other commands of this type when they are created in small time interval */
virtual bool mergeWith( const QUndoCommand* other ) override;

private:
QgsMapLayer* mLayer;
QDomNode mXml;
QDomNode mLastState;
QTime mTime;
};

class APP_EXPORT QgsLayerStylingWidget : public QWidget, private Ui::QgsLayerStylingWidgetBase
Expand Down

3 comments on commit fd446cf

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on fd446cf Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, great stuff.

Do you think it would be possible to batch them by topic rather than time? I think that would enhance it even more.

@wonder-sk
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we discussed that with @NathanW2 - that would be a preferable way to go. That will just need more work as we would need to add some kind of extra identifiers (everywhere) on what has changed - or have some diff for stored XML data to automatically identify the topic...

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on fd446cf Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I expected.
XML for this task sucks anyway ;)

Please sign in to comment.