Undo: implement undo of dive site editing

This one is a bit more tricky. There are two modes: set dive site
and set newly created dive site. This is realized using an OO model
with derived classed. Quite convoluted - but it seems to work.

Moreover, editing a dive site is not simply setting a value,
but the list of dives in a dive site has to be kept up to date.

Finally, we have to inform the dive site list of the changed
number of dives. Therefore add a new signal diveSiteDivesChanged.
To send only one signal per dive site, hook into the undo() and
redo() functions and call the functions of the base class there.

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

core/subsurface-qt/DiveListNotifier.h

@@ -16,7 +16,7 @@ enum class DiveField {
 	DATETIME,
 	AIR_TEMP,
 	WATER_TEMP,
-	LOCATION,
+	DIVESITE,
 	DIVEMASTER,
 	BUDDY,
 	RATING,
@@ -66,6 +66,7 @@ signals:
 	void diveSiteDeleted(dive_site *ds, int idx);
 	void diveSiteDiveCountChanged(dive_site *ds);
 	void diveSiteChanged(dive_site *ds, int field); // field according to LocationInformationModel
+	void diveSiteDivesChanged(dive_site *ds); // The dives associated with that site changed
 
 	// Signals emitted when dives are edited.
 	void divesEdited(const QVector<dive *> &dives, DiveField);

desktop-widgets/command.cpp

@@ -165,4 +165,14 @@ void editWaterTemp(const QVector<dive *> dives, int newValue, int oldValue)
 	execute(new EditWaterTemp(dives, newValue, oldValue));
 }
 
+void editDiveSite(const QVector<dive *> dives, struct dive_site *newValue, struct dive_site *oldValue)
+{
+	execute(new EditDiveSite(dives, newValue, oldValue));
+}
+
+void editDiveSiteNew(const QVector<dive *> dives, const QString &newName, struct dive_site *oldValue)
+{
+	execute(new EditDiveSiteNew(dives, newName, oldValue));
+}
+
 } // namespace Command

desktop-widgets/command.h

@@ -59,6 +59,8 @@ void editRating(const QVector<dive *> dives, int newValue, int oldValue);
 void editVisibility(const QVector<dive *> dives, int newValue, int oldValue);
 void editAirTemp(const QVector<dive *> dives, int newValue, int oldValue);
 void editWaterTemp(const QVector<dive *> dives, int newValue, int oldValue);
+void editDiveSite(const QVector<dive *> dives, struct dive_site *newValue, struct dive_site *oldValue);
+void editDiveSiteNew(const QVector<dive *> dives, const QString &newName, struct dive_site *oldValue);
 
 } // namespace Command
 

desktop-widgets/command_edit.cpp

@@ -2,6 +2,7 @@
 
 #include "command_edit.h"
 #include "core/divelist.h"
+#include "core/qthelper.h" // for copy_qstring
 
 namespace Command {
 
@@ -70,12 +71,17 @@ template
 EditBase<QString>::EditBase(const QVector<dive *> &dives, QString oldValue, QString newValue);
 template
 EditBase<int>::EditBase(const QVector<dive *> &dives, int oldValue, int newValue);
+template
+EditBase<struct dive_site *>::EditBase(const QVector<dive *> &dives, struct dive_site *oldValue, struct dive_site *newValue);
 
 // Undo and redo do the same as just the stored value is exchanged
 template<typename T>
 void EditBase<T>::redo()
 {
-	undo();
+	// Note: here, we explicitly call the undo function of EditBase<T> and don't do
+	// virtual dispatch. Thus, derived classes can call this redo function without
+	// having their own undo() function called.
+	EditBase<T>::undo();
 }
 
 // Implementation of virtual functions
@@ -208,6 +214,78 @@ DiveField EditWaterTemp::fieldId() const
 	return DiveField::WATER_TEMP;
 }
 
+// ***** DiveSite *****
+void EditDiveSite::set(struct dive *d, struct dive_site *dive_site) const
+{
+	unregister_dive_from_dive_site(d);
+	add_dive_to_dive_site(d, dive_site);
+}
+
+struct dive_site *EditDiveSite::data(struct dive *d) const
+{
+	return d->dive_site;
+}
+
+QString EditDiveSite::fieldName() const
+{
+	return tr("dive site");
+}
+
+DiveField EditDiveSite::fieldId() const
+{
+	return DiveField::DIVESITE;
+}
+
+void EditDiveSite::undo()
+{
+	// Do the normal undo thing, then send dive site changed signals
+	EditBase<dive_site *>::undo();
+	if (value)
+		emit diveListNotifier.diveSiteDivesChanged(value);
+	if (old)
+		emit diveListNotifier.diveSiteDivesChanged(old);
+}
+
+void EditDiveSite::redo()
+{
+	EditDiveSite::undo(); // Undo and redo do the same
+}
+
+static struct dive_site *createDiveSite(const QString &name, struct dive_site *old)
+{
+	struct dive_site *ds = alloc_dive_site();
+	if (old) {
+		copy_dive_site(old, ds);
+		free(ds->name); // Free name, as we will overwrite it with our own version
+	}
+	ds->name = copy_qstring(name);
+	return ds;
+}
+
+EditDiveSiteNew::EditDiveSiteNew(const QVector<dive *> &dives, const QString &newName, struct dive_site *oldValue) :
+	EditDiveSite(dives, createDiveSite(newName, oldValue), oldValue),
+	diveSiteToAdd(value),
+	diveSiteToRemove(nullptr)
+{
+}
+
+void EditDiveSiteNew::undo()
+{
+	EditDiveSite::undo();
+	int idx = unregister_dive_site(diveSiteToRemove);
+	diveSiteToAdd.reset(diveSiteToRemove);
+	emit diveListNotifier.diveSiteDeleted(diveSiteToRemove, idx); // Inform frontend of removed dive site.
+	diveSiteToRemove = nullptr;
+}
+
+void EditDiveSiteNew::redo()
+{
+	diveSiteToRemove = diveSiteToAdd.get();
+	int idx = register_dive_site(diveSiteToAdd.release()); // Return ownership to backend.
+	emit diveListNotifier.diveSiteAdded(diveSiteToRemove, idx); // Inform frontend of new dive site.
+	EditDiveSite::redo();
+}
+
 // ***** Mode *****
 // Editing the dive mode has very peculiar semantics for historic reasons:
 // Since the dive-mode depends on the dive computer, the i-th dive computer

desktop-widgets/command_edit.h

@@ -25,6 +25,7 @@ namespace Command {
 
 template <typename T>
 class EditBase : public Base {
+protected:
 	T value; // Value to be set
 	T old; // Previous value
 
@@ -101,6 +102,30 @@ public:
 	DiveField fieldId() const override;
 };
 
+class EditDiveSite : public EditBase<struct dive_site *> {
+public:
+	using EditBase<struct dive_site *>::EditBase;	// Use constructor of base class.
+	void set(struct dive *d, struct dive_site *value) const override;
+	struct dive_site *data(struct dive *d) const override;
+	QString fieldName() const override;
+	DiveField fieldId() const override;
+
+	// We specialize these so that we can send dive-site changed signals.
+	void undo() override;
+	void redo() override;
+};
+
+// Edit dive site, but add a new dive site first. Reuses the code of EditDiveSite by
+// deriving from it and hooks into undo() and redo() to add / remove the dive site.
+class EditDiveSiteNew : public EditDiveSite {
+public:
+	OwningDiveSitePtr diveSiteToAdd;
+	struct dive_site *diveSiteToRemove;
+	EditDiveSiteNew(const QVector<dive *> &dives, const QString &newName, struct dive_site *oldValue);
+	void undo() override;
+	void redo() override;
+};
+
 class EditMode : public EditBase<int> {
 	int index;
 public:

desktop-widgets/mainwindow.cpp

@@ -213,7 +213,6 @@ MainWindow::MainWindow() : QMainWindow(),
 	}
 	ui.menuFile->insertSeparator(ui.actionQuit);
 	connect(mainTab, SIGNAL(addDiveFinished()), graphics, SLOT(setProfileState()));
-	connect(mainTab, SIGNAL(dateTimeChanged()), graphics, SLOT(dateTimeChanged()));
 	connect(DivePlannerPointsModel::instance(), SIGNAL(planCreated()), this, SLOT(planCreated()));
 	connect(DivePlannerPointsModel::instance(), SIGNAL(planCanceled()), this, SLOT(planCanceled()));
 	connect(DivePlannerPointsModel::instance(), SIGNAL(variationsComputed(QString)), this, SLOT(updateVariations(QString)));
@@ -387,7 +386,8 @@ void MainWindow::editDiveSite(dive_site *ds)
 
 void MainWindow::startDiveSiteEdit()
 {
-	editDiveSite(get_dive_site_for_dive(&displayed_dive));
+	if (current_dive)
+		editDiveSite(get_dive_site_for_dive(current_dive));
 }
 
 void MainWindow::enableDisableCloudActions()

desktop-widgets/tab-widgets/maintab.cpp

@@ -368,6 +368,10 @@ void MainTab::divesEdited(const QVector<dive *> &, DiveField field)
 		MainWindow::instance()->graphics->dateTimeChanged();
 		DivePlannerPointsModel::instance()->getDiveplan().when = current_dive->when;
 		break;
+	case DiveField::DIVESITE:
+		updateDiveSite(current_dive);
+		emit diveSiteChanged();
+		break;
 	default:
 		break;
 	}
@@ -438,6 +442,23 @@ void MainTab::updateDateTime(struct dive *d)
 	ui.timeEdit->setTime(localTime.time());
 }
 
+void MainTab::updateDiveSite(struct dive *d)
+{
+	struct dive_site *ds = d->dive_site;
+	if (ds) {
+		ui.location->setCurrentDiveSite(ds);
+		ui.locationTags->setText(constructLocationTags(&ds->taxonomy, true));
+
+		if (ui.locationTags->text().isEmpty() && has_location(&ds->location))
+			ui.locationTags->setText(printGPSCoords(&ds->location));
+		ui.editDiveSiteButton->setEnabled(true);
+	} else {
+		ui.location->clear();
+		ui.locationTags->clear();
+		ui.editDiveSiteButton->setEnabled(false);
+	}
+}
+
 void MainTab::updateDiveInfo(bool clear)
 {
 	ui.location->refreshDiveSiteCache();
@@ -466,19 +487,7 @@ void MainTab::updateDiveInfo(bool clear)
 	updateMode(&displayed_dive);
 
 	if (!clear) {
-		struct dive_site *ds = NULL;
-		ds = displayed_dive.dive_site;
-		if (ds) {
-			ui.location->setCurrentDiveSite(ds);
-			ui.locationTags->setText(constructLocationTags(&ds->taxonomy, true));
-
-			if (ui.locationTags->text().isEmpty() && has_location(&ds->location))
-				ui.locationTags->setText(printGPSCoords(&ds->location));
-		} else {
-			ui.location->clear();
-			ui.locationTags->clear();
-		}
-
+		updateDiveSite(&displayed_dive);
 		updateDateTime(&displayed_dive);
 		if (MainWindow::instance() && MainWindow::instance()->diveList->selectedTrips().count() == 1) {
 			// Remember the tab selected for last dive
@@ -692,49 +701,6 @@ void MainTab::refreshDisplayedDiveSite()
 		ui.location->setCurrentDiveSite(ds);
 }
 
-// when this is called we already have updated the current_dive and know that it exists
-// there is no point in calling this function if there is no current dive
-struct dive_site *MainTab::updateDiveSite(struct dive_site *pickedDs, dive *d)
-{
-	if (!d)
-		return 0;
-
-	if (ui.location->text().isEmpty())
-		return 0;
-
-	if (!pickedDs)
-		return 0;
-
-	struct dive_site *origDs = d->dive_site;
-	bool createdNewDive = false;
-
-	if (pickedDs == origDs)
-		return origDs;
-
-	if (pickedDs == RECENTLY_ADDED_DIVESITE) {
-		QString name = ui.location->text();
-		pickedDs = create_dive_site(qPrintable(name), &dive_site_table);
-		createdNewDive = true;
-	}
-
-	if (origDs) {
-		if(createdNewDive) {
-			copy_dive_site(origDs, pickedDs);
-			free(pickedDs->name);
-			pickedDs->name = copy_qstring(ui.location->text());
-			qDebug() << "Creating and copying dive site";
-		} else if (!has_location(&pickedDs->location)) {
-			pickedDs->location = origDs->location;
-			qDebug() << "Copying GPS information";
-		}
-	}
-
-	unregister_dive_from_dive_site(d);
-	add_dive_to_dive_site(d, pickedDs);
-	qDebug() << "Setting the dive site id on the dive:" << pickedDs->uuid;
-	return pickedDs;
-}
-
 // Get the list of selected dives, but put the current dive at the last position of the vector
 static QVector<dive *> getSelectedDivesCurrentLast()
 {
@@ -816,11 +782,9 @@ void MainTab::acceptChanges()
 			copy_samples(&displayed_dive.dc, &current_dive->dc);
 			addedId = displayed_dive.id;
 		}
-		struct dive *cd = current_dive;
 		// now check if something has changed and if yes, edit the selected dives that
 		// were identical with the master dive shown (and mark the divelist as changed)
-		if (displayed_dive.dive_site != cd->dive_site)
-			MODIFY_DIVES(selectedDives, EDIT_VALUE(dive_site));
+		struct dive *cd = current_dive;
 
 		// three text fields are somewhat special and are represented as tags
 		// in the UI - they need somewhat smarter handling
@@ -887,18 +851,6 @@ void MainTab::acceptChanges()
 			}
 		}
 
-		// update the dive site for the selected dives that had the same dive site as the current dive
-		struct dive_site *newDs = nullptr;
-		MODIFY_DIVES(selectedDives,
-			if (mydive->dive_site == current_dive->dive_site)
-				newDs = updateDiveSite(!newDs ? ui.location->currDiveSite() : newDs, mydive);
-		);
-		// the code above can change the correct uuid for the displayed dive site - and the
-		// code below triggers an update of the display without re-initializing displayed_dive
-		// so let's make sure here that our data is consistent now that we have handled the
-		// dive sites
-		displayed_dive.dive_site = current_dive->dive_site;
-
 		// each dive that was selected might have had the temperatures in its active divecomputer changed
 		// so re-populate the temperatures - easiest way to do this is by calling fixup_dive
 		for_each_dive (i, d) {
@@ -1286,39 +1238,16 @@ void MainTab::on_tagWidget_textChanged()
 	markChangedWidget(ui.tagWidget);
 }
 
-void MainTab::on_location_textChanged()
-{
-	if (editMode == IGNORE)
-		return;
-
-	// we don't want to act on the edit until editing is finished,
-	// but we want to mark the field so it's obvious it is being edited
-	QString currentLocation;
-	struct dive_site *ds = displayed_dive.dive_site;
-	if (ds)
-		currentLocation = ds->name;
-	if (ui.location->text() != currentLocation)
-		markChangedWidget(ui.location);
-}
-
 void MainTab::on_location_diveSiteSelected()
 {
-	if (editMode == IGNORE || acceptingEdit == true)
+	if (editMode == IGNORE || acceptingEdit == true || !current_dive)
 		return;
 
-	if (ui.location->text().isEmpty()) {
-		displayed_dive.dive_site = nullptr;
-		markChangedWidget(ui.location);
-		emit diveSiteChanged();
-		return;
-	} else {
-		if (ui.location->currDiveSite() != displayed_dive.dive_site) {
-			markChangedWidget(ui.location);
-		} else {
-			QPalette p;
-			ui.location->setPalette(p);
-		}
-	}
+	struct dive_site *newDs = ui.location->currDiveSite();
+	if (newDs == RECENTLY_ADDED_DIVESITE)
+		Command::editDiveSiteNew(getSelectedDivesCurrentLast(), ui.location->text(), current_dive->dive_site);
+	else
+		Command::editDiveSite(getSelectedDivesCurrentLast(), newDs, current_dive->dive_site);
 }
 
 void MainTab::on_diveTripLocation_textEdited(const QString& text)

desktop-widgets/tab-widgets/maintab.h

@@ -69,11 +69,11 @@ slots:
 	void updateNotes(const struct dive *d);
 	void updateMode(struct dive *d);
 	void updateDateTime(struct dive *d);
+	void updateDiveSite(struct dive *d);
 	void updateDepthDuration();
 	void acceptChanges();
 	void rejectChanges();
 	void on_location_diveSiteSelected();
-	void on_location_textChanged();
 	void on_divemaster_textChanged();
 	void on_buddy_textChanged();
 	void on_suit_editingFinished();
@@ -126,7 +126,6 @@ private:
 	dive_trip_t *currentTrip;
 	dive_trip_t displayedTrip;
 	bool acceptingEdit;
-	struct dive_site *updateDiveSite(struct dive_site *pickedDs, dive *d);
 	QList<TabBase*> extraWidgets;
 };
 

qt-models/divelocationmodel.cpp

@@ -27,6 +27,7 @@ LocationInformationModel::LocationInformationModel(QObject *obj) : QAbstractTabl
 	connect(&diveListNotifier, &DiveListNotifier::diveSiteAdded, this, &LocationInformationModel::diveSiteAdded);
 	connect(&diveListNotifier, &DiveListNotifier::diveSiteDeleted, this, &LocationInformationModel::diveSiteDeleted);
 	connect(&diveListNotifier, &DiveListNotifier::diveSiteChanged, this, &LocationInformationModel::diveSiteChanged);
+	connect(&diveListNotifier, &DiveListNotifier::diveSiteDivesChanged, this, &LocationInformationModel::diveSiteDivesChanged);
 }
 
 int LocationInformationModel::columnCount(const QModelIndex &) const
@@ -167,6 +168,14 @@ void LocationInformationModel::diveSiteChanged(struct dive_site *ds, int field)
 	dataChanged(createIndex(idx, field), createIndex(idx, field));
 }
 
+void LocationInformationModel::diveSiteDivesChanged(struct dive_site *ds)
+{
+	int idx = get_divesite_idx(ds, &dive_site_table);
+	if (idx < 0)
+		return;
+	dataChanged(createIndex(idx, NUM_DIVES), createIndex(idx, NUM_DIVES));
+}
+
 bool DiveSiteSortedModel::filterAcceptsRow(int sourceRow, const QModelIndex &source_parent) const
 {
 	// TODO: filtering

qt-models/divelocationmodel.h

@@ -35,6 +35,7 @@ public slots:
 	void diveSiteAdded(struct dive_site *ds, int idx);
 	void diveSiteDeleted(struct dive_site *ds, int idx);
 	void diveSiteChanged(struct dive_site *ds, int field);
+	void diveSiteDivesChanged(struct dive_site *ds);
 };
 
 class DiveSiteSortedModel : public QSortFilterProxyModel {