undo: pass dive as unique_ptr to addDive()

Before, a non-owning pointer was passed and the dive moved
away from the dive. Instead, let the caller decide if they
still want to keep a copy of the dive, or give up ownership:

In MainWindow and QMLManager new dives are generated, so
one might just as well give up ownership. In contrast,
the planner works on a copy (originally the infamous
"displayed_dive") and now moves the data manually.

This commit also removes duplicate code, by moving the
"create default dive" code from MainWindow and QMLManager
to struct dive.

Finally, determination of the "time zone offset" is not done
in POSIX, since we want to avoid calls form the core into
Qt.

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

commands/command.cpp

@@ -13,9 +13,9 @@
 namespace Command {
 
 // Dive-list related commands
-void addDive(dive *d, bool autogroup, bool newNumber)
+void addDive(std::unique_ptr<dive> d, bool autogroup, bool newNumber)
 {
-	execute(new AddDive(d, autogroup, newNumber));
+	execute(new AddDive(std::move(d), autogroup, newNumber));
 }
 
 void importDives(struct divelog *log, int flags, const QString &source)

commands/command.h

@@ -40,7 +40,7 @@ bool placingCommand();			// Currently executing a new command -> might not have
 // 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 addDive(dive *d, bool autogroup, bool newNumber);
+void addDive(std::unique_ptr<dive> d, bool autogroup, bool newNumber);
 void importDives(struct divelog *log, int flags, const QString &source); // The tables are consumed!
 void deleteDive(const QVector<struct dive*> &divesToDelete);
 void shiftTime(const std::vector<dive *> &changedDives, int amount);

commands/command_divelist.cpp

@@ -386,42 +386,39 @@ void DiveListBase::redo()
 	finishWork();
 }
 
-AddDive::AddDive(dive *d, bool autogroup, bool newNumber)
+AddDive::AddDive(std::unique_ptr<dive> d, bool autogroup, bool newNumber)
 {
 	setText(Command::Base::tr("add dive"));
-	// By convention, d is a pointer to "displayed dive" or a temporary variable and can be overwritten.
 	d->maxdepth.mm = 0;
 	d->dcs[0].maxdepth.mm = 0;
-	fixup_dive(d);
+	fixup_dive(d.get());
 
 	// this only matters if undoit were called before redoit
 	currentDive = nullptr;
 
-	// Get an owning pointer to a moved dive.
-	std::unique_ptr<dive> divePtr = move_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.
+	d->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.
 	std::unique_ptr<dive_trip> allocTrip;
-	dive_trip *trip = divePtr->divetrip;
-	dive_site *site = divePtr->dive_site;
+	dive_trip *trip = d->divetrip;
+	dive_site *site = d->dive_site;
 	// We have to delete the pointers to trip and site, because this would prevent the core from adding to the
 	// trip or site and we would get the count-of-dives in the trip or site wrong. Yes, that's all horribly subtle!
-	divePtr->divetrip = nullptr;
-	divePtr->dive_site = nullptr;
+	d->divetrip = nullptr;
+	d->dive_site = nullptr;
 	if (!trip && autogroup) {
-		auto [t, allocated] = get_trip_for_new_dive(divelog, divePtr.get());
+		auto [t, allocated] = get_trip_for_new_dive(divelog, d.get());
 		trip = t;
 		allocTrip = std::move(allocated);
 	}
 
-	int idx = divelog.dives.get_insertion_index(divePtr.get());
+	int idx = divelog.dives.get_insertion_index(d.get());
 	if (newNumber)
-		divePtr->number = divelog.dives.get_dive_nr_at_idx(idx);
+		d->number = divelog.dives.get_dive_nr_at_idx(idx);
 
-	divesToAdd.dives.push_back({ std::move(divePtr), trip, site });
+	divesToAdd.dives.push_back({ std::move(d), trip, site });
 	if (allocTrip)
 		divesToAdd.trips.push_back(std::move(allocTrip));
 }

commands/command_divelist.h

@@ -79,7 +79,7 @@ private:
 
 class AddDive : public DiveListBase {
 public:
-	AddDive(dive *dive, bool autogroup, bool newNumber);
+	AddDive(std::unique_ptr<struct dive> dive, bool autogroup, bool newNumber);
 private:
 	void undoit() override;
 	void redoit() override;

core/dive.cpp

@@ -28,6 +28,8 @@
 #include "trip.h"
 #include "fulltext.h"
 
+#include <time.h>
+
 // For user visible text but still not translated
 const char *divemode_text_ui[] = {
 	QT_TRANSLATE_NOOP("gettextFromC", "Open circuit"),
@@ -39,8 +41,8 @@ const char *divemode_text_ui[] = {
 // For writing/reading files.
 const char *divemode_text[] = {"OC", "CCR", "PSCR", "Freedive"};
 
-// It's the "manually added" divecomputer.
 // Even for dives without divecomputer, we allocate a divecomputer structure.
+// It's the "manually added" divecomputer.
 dive::dive() : dcs(1)
 {
 	id = dive_getUniqID();
@@ -51,6 +53,22 @@ dive::dive(dive &&) = default;
 dive &dive::operator=(const dive &) = default;
 dive::~dive() = default;
 
+// create a dive an hour from now with a default depth (15m/45ft) and duration (40 minutes)
+// as a starting point for the user to edit
+std::unique_ptr<dive> dive::default_dive()
+{
+	auto d = std::make_unique<dive>();
+	d->when = time(nullptr) + gettimezoneoffset() + 3600;
+	d->dcs[0].duration.seconds = 40 * 60;
+	d->dcs[0].maxdepth.mm = M_OR_FT(15, 45);
+	d->dcs[0].meandepth.mm = M_OR_FT(13, 39); // this creates a resonable looking safety stop
+	make_manually_added_dive_dc(&d->dcs[0]);
+	fake_dc(&d->dcs[0]);
+	add_default_cylinder(d.get());
+	fixup_dive(d.get());
+	return d;
+}
+
 /*
  * The legacy format for sample pressures has a single pressure
  * for each sample that can have any sensor, plus a possible
@@ -178,16 +196,6 @@ void copy_dive(const struct dive *s, struct dive *d)
 	invalidate_dive_cache(d);
 }
 
-/* make a clone of the source dive and clean out the source dive;
- * this allows us to create a dive on the stack and then
- * add it to the divelist. */
-struct std::unique_ptr<dive> move_dive(struct dive *s)
-{
-	auto d = std::make_unique<dive>();
-	std::swap(*s, *d);
-	return d;
-}
-
 #define CONDITIONAL_COPY_STRING(_component) \
 	if (what._component)                \
 		d->_component = s->_component

core/dive.h

@@ -78,6 +78,7 @@ struct dive {
 	dive(const dive &);
 	dive(dive &&);
 	dive &operator=(const dive &);
+	static std::unique_ptr<dive> default_dive();
 
 	timestamp_t endtime() const;		/* maximum over divecomputers (with samples) */
 	duration_t totaltime() const;		/* maximum over divecomputers (with samples) */
@@ -172,7 +173,6 @@ extern bool subsurface_user_is_root();
 extern void clear_dive(struct dive *dive);
 extern void copy_dive(const struct dive *s, struct dive *d);
 extern void selective_copy_dive(const struct dive *s, struct dive *d, struct dive_components what, bool clear);
-extern struct std::unique_ptr<dive> move_dive(struct dive *s);
 
 extern int legacy_format_o2pressures(const struct dive *dive, const struct divecomputer *dc);
 

core/qthelper.cpp

@@ -692,15 +692,6 @@ QString getPrintingTemplatePathBundle()
 	return path;
 }
 
-int gettimezoneoffset()
-{
-	QDateTime dt1, dt2;
-	dt1 = QDateTime::currentDateTime();
-	dt2 = dt1.toUTC();
-	dt1.setTimeSpec(Qt::UTC);
-	return dt2.secsTo(dt1);
-}
-
 QDateTime timestampToDateTime(timestamp_t when)
 {
 	// Subsurface always uses "local time" as in "whatever was the local time at the location"

core/qthelper.h

@@ -64,7 +64,6 @@ QString get_water_type_string(int salinity);
 QString getSubsurfaceDataPath(QString folderToFind);
 QString getPrintingTemplatePathUser();
 QString getPrintingTemplatePathBundle();
-int gettimezoneoffset();
 QDateTime timestampToDateTime(timestamp_t when);
 timestamp_t dateTimeToTimestamp(const QDateTime &t);
 int parseDurationToSeconds(const QString &text);

core/subsurface-time.h

@@ -10,6 +10,7 @@ extern timestamp_t utc_mktime(const struct tm *tm);
 extern void utc_mkdate(timestamp_t, struct tm *tm);
 extern int utc_year(timestamp_t timestamp);
 extern int utc_weekday(timestamp_t timestamp);
+extern int gettimezoneoffset();
 
 /* parse and format date times of the form YYYY-MM-DD hh:mm:ss */
 extern timestamp_t parse_datetime(const char *s); /* returns 0 on error */

core/time.cpp

@@ -225,3 +225,18 @@ const char *monthname(int mon)
 	};
 	return translate("gettextFromC", month_array[mon]);
 }
+
+int gettimezoneoffset()
+{
+	time_t now = time(nullptr);
+#ifdef WIN32
+	// Somewhat surprisingly, Windows doesn't have localtime_r (I thought this was POSIX?).
+	// Let's use the global timezone variable.
+	// Ultimately, use the portable C++20 API.
+	return static_cast<int>(-timezone);
+#else
+	struct tm local;
+	localtime_r(&now, &local);
+	return local.tm_gmtoff;
+#endif
+}

desktop-widgets/mainwindow.cpp

@@ -698,20 +698,8 @@ void MainWindow::on_actionAddDive_triggered()
 	if (!plannerStateClean())
 		return;
 
-	// create a dive an hour from now with a default depth (15m/45ft) and duration (40 minutes)
-	// as a starting point for the user to edit
-	struct dive d;
-	d.id = dive_getUniqID();
-	d.when = QDateTime::currentMSecsSinceEpoch() / 1000L + gettimezoneoffset() + 3600;
-	d.dcs[0].duration.seconds = 40 * 60;
-	d.dcs[0].maxdepth.mm = M_OR_FT(15, 45);
-	d.dcs[0].meandepth.mm = M_OR_FT(13, 39); // this creates a resonable looking safety stop
-	make_manually_added_dive_dc(&d.dcs[0]);
-	fake_dc(&d.dcs[0]);
-	add_default_cylinder(&d);
-	fixup_dive(&d);
-
-	Command::addDive(&d, divelog.autogroup, true);
+	auto d = dive::default_dive();
+	Command::addDive(std::move(d), divelog.autogroup, true);
 }
 
 void MainWindow::on_actionRenumber_triggered()

mobile-widgets/qmlmanager.cpp

@@ -1715,23 +1715,11 @@ void QMLManager::cancelDownloadDC()
 
 int QMLManager::addDive()
 {
-	// TODO: Duplicate code with desktop-widgets/mainwindow.cpp
-	// create a dive an hour from now with a default depth (15m/45ft) and duration (40 minutes)
-	// as a starting point for the user to edit
-	struct dive d;
-	int diveId = d.id = dive_getUniqID();
-	d.when = QDateTime::currentMSecsSinceEpoch() / 1000L + gettimezoneoffset() + 3600;
-	d.dcs[0].duration.seconds = 40 * 60;
-	d.dcs[0].maxdepth.mm = M_OR_FT(15, 45);
-	d.dcs[0].meandepth.mm = M_OR_FT(13, 39); // this creates a resonable looking safety stop
-	make_manually_added_dive_dc(&d.dcs[0]);
-	fake_dc(&d.dcs[0]);
-	fixup_dive(&d);
-
-	// addDive takes over the dive and clears out the structure passed in
 	// we do NOT save the modified data at this stage because of the UI flow here... this will
 	// be saved once the user finishes editing the newly added dive
-	Command::addDive(&d, divelog.autogroup, true);
+	auto d = dive::default_dive();
+	int diveId = d->id;
+	Command::addDive(std::move(d), divelog.autogroup, true);
 
 	if (verbose)
 		appendTextToLog(QString("Adding new dive with id '%1'").arg(diveId));

qt-models/diveplannermodel.cpp

@@ -13,6 +13,7 @@
 #include "core/range.h"
 #include "core/sample.h"
 #include "core/selection.h"
+#include "core/subsurface-time.h"
 #include "core/settings/qPrefDivePlanner.h"
 #include "core/settings/qPrefUnit.h"
 #if !defined(SUBSURFACE_TESTING)
@@ -1270,6 +1271,16 @@ void DivePlannerPointsModel::computeVariationsDone(QString variations)
 	emit calculatedPlanNotes(notes);
 }
 
+static void addDive(dive *d, bool autogroup, bool newNumber)
+{
+	// Create a new dive and clear out the old one.
+	auto new_d = std::make_unique<dive>();
+	std::swap(*d, *new_d);
+#if !defined(SUBSURFACE_TESTING)
+	Command::addDive(std::move(new_d), autogroup, newNumber);
+#endif // !SUBSURFACE_TESTING
+}
+
 void DivePlannerPointsModel::createPlan(bool saveAsNew)
 {
 	// Ok, so, here the diveplan creates a dive
@@ -1327,17 +1338,13 @@ void DivePlannerPointsModel::createPlan(bool saveAsNew)
 	if (!current_dive || d->id != current_dive->id) {
 		// we were planning a new dive, not re-planning an existing one
 		d->divetrip = nullptr; // Should not be necessary, just in case!
-#if !defined(SUBSURFACE_TESTING)
-		Command::addDive(d, divelog.autogroup, true);
-#endif // !SUBSURFACE_TESTING
+		addDive(d, divelog.autogroup, true);
 	} else {
 		copy_events_until(current_dive, d, dcNr, preserved_until.seconds);
 		if (saveAsNew) {
 			// we were planning an old dive and save as a new dive
 			d->id = dive_getUniqID(); // Things will break horribly if we create dives with the same id.
-#if !defined(SUBSURFACE_TESTING)
-			Command::addDive(d, false, false);
-#endif // !SUBSURFACE_TESTING
+			addDive(d, false, false);
 		} else {
 			// we were planning an old dive and rewrite the plan
 #if !defined(SUBSURFACE_TESTING)