Request For Comments - Undo/Redo framework

[Dirk Hohndel]

On Thu, Mar 12, 2015 at 09:13:06AM +0300, Grace Karanja wrote:
> diff --git a/qt-ui/divelistview.cpp b/qt-ui/divelistview.cpp
> index bded95e..d1c5260 100644
> --- a/qt-ui/divelistview.cpp
> +++ b/qt-ui/divelistview.cpp
> @@ -598,15 +598,13 @@ void DiveListView::removeFromTrip()
>  	//TODO: move this to C-code.
>  	int i;
>  	struct dive *d;
> +	QMap<struct dive*, dive_trip*> dives;

So sometimes "dives" is a list of dives. Sometimes you use "diveList" for
a list of dives (I like that better), and now "dives" is a map from dive
to trip (I really dislike this). Can you come up with a consistent naming
scheme for your variables and try to stick with it? I know we are not very
good rolemodels in the overall code, but here with you as the only author,
this would be good.

>  	for_each_dive (i, d) {
>  		if (d->selected)
> -			remove_dive_from_trip(d, false);
> +			dives.insert(d, d->divetrip);
>  	}
> -	rememberSelection();
> -	reload(currentLayout, false);
> -	fixMessyQtModelBehaviour();
> -	restoreSelection();
> -	mark_divelist_changed(true);

You remove the selection handling here...

> +	UndoRemoveDivesFromTrip *undoCommand = new UndoRemoveDivesFromTrip(dives);
> +	MainWindow::instance()->undoStack->push(undoCommand);
>  }
>  
>  void DiveListView::newTripAbove()
> diff --git a/qt-ui/undocommands.cpp b/qt-ui/undocommands.cpp
> index 758b5eb..cd776e0 100644
> --- a/qt-ui/undocommands.cpp
> +++ b/qt-ui/undocommands.cpp
> @@ -189,3 +189,32 @@ void UndoDownloadDives::redo()
>  	mark_divelist_changed(true);
>  	MainWindow::instance()->refreshDisplay();
>  }
> +
> +
> +UndoRemoveDivesFromTrip::UndoRemoveDivesFromTrip(QMap<dive *, dive_trip *> movedDives)
> +{
> +	dives = movedDives;

See above. "dives" is not a good name

> +	setText("remove dives from trip");
> +}
> +
> +void UndoRemoveDivesFromTrip::undo()
> +{
> +	QMapIterator<dive*, dive_trip*> i(dives);
> +	while (i.hasNext()) {
> +		i.next();
> +		add_dive_to_trip(i.key(), i.value());
> +	}
> +	mark_divelist_changed(true);
> +	MainWindow::instance()->refreshDisplay();
> +}
> +
> +void UndoRemoveDivesFromTrip::redo()
> +{
> +	QMapIterator<dive*, dive_trip* > i(dives);
> +	while (i.hasNext()) {
> +		i.next();
> +		remove_dive_from_trip(i.key(), false);
> +	}
> +	mark_divelist_changed(true);
> +	MainWindow::instance()->refreshDisplay();

You don't do the selection handling here - which I think is correct.
We shouldn't mess with the selection in the undo/redo cycle. But that
means we shouldn't have removed it above...

/D