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
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
