Fixing grouping/ungrouping crash

[strk]

See http://hub.qgis.org/issues/11371

[strk]

@nyalldawson this is ready for review, could you please take a look ?

The crash was basically due to unclear ownership of the "group" item, the code was deleting it on "ungroup" while it was still referenced by the UndoRedoCommand of the "group item move".

My patch drops the immediate deletion replacing it with an UndoRedoCommand that adds/remove the grouping, with "Remove" command taking ownership of the group.

[strk]

Ok it's still not ready as on "delete group item" we're still removing the items contained in the group while we should not be doing that.

[strk]

Also, "undo grouping" leaves the Scene with unselectable items

[strk]

Note that the premature deletion of items still results in a crash on exit (double free, I suspect):

#0  0x0000000000000000 in ?? ()
#1  0x00007f7fa1dd0ac0 in QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand (this=0x6768c70, __in_chrg=<optimized out>)
    at /usr/src/qgis/qgis-master/src/core/composer/qgsaddremoveitemcommand.cpp:33
#2  0x00007f7fa1dd0b80 in QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand (this=0x6768c70, __in_chrg=<optimized out>)
    at /usr/src/qgis/qgis-master/src/core/composer/qgsaddremoveitemcommand.cpp:35
#3  0x00007f7fa059246e in QUndoCommand::~QUndoCommand() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#4  0x00007f7fa1dd0ae2 in QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand (this=0x6633e90, __in_chrg=<optimized out>)
    at /usr/src/qgis/qgis-master/src/core/composer/qgsaddremoveitemcommand.cpp:29
#5  0x00007f7fa1dd0b80 in QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand (this=0x6633e90, __in_chrg=<optimized out>)
    at /usr/src/qgis/qgis-master/src/core/composer/qgsaddremoveitemcommand.cpp:35
#6  0x00007f7fa05926c6 in QUndoStack::clear() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#7  0x00007f7fa0592817 in QUndoStack::~QUndoStack() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#8  0x00007f7fa0592849 in QUndoStack::~QUndoStack() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#9  0x00007f7fa1f2fa18 in QgsComposition::~QgsComposition (this=0x63b8000, __in_chrg=<optimized out>)

[strk]

I've tested more and fixed more undo/redo issues. I'd add a few more tests (for position and undostack state) and then cleanup and merge.

[nyalldawson]

@strk great! will take a look first thing tomorrow

[strk]

Hopefully for tomorrow I'll be finished with tests and tweaks.
For example, I think the mechanism to decide on having or not ownership of the item, from the AddRemoveItemCommand, is too weak if we also toggle the command state on undo/redo

[strk]

More tests have been added. Travis still fails due to lack of SIP bindings update (due to the new QgsGroupUngroupItemsCommand class). No test was added to expose the possible memory leak (only theoretical so far)

[strk]

Tests for sent signals are also missing (itemAdded/itemRemoved being sent on "undo"/"redo" operations and more) -- @nyalldawson if you want to help there, I won't be on the code before ~10 hours from now

[nyalldawson]

no need to include private members in sip - they aren't accessible anyway

[strk]

how about signals, are them needed in SIP ?

[nyalldawson]

Yes, signals should be included

[nyalldawson]

@strk looks good to me. Honestly the whole ownership of items in composer is a mess, and desperately in need of reworking. In future I'd like to change it so items are always deleted when removed from the scene, and only the xml representation of the composition/items is stored by undo/redo commands.
But this looks like a good approach given the current architecture.

[strk]

Do you have any hint about testing signals dispatch ? As I'm not sure "itemAdded"/"itemRemoved" are sent correclty on undo/redo, and hadn't seen the current testcase checking for them.

[strk]

I'll play with QSignalSpy some before merging this.

[nyalldawson]

Given that it's a c++ test you can use QSignalSpy. See TestQgsAtlasComposition::test_signals() for similar code you could copy.

[strk]

Ok QSignalSpy testing gave its first fruits already.
It looks like QgsComposer::addItem(QgsComposerPolygon_) on itself does NOT send a composerPolygonAdded(QgsComposerPolygon_) -- should it ?

[strk]

Also, should grouping/ungrouping send its own signals ?

[strk]

Another thing cough bu the test: QgsComposition::ungroupItems( group ) -- emits an ItemRemoved signal for each member of the group. Seems to be WRONG to me, as there's eventually a single item removed, assuming the group is considered an item.

Likewise, there is no emission on grouping, which is inconsistent with the emissions on ungrouping.

[strk]

There, I think the code is good to ready for merging now.
I can further reduce/squash the commits if you think it's worth it.

If Travis is happy, I'd merge. Or feel free to merge it yourself Nyall, if you have more sun than I will in the next 10 hours :)

[strk]

squash-rebased for final check

[strk]

Gah, travis fails for unrelated issues: PyQgsWFSProviderGUI

[strk]

Rebase-pushed as a211c98

[nyalldawson]

Nice work @strk!

[strk]

@nyalldawson how do you feel about backporting this to 2.14 ? See #3209