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

Reply via email to