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/downloadfromdivecomputer.cpp b/qt-ui/downloadfromdivecomputer.cpp
> index c57aa1e..994f67d 100644
> --- a/qt-ui/downloadfromdivecomputer.cpp
> +++ b/qt-ui/downloadfromdivecomputer.cpp
> @@ -3,6 +3,7 @@
>  #include "mainwindow.h"
>  #include "divelistview.h"
>  #include "display.h"
> +#include "undocommands.h"
>  
>  #include <QTimer>
>  #include <QFileDialog>
> @@ -426,11 +427,14 @@ void DownloadFromDCWidget::on_ok_clicked()
>  		return;
>  
>  	// record all the dives in the 'real' dive_table
> +	QList<struct dive*> diveList;
>  	for (int i = 0; i < downloadTable.nr; i++) {
>  		if (diveImportedModel->data(diveImportedModel->index(i, 0),Qt::CheckStateRole) == Qt::Checked)
> -			record_dive(downloadTable.dives[i]);
> +			diveList.append(downloadTable.dives[i]);
>  		downloadTable.dives[i] = NULL;
>  	}
> +	UndoDownloadDives *undoCommand = new UndoDownloadDives(diveList);

I know this is nitpicking... but "UndoDownloadDives" seems like the wrong
name. It doesn't undo the downloading, it undoes the recording of the
downloaded dives, right?

> +	MainWindow::instance()->undoStack->push(undoCommand);
>  	downloadTable.nr = 0;
>  
>  	int uniqId, idx;
> diff --git a/qt-ui/undocommands.cpp b/qt-ui/undocommands.cpp
> index 012faaf..758b5eb 100644
> --- a/qt-ui/undocommands.cpp
> +++ b/qt-ui/undocommands.cpp
> @@ -154,3 +154,38 @@ void UndoMergeDives::redo()
>  	mark_divelist_changed(true);
>  	MainWindow::instance()->refreshDisplay();
>  }
> +
> +
> +UndoDownloadDives::UndoDownloadDives(QList<dive *> downloadedDives)
> +{
> +	dives = downloadedDives;
> +	setText("download dives");
> +}
> +
> +void UndoDownloadDives::undo()
> +{
> +	QList<struct dive*> newList;
> +	for (int i = 0; i < dives.count(); i++) {
> +		//make a copy of the dive before deleting it

WHY? They are already in the dives list, aren't they?

> +		struct dive* d = alloc_dive();
> +		copy_dive(dives.at(i), d);
> +		newList.append(d);
> +		//delete the dive
> +		delete_single_dive(get_divenr(dives.at(i)));
> +	}
> +	mark_divelist_changed(true);
> +	MainWindow::instance()->refreshDisplay();
> +	dives.clear();
> +	dives = newList;

So here you throw away your perfectly good list, only to replace it with
the new one...

> +}
> +
> +void UndoDownloadDives::redo()
> +{
> +	for (int i = 0; i < dives.count(); i++) {
> +		struct dive* d = alloc_dive();
> +		copy_dive(dives.at(i), d);
> +		record_dive(d);

see? The dives in dives (your variable names sometimes make it hard to
talk about the code :-) ) are copied before being recorded, so there's no
reason in undo to create that new list.

/D