Undo: make adding of planned dive undo-able

Planned dives were still added by directly calling core code.
This could confuse the undo-machinery, leading to crashes.

Instead, use the proper undo-command. The problem is that as
opposed to the other AddDive-commands, planned dives may
belong to a trip. Thus, the interface to the AddDive command
was changed to respect the divetrip field. Make sure that
the other callers reset that field (actually, it should never
be set). Add a comment describing the perhaps surprising
interface (the passed-in dive, usually displayed dive, is
reset).

Moreover, a dive cloned in the planner is not assigned a
new number. Thus, add an argument to the AddDive-command,
which expresses whether a new number should be generated
for the to-be-added dive.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

desktop-widgets/command.cpp

@@ -6,9 +6,9 @@
 namespace Command {
 
 // Dive-list related commands
-void addDive(dive *d, bool autogroup)
+void addDive(dive *d, bool autogroup, bool newNumber)
 {
-	execute(new AddDive(d, autogroup));
+	execute(new AddDive(d, autogroup, newNumber));
 }
 
 void deleteDive(const QVector<struct dive*> &divesToDelete)

desktop-widgets/command.h

@@ -9,13 +9,18 @@
 // We put everything in a namespace, so that we can shorten names without polluting the global namespace
 namespace Command {
 
-// General commands
+// 1) General commands
+
 void clear();				// Reset the undo stack. Delete all commands.
 QAction *undoAction(QObject *parent);	// Create an undo action.
 QAction *redoAction(QObject *parent);	// Create an redo action.
 
-// Dive-list related commands
+// 2) Dive-list related commands
-void addDive(dive *d, bool autogroup);
+
+void addDive(dive *d, bool autogroup, bool newNumber); // If d->dive_trip is null and autogroup is true, dives within the auto-group
+						       // distance are added to a trip. dive d is consumed (the structure is reset)!
+						       // If newNumber is true, the dive is assigned a new number, depending on the
+						       // insertion position.
 void deleteDive(const QVector<struct dive*> &divesToDelete);
 void shiftTime(const QVector<dive *> &changedDives, int amount);
 void renumberDives(const QVector<QPair<dive *, int>> &divesToRenumber);

desktop-widgets/command_divelist.cpp

@@ -483,21 +483,28 @@ void DiveListBase::redo()
 	finishWork();
 }
 
-AddDive::AddDive(dive *d, bool autogroup)
+AddDive::AddDive(dive *d, bool autogroup, bool newNumber)
 {
 	setText(tr("add dive"));
+	// By convention, d is "displayed dive" and can be overwritten.
 	d->maxdepth.mm = 0;
+	d->dc.maxdepth.mm = 0;
 	fixup_dive(d);
-	d->divetrip = nullptr;
 
-	// Get an owning pointer to a copy of the dive
+	// Get an owning pointer to a copied or moved dive
-	// Note: this destroys the old dive!
+	// Note: if move is true, this destroys the old dive!
 	OwningDivePtr divePtr(clone_dive(d));
+	divePtr->selected = false; // If we clone a planned dive, it might have been selected.
+				   // We have to clear the flag, as selections will be managed
+				   // on dive-addition.
 
 	// If we alloc a new-trip for autogrouping, get an owning pointer to it.
 	OwningTripPtr allocTrip;
-	dive_trip *trip = nullptr;
+	dive_trip *trip = divePtr->divetrip;
-	if (autogroup) {
+	// We have to delete the pointer-to-trip, because this would prevent the core from adding to the trip
+	// and we would get the count-of-dives in the trip wrong. Yes, that's all horribly subtle!
+	divePtr->divetrip = nullptr;
+	if (!trip && autogroup) {
 		bool alloc;
 		trip = get_trip_for_new_dive(divePtr.get(), &alloc);
 		if (alloc)
@@ -505,7 +512,8 @@ AddDive::AddDive(dive *d, bool autogroup)
 	}
 
 	int idx = dive_get_insertion_index(divePtr.get());
-	divePtr->number = get_dive_nr_at_idx(idx);
+	if (newNumber)
+		divePtr->number = get_dive_nr_at_idx(idx);
 
 	divesToAdd.push_back({ std::move(divePtr), std::move(allocTrip), trip, idx });
 }

desktop-widgets/command_divelist.h

@@ -82,7 +82,7 @@ private:
 
 class AddDive : public DiveListBase {
 public:
-	AddDive(dive *dive, bool autogroup);
+	AddDive(dive *dive, bool autogroup, bool newNumber);
 private:
 	void undoit() override;
 	void redoit() override;

desktop-widgets/mainwindow.cpp

@@ -890,19 +890,11 @@ void MainWindow::planCanceled()
 
 void MainWindow::planCreated()
 {
-	// get the new dive selected and assign a number if reasonable
-	graphics()->setProfileState();
-	if (displayed_dive.id == 0) {
-		// we might have added a new dive (so displayed_dive was cleared out by clone_dive()
-		dive_list()->unselectDives();
-		select_dive(get_dive(dive_table.nr - 1));
-		dive_list()->selectDive(get_divenr(current_dive)); // TODO: don't convert dive->idx and back
-		set_dive_nr_for_current_dive();
-	}
 	// make sure our UI is in a consistent state
-	information()->updateDiveInfo();
 	showProfile();
-	refreshDisplay();
+	setApplicationState("Default");
+	dive_list()->setEnabled(true);
+	dive_list()->setFocus();
 }
 
 void MainWindow::setPlanNotes()

desktop-widgets/tab-widgets/maintab.cpp

@@ -799,7 +799,7 @@ void MainTab::acceptChanges()
 		updateDiveSite(ui.location->currDiveSiteUuid(), &displayed_dive);
 		copyTagsToDisplayedDive();
 
-		Command::addDive(&displayed_dive, autogroup);
+		Command::addDive(&displayed_dive, autogroup, true);
 
 		editMode = NONE;
 		MainWindow::instance()->exitEditState();
@@ -810,8 +810,6 @@ void MainTab::acceptChanges()
 		ui.editDiveSiteButton->setEnabled(!ui.location->text().isEmpty());
 		emit addDiveFinished();
 		DivePlannerPointsModel::instance()->setPlanMode(DivePlannerPointsModel::NOTHING);
-		int scrolledBy = MainWindow::instance()->dive_list()->verticalScrollBar()->sliderPosition();
-		MainWindow::instance()->dive_list()->verticalScrollBar()->setSliderPosition(scrolledBy);
 		MainWindow::instance()->dive_list()->setFocus();
 		resetPallete();
 		displayed_dive.divetrip = nullptr; // Should not be necessary, just in case!

qt-models/diveplannermodel.cpp

@@ -7,6 +7,7 @@
 #include "core/device.h"
 #include "core/qthelper.h"
 #include "core/settings/qPrefDivePlanner.h"
+#include "desktop-widgets/command.h"
 #include "core/gettextfromc.h"
 #include <QApplication>
 #include <QTextDocument>
@@ -1110,15 +1111,9 @@ void DivePlannerPointsModel::createPlan(bool replanCopy)
 	computeVariations(plan_copy, &ds_after_previous_dives);
 
 	free(cache);
-	if (!current_dive || displayed_dive.id != current_dive->id) {
+
-		// we were planning a new dive, not re-planning an existing on
+	// Fixup planner notes.
-		record_dive(clone_dive(&displayed_dive));
+	if (current_dive && displayed_dive.id == current_dive->id) {
-	} else if (current_dive && displayed_dive.id == current_dive->id) {
-		// we are replanning a dive - make sure changes are reflected
-		// correctly in the dive structure and copy it back into the dive table
-		displayed_dive.maxdepth.mm = 0;
-		displayed_dive.dc.maxdepth.mm = 0;
-		fixup_dive(&displayed_dive);
 		// Try to identify old planner output and remove only this part
 		// Treat user provided text as plain text.
 		QTextDocument notesDocument;
@@ -1134,24 +1129,28 @@ void DivePlannerPointsModel::createPlan(bool replanCopy)
 		oldnotes.append(displayed_dive.notes);
 		displayed_dive.notes = copy_qstring(oldnotes);
 		// If we save as new create a copy of the dive here
-		if (replanCopy) {
+	}
-			struct dive *copy = alloc_dive();
+
-			copy_dive(current_dive, copy);
+	setPlanMode(NOTHING);
-			copy->id = 0;
+	planCreated(); // This signal will exit the profile from planner state. This must be *before* adding the dive,
-			copy->selected = false;
+		       // so that the Undo-commands update the display accordingly (see condition in updateDiveInfo().
-			copy->divetrip = NULL;
+
-			if (current_dive->divetrip)
+	// Now, add or modify the dive.
-				add_dive_to_trip(copy, current_dive->divetrip);
+	if (!current_dive || displayed_dive.id != current_dive->id) {
-			record_dive(copy);
+		// we were planning a new dive, not re-planning an existing one
-		}
+		displayed_dive.divetrip = nullptr; // Should not be necessary, just in case!
+		Command::addDive(&displayed_dive, autogroup, true);
+	} else if (replanCopy) {
+		// we were planning an old dive and save as a new dive
+		displayed_dive.id = dive_getUniqID(); // Things will break horribly if we create dives with the same id.
+		Command::addDive(&displayed_dive, false, false);
+	} else {
+		// we were planning an old dive and rewrite the plan
+		mark_divelist_changed(true);
 		copy_dive(&displayed_dive, current_dive);
 	}
-	mark_divelist_changed(true);
-	sort_table(&dive_table);
 
 	// Remove and clean the diveplan, so we don't delete
 	// the dive by mistake.
 	free_dps(&diveplan);
-	setPlanMode(NOTHING);
-	planCreated();
 }