Request For Comments - Undo/Redo framework

Dirk Hohndel dirk at hohndel.org
Thu Feb 26 09:11:04 PST 2015


Hi Grace...

On Tue, Feb 24, 2015 at 07:54:15PM +0300, Grace Karanja wrote:
> diff --git a/qt-ui/simplewidgets.cpp b/qt-ui/simplewidgets.cpp
> index f7944c9..7de067d 100644
> --- a/qt-ui/simplewidgets.cpp
> +++ b/qt-ui/simplewidgets.cpp
> @@ -142,8 +142,19 @@ void RenumberDialog::renumberOnlySelected(bool selected)
>  
>  void RenumberDialog::buttonClicked(QAbstractButton *button)
>  {
> -	if (ui.buttonBox->buttonRole(button) == QDialogButtonBox::AcceptRole)
> +	if (ui.buttonBox->buttonRole(button) == QDialogButtonBox::AcceptRole) {
> +		QMap<int,int> renumberedDives;
> +		int i;
> +		struct dive *dive = NULL;
> +		for_each_dive (i, dive) {
> +			if (dive->selected) {

Careful - you are ignoring the selectedOnly flag... so the condition
should be   if (!selected_only || dive->selected)

> +				renumberedDives.insert(dive->id, dive->number);
> +			}
> +		}
> +		UndoRenumberDives *undoCommand = new UndoRenumberDives(renumberedDives, ui.spinBox->value());
> +		MainWindow::instance()->undoStack->push(undoCommand);
>  		renumber_dives(ui.spinBox->value(), selectedOnly);

Oh, so this time you ARE calling the activity function from here instead
of having the redo registration take care of that. That feels a little
inconsistent with your last patch...

> diff --git a/qt-ui/undocommands.cpp b/qt-ui/undocommands.cpp
> index 316def4..a075e99 100644
> --- a/qt-ui/undocommands.cpp
> +++ b/qt-ui/undocommands.cpp
> @@ -62,3 +62,41 @@ void UndoShiftTime::redo()
>  	mark_divelist_changed(true);
>  	MainWindow::instance()->refreshDisplay();
>  }
> +
> +
> +UndoRenumberDives::UndoRenumberDives(QMap<int, int> originalNumbers, int startNumber)
> +{
> +	oldNumbers = originalNumbers;
> +	start = startNumber;
> +	isFirstRedo = true;

And because of that (previous comment) you need this weird flag...

> +	setText("renumber dive");
> +	if (oldNumbers.count() > 1)
> +		setText(QString("renumber %1 dives").arg(QString::number(oldNumbers.count())));
> +}
> +
> +void UndoRenumberDives::undo()
> +{
> +	foreach (int key, oldNumbers.keys()) {
> +		struct dive* d = get_dive_by_uniq_id(key);
> +		d->number = oldNumbers.value(key);
> +	}
> +	mark_divelist_changed(true);
> +	MainWindow::instance()->refreshDisplay();
> +}
> +
> +void UndoRenumberDives::redo()
> +{
> +	if (isFirstRedo) {
> +		isFirstRedo = false;
> +		return; //Skip the first redo. Qt calls redo() when the QUndoCommand is pushed
> +			//to the QUndoStack

Yeah, that's just silly - I liked what you did last time better. Put a
coment where the current call to renumber_dives() is so that no one gets
confused, but then have just one place where you do this operation,

Would you mind redoing / resubmitting the patch?

Thanks

/D


More information about the subsurface mailing list