undo: implement undo of setting a picture time by drag&drop

Even though the functionality is seemingly trivial, this is a bit
invasive, as the code has to be split into two distinct parts:
1) Post undo command
2) React to changes to the divelist

Don't compile that code on mobile.

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

commands/CMakeLists.txt

@@ -16,6 +16,8 @@ set(SUBSURFACE_GENERIC_COMMANDS_SRCS
 	command_edit_trip.h
 	command_event.cpp
 	command_event.h
+	command_pictures.cpp
+	command_pictures.h
 )
 add_library(subsurface_commands STATIC ${SUBSURFACE_GENERIC_COMMANDS_SRCS})
 target_link_libraries(subsurface_commands ${QT_LIBRARIES})

commands/command.cpp

@@ -6,6 +6,7 @@
 #include "command_edit.h"
 #include "command_edit_trip.h"
 #include "command_event.h"
+#include "command_pictures.h"
 
 namespace Command {
 
@@ -359,4 +360,11 @@ void addGasSwitch(struct dive *d, int dcNr, int seconds, int tank)
 	execute(new AddGasSwitch(d, dcNr, seconds, tank));
 }
 
+// Picture (media) commands
+
+void setPictureOffset(dive *d, const QString &filename, offset_t offset)
+{
+	execute(new SetPictureOffset(d, filename, offset));
+}
+
 } // namespace Command

commands/command.h

@@ -118,6 +118,10 @@ void renameEvent(struct dive *d, int dcNr, struct event *ev, const char *name);
 void removeEvent(struct dive *d, int dcNr, struct event *ev);
 void addGasSwitch(struct dive *d, int dcNr, int seconds, int tank);
 
+// 7) Picture (media) commands
+
+void setPictureOffset(dive *d, const QString &filename, offset_t offset);
+
 } // namespace Command
 
 #endif // COMMAND_H

commands/command_pictures.cpp

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "command_pictures.h"
+#include "core/subsurface-qt/divelistnotifier.h"
+
+namespace Command {
+
+static picture *dive_get_picture(const dive *d, const QString &fn)
+{
+	int idx = get_picture_idx(&d->pictures, qPrintable(fn));
+	return idx < 0 ? nullptr : &d->pictures.pictures[idx];
+}
+
+SetPictureOffset::SetPictureOffset(dive *dIn, const QString &filenameIn, offset_t offsetIn) :
+	d(dIn), filename(filenameIn), offset(offsetIn)
+{
+	if (!dive_get_picture(d, filename))
+		d = nullptr;
+	setText(Command::Base::tr("Change media time"));
+}
+
+void SetPictureOffset::redo()
+{
+	picture *pic = dive_get_picture(d, filename);
+	if (!pic) {
+		fprintf(stderr, "SetPictureOffset::redo(): picture disappeared!");
+		return;
+	}
+	std::swap(pic->offset, offset);
+	offset_t newOffset = pic->offset;
+
+	// Instead of trying to be smart, let's simply resort the picture table.
+	// If someone complains about speed, do our usual "smart" thing.
+	sort_picture_table(&d->pictures);
+	emit diveListNotifier.pictureOffsetChanged(d, filename, newOffset);
+	invalidate_dive_cache(d);
+}
+
+// Undo and redo do the same thing
+void SetPictureOffset::undo()
+{
+	redo();
+}
+
+bool SetPictureOffset::workToBeDone()
+{
+	return !!d;
+}
+
+} // namespace Command

commands/command_pictures.h

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+// Note: this header file is used by the undo-machinery and should not be included elsewhere.
+
+#ifndef COMMAND_PICTURES_H
+#define COMMAND_PICTURES_H
+
+#include "command_base.h"
+
+// We put everything in a namespace, so that we can shorten names without polluting the global namespace
+namespace Command {
+
+class SetPictureOffset final : public Base {
+public:
+	SetPictureOffset(dive *d, const QString &filename, offset_t offset); // Pictures are identified by the unique (dive,filename) pair
+private:
+	dive *d; // null means no work to be done
+	QString filename;
+	offset_t offset;
+
+	void undo() override;
+	void redo() override;
+	bool workToBeDone() override;
+};
+
+} // namespace Command
+#endif

core/subsurface-qt/divelistnotifier.h

@@ -116,6 +116,9 @@ signals:
 	// Event-related signals. Currently, we're very blunt: only one signal for any changes to the events.
 	void eventsChanged(dive *d);
 
+	// Picture (media) related signals
+	void pictureOffsetChanged(dive *d, QString filename, offset_t offset);
+
 	// This signal is emited every time a command is executed.
 	// This is used to hide an old multi-dives-edited warning message.
 	// This is necessary, so that the user can't click on the "undo" button and undo

packaging/ios/Subsurface-mobile.pro

@@ -21,6 +21,7 @@ SOURCES += ../../subsurface-mobile-main.cpp \
 	../../commands/command_divesite.cpp \
 	../../commands/command_edit.cpp \
 	../../commands/command_edit_trip.cpp \
+	../../commands/command_pictures.cpp \
 	../../core/cloudstorage.cpp \
 	../../core/configuredivecomputerthreads.cpp \
 	../../core/devicedetails.cpp \
@@ -186,6 +187,7 @@ HEADERS += \
 	../../commands/command_divesite.h \
 	../../commands/command_edit.h \
 	../../commands/command_edit_trip.h \
+	../../commands/command_pictures.h \
 	../../core/libdivecomputer.h \
 	../../core/cloudstorage.h \
 	../../core/configuredivecomputerthreads.h \

profile-widget/profilewidget2.cpp

@@ -19,7 +19,7 @@
 #include "core/pref.h"
 #include "qt-models/diveplannermodel.h"
 #include "qt-models/models.h"
-#include "qt-models/divepicturemodel.h"
+#include "qt-models/divepicturemodel.h" // TODO: remove once divepictures have been undo-ified
 #include "core/divelist.h"
 #include "core/errorhelper.h"
 #ifndef SUBSURFACE_MOBILE
@@ -174,6 +174,7 @@ ProfileWidget2::ProfileWidget2(QWidget *parent) : QGraphicsView(parent),
 	connect(DivePictureModel::instance(), &DivePictureModel::modelReset, this, &ProfileWidget2::plotPictures);
 	connect(&diveListNotifier, &DiveListNotifier::cylinderEdited, this, &ProfileWidget2::profileChanged);
 	connect(&diveListNotifier, &DiveListNotifier::eventsChanged, this, &ProfileWidget2::profileChanged);
+	connect(&diveListNotifier, &DiveListNotifier::pictureOffsetChanged, this, &ProfileWidget2::pictureOffsetChanged);
 #endif // SUBSURFACE_MOBILE
 
 #if !defined(QT_NO_DEBUG) && defined(SHOW_PLOT_INFO_TABLE)
@@ -2175,86 +2176,16 @@ void ProfileWidget2::profileChanged(dive *d)
 
 void ProfileWidget2::dropEvent(QDropEvent *event)
 {
+#ifndef SUBSURFACE_MOBILE
 	if (event->mimeData()->hasFormat("application/x-subsurfaceimagedrop")) {
 		QByteArray itemData = event->mimeData()->data("application/x-subsurfaceimagedrop");
 		QDataStream dataStream(&itemData, QIODevice::ReadOnly);
 
 		QString filename;
-		int diveId;
-		dataStream >> filename >> diveId;
-
-		// If the id of the drag & dropped picture belongs to a different dive, then
-		// the offset we determine makes no sense what so ever. Simply ignore such an event.
-		// In the future, we might think about duplicating the picture or moving the picture
-		// from one dive to the other.
-		if (!current_dive || displayed_dive.id != diveId) {
-			event->ignore();
-			return;
-		}
-
-#ifndef SUBSURFACE_MOBILE
-		// Calculate time in dive where picture was dropped and whether the new position is during the dive.
+		dataStream >> filename;
 		QPointF mappedPos = mapToScene(event->pos());
 		offset_t offset { (int32_t)lrint(timeAxis->valueAt(mappedPos)) };
-		bool duringDive = current_dive && offset.seconds > 0 && offset.seconds < current_dive->duration.seconds;
-
-		// A picture was drag&dropped onto the profile: We have four cases to consider:
-		//	1a) The image was already shown on the profile and is moved to a different position on the profile.
-		//	    Calculate the new position and move the picture.
-		//	1b) The image was on the profile and is moved outside of the dive time.
-		//	    Remove the picture.
-		//	2a) The image was not on the profile and is moved into the dive time.
-		//	    Add the picture to the profile.
-		//	2b) The image was not on the profile and is moved outside of the dive time.
-		//	    Do nothing.
-		auto oldPos = std::find_if(pictures.begin(), pictures.end(), [filename](const PictureEntry &e)
-					   { return e.filename == filename; });
-		if (oldPos != pictures.end()) {
-			// Cases 1a) and 1b): picture is on profile
-			if (duringDive) {
-				// Case 1a): move to new position
-				// First, find new position. Note that we also have to compare filenames,
-				// because it is quite easy to generate equal offsets.
-				auto newPos = std::find_if(pictures.begin(), pictures.end(), [offset, &filename](const PictureEntry &e)
-							   { return std::tie(e.offset.seconds, e.filename) > std::tie(offset.seconds, filename); });
-				// Set new offset
-				oldPos->offset.seconds = offset.seconds;
-				updateThumbnailXPos(*oldPos);
-
-				// Move image from old to new position
-				int oldIndex = oldPos - pictures.begin();
-				int newIndex = newPos - pictures.begin();
-				moveInVector(pictures, oldIndex, oldIndex + 1, newIndex);
-			} else {
-				// Case 1b): remove picture
-				pictures.erase(oldPos);
-			}
-
-			// In both cases the picture list changed, therefore we must recalculate the y-coordinatesA.
-			calculatePictureYPositions();
-		} else {
-			// Cases 2a) and 2b): picture not on profile. We only have to take action for
-			// the first case: picture is moved into dive-time.
-			if (duringDive) {
-				// Case 2a): add the picture at the appropriate position.
-				// The case move from outside-to-outside of the profile plot was handled by
-				// the "&& duringDive" condition in the if above.
-				// As for case 1a), we have to also consider filenames in the case of equal offsets.
-				auto newPos = std::find_if(pictures.begin(), pictures.end(), [offset, &filename](const PictureEntry &e)
-							   { return std::tie(e.offset.seconds, e.filename) > std::tie(offset.seconds, filename); });
-				// emplace() constructs the element at the given position in the vector.
-				// The parameters are passed directly to the contructor.
-				// The call returns an iterator to the new element (which might differ from
-				// the old iterator, since the buffer might have been reallocated).
-				newPos = pictures.emplace(newPos, offset, filename, scene(), false);
-				updateThumbnailXPos(*newPos);
-				calculatePictureYPositions();
-			}
-		}
-
-		// Only signal the drag&drop action if the picture actually belongs to the dive.
-		DivePictureModel::instance()->updateDivePictureOffset(displayed_dive.id, filename, offset.seconds);
-#endif
+		Command::setPictureOffset(current_dive, filename, offset);
 
 		if (event->source() == this) {
 			event->setDropAction(Qt::MoveAction);
@@ -2265,8 +2196,74 @@ void ProfileWidget2::dropEvent(QDropEvent *event)
 	} else {
 		event->ignore();
 	}
+#endif
 }
 
+#ifndef SUBSURFACE_MOBILE
+void ProfileWidget2::pictureOffsetChanged(dive *d, QString filename, offset_t offset)
+{
+	if (d->id != displayed_dive.id)
+		return; // Picture of a different dive than the one shown changed.
+
+	// Calculate time in dive where picture was dropped and whether the new position is during the dive.
+	bool duringDive = current_dive && offset.seconds > 0 && offset.seconds < current_dive->duration.seconds;
+
+	// A picture was drag&dropped onto the profile: We have four cases to consider:
+	//	1a) The image was already shown on the profile and is moved to a different position on the profile.
+	//	    Calculate the new position and move the picture.
+	//	1b) The image was on the profile and is moved outside of the dive time.
+	//	    Remove the picture.
+	//	2a) The image was not on the profile and is moved into the dive time.
+	//	    Add the picture to the profile.
+	//	2b) The image was not on the profile and is moved outside of the dive time.
+	//	    Do nothing.
+	auto oldPos = std::find_if(pictures.begin(), pictures.end(), [filename](const PictureEntry &e)
+				   { return e.filename == filename; });
+	if (oldPos != pictures.end()) {
+		// Cases 1a) and 1b): picture is on profile
+		if (duringDive) {
+			// Case 1a): move to new position
+			// First, find new position. Note that we also have to compare filenames,
+			// because it is quite easy to generate equal offsets.
+			auto newPos = std::find_if(pictures.begin(), pictures.end(), [offset, &filename](const PictureEntry &e)
+						   { return std::tie(e.offset.seconds, e.filename) > std::tie(offset.seconds, filename); });
+			// Set new offset
+			oldPos->offset.seconds = offset.seconds;
+			updateThumbnailXPos(*oldPos);
+
+			// Move image from old to new position
+			int oldIndex = oldPos - pictures.begin();
+			int newIndex = newPos - pictures.begin();
+			moveInVector(pictures, oldIndex, oldIndex + 1, newIndex);
+		} else {
+			// Case 1b): remove picture
+			pictures.erase(oldPos);
+		}
+
+		// In both cases the picture list changed, therefore we must recalculate the y-coordinatesA.
+		calculatePictureYPositions();
+	} else {
+		// Cases 2a) and 2b): picture not on profile. We only have to take action for
+		// the first case: picture is moved into dive-time.
+		if (duringDive) {
+			// Case 2a): add the picture at the appropriate position.
+			// The case move from outside-to-outside of the profile plot was handled by
+			// the "&& duringDive" condition in the if above.
+			// As for case 1a), we have to also consider filenames in the case of equal offsets.
+			auto newPos = std::find_if(pictures.begin(), pictures.end(), [offset, &filename](const PictureEntry &e)
+						   { return std::tie(e.offset.seconds, e.filename) > std::tie(offset.seconds, filename); });
+			// emplace() constructs the element at the given position in the vector.
+			// The parameters are passed directly to the contructor.
+			// The call returns an iterator to the new element (which might differ from
+			// the old iterator, since the buffer might have been reallocated).
+			newPos = pictures.emplace(newPos, offset, filename, scene(), false);
+			updateThumbnailXPos(*newPos);
+			calculatePictureYPositions();
+		}
+	}
+}
+#endif
+
 void ProfileWidget2::dragEnterEvent(QDragEnterEvent *event)
 {
 	if (event->mimeData()->hasFormat("application/x-subsurfaceimagedrop")) {

profile-widget/profilewidget2.h

@@ -117,6 +117,7 @@ slots: // Necessary to call from QAction's signals.
 	void pointsRemoved(const QModelIndex &, int start, int end);
 	void updateThumbnail(QString filename, QImage thumbnail, duration_t duration);
 	void profileChanged(dive *d);
+	void pictureOffsetChanged(dive *d, QString filename, offset_t offset);
 
 	/* this is called for every move on the handlers. maybe we can speed up this a bit? */
 	void recreatePlannedDive();

qt-models/divepicturemodel.cpp

@@ -6,6 +6,7 @@
 #include "core/imagedownloader.h"
 #include "core/picture.h"
 #include "core/qthelper.h"
+#include "core/subsurface-qt/divelistnotifier.h"
 
 #include <QFileInfo>
 #include <QPainter>
@@ -20,6 +21,8 @@ DivePictureModel::DivePictureModel() : zoomLevel(0.0)
 {
 	connect(Thumbnailer::instance(), &Thumbnailer::thumbnailChanged,
 		this, &DivePictureModel::updateThumbnail, Qt::QueuedConnection);
+	connect(&diveListNotifier, &DiveListNotifier::pictureOffsetChanged,
+		this, &DivePictureModel::pictureOffsetChanged);
 }
 
 void DivePictureModel::setZoomLevel(int level)
@@ -212,13 +215,13 @@ void DivePictureModel::updateThumbnail(QString filename, QImage thumbnail, durat
 	}
 }
 
-void DivePictureModel::updateDivePictureOffset(int diveId, const QString &filenameIn, int offsetSeconds)
+void DivePictureModel::pictureOffsetChanged(dive *d, const QString filenameIn, offset_t offset)
 {
 	std::string filename = filenameIn.toStdString();
 
 	// Find the pictures of the given dive.
-	auto from = std::find_if(pictures.begin(), pictures.end(), [diveId](const PictureEntry &e) { return e.diveId == diveId; });
-	auto to = std::find_if(from, pictures.end(), [diveId](const PictureEntry &e) { return e.diveId != diveId; });
+	auto from = std::find_if(pictures.begin(), pictures.end(), [d](const PictureEntry &e) { return e.diveId == d->id; });
+	auto to = std::find_if(from, pictures.end(), [d](const PictureEntry &e) { return e.diveId != d->id; });
 
 	// Find picture with the given filename
 	auto oldPos = std::find_if(from, to, [filename](const PictureEntry &e) { return e.filename == filename; });
@@ -226,20 +229,11 @@ void DivePictureModel::updateDivePictureOffset(int diveId, const QString &filena
 		return;
 
 	// Find new position
-	auto newPos = std::find_if(from, to, [offsetSeconds](const PictureEntry &e) { return e.offsetSeconds > offsetSeconds; });
+	auto newPos = std::find_if(from, to, [offset](const PictureEntry &e) { return e.offsetSeconds > offset.seconds; });
 
 	// Update the offset here and in the backend
-	oldPos->offsetSeconds = offsetSeconds;
-	if (struct dive *dive = get_dive_by_uniq_id(diveId)) {
-		FOR_EACH_PICTURE(dive) {
-			if (picture->filename == filename) {
-				picture->offset.seconds = offsetSeconds;
-				mark_divelist_changed(true);
-				break;
-			}
-		}
-		copy_dive(current_dive, &displayed_dive);
-	}
+	oldPos->offsetSeconds = offset.seconds;
+	copy_dive(current_dive, &displayed_dive); // TODO: remove once profile can display arbitrary dives
 
 	// Henceforth we will work with indices instead of iterators
 	int oldIndex = oldPos - pictures.begin();

qt-models/divepicturemodel.h

@@ -18,6 +18,7 @@ struct PictureEntry {
 	duration_t length;
 };
 
+struct dive;
 class DivePictureModel : public QAbstractTableModel {
 	Q_OBJECT
 public:
@@ -27,12 +28,12 @@ public:
 	int rowCount(const QModelIndex &parent = QModelIndex()) const override;
 	void updateDivePictures();
 	void removePictures(const QVector<QString> &fileUrls);
-	void updateDivePictureOffset(int diveId, const QString &filename, int offsetSeconds);
 signals:
 	void picturesRemoved(const QVector<QString> &fileUrls);
 public slots:
 	void setZoomLevel(int level);
 	void updateThumbnail(QString filename, QImage thumbnail, duration_t duration);
+	void pictureOffsetChanged(dive *d, const QString filename, offset_t offset);
 private:
 	DivePictureModel();
 	QVector<PictureEntry> pictures;