Request For Comments - Undo/Redo framework

[Dirk Hohndel]

On Wed, Mar 11, 2015 at 10:49:17PM +0300, Grace Karanja wrote:
> diff --git a/qt-ui/undocommands.cpp b/qt-ui/undocommands.cpp
> index 6a15f7e..012faaf 100644
> --- a/qt-ui/undocommands.cpp
> +++ b/qt-ui/undocommands.cpp
> +
> +UndoMergeDives::UndoMergeDives()
> +{
> +	setText("merge dives");
> +	int i;
> +	struct dive* dive = NULL;
> +
> +	for_each_dive(i, dive) {
> +		if (dive->selected) {
> +			struct dive* d = alloc_dive();
> +			copy_dive(dive, d);
> +			dives.append(d);

So we allocate / copy the dives that will be merged. Based on the
selection. This is fine as at the time this is called, we have the correct
selection.

> +void UndoMergeDives::undo()
> +{
> +	delete_single_dive(get_divenr(mainDive));
> +
> +	for (int i = 0; i < dives.count(); i++) {
> +		struct dive* dive = alloc_dive();
> +		copy_dive(dives.at(i), dive);
> +		record_dive(dive);

And now we allocate / copy again to insert these dives. Is this leaking
memory?

> +void UndoMergeDives::redo()
> +{
> +	struct dive* newDive = NULL;
> +
> +	for (int i = 0; i < dives.count(); i++) {
> +		struct dive* dive = alloc_dive();

So this allocs a dive

> +		dive = get_dive_by_uniq_id(dives.at(i)->id);

And then overwrites it with a dive from the list. So this leaks memory for
no good reason.

> +
> +		if (!can_merge(newDive, dive)) {
> +			newDive = dive;
> +		} else {
> +			newDive = merge_two_dives(newDive, dive);
> +		}
> +	}

Hmm - I need to think about this some more, but I wonder why this logic
would be inside the redo() function. If you have a selection where some
dives can merged and others can't, what happens if you repeatedly
undo/redo?
The undo function above simply records each dive that was in dives and
deleted the mainDive... so I'm pretty sure that this would start
duplicating the dives for which can_merge returns false, right?

Can you look through these comments, Grace?

/D