undo: make picture (media) deletion undoable

The code is rather complex. Firstly, we have different representations
of pictures throughout the code. Secondly, this tries to do add the
pictures in batches to the divepicture model and that is always rather
tricky.

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

commands/command.cpp

@@ -367,4 +367,9 @@ void setPictureOffset(dive *d, const QString &filename, offset_t offset)
 	execute(new SetPictureOffset(d, filename, offset));
 }
 
+void removePictures(const std::vector<PictureListForDeletion> &pictures)
+{
+	execute(new RemovePictures(pictures));
+}
+
 } // namespace Command

commands/command.h

@@ -3,6 +3,7 @@
 #define COMMAND_H
 
 #include "core/dive.h"
+#include "core/pictureobj.h"
 #include <QVector>
 #include <QAction>
 #include <vector>
@@ -120,7 +121,16 @@ void addGasSwitch(struct dive *d, int dcNr, int seconds, int tank);
 
 // 7) Picture (media) commands
 
+struct PictureListForDeletion {
+	dive *d;
+	std::vector<std::string> filenames;
+};
+struct PictureListForAddition {
+	dive *d;
+	std::vector<PictureObj> pics;
+};
 void setPictureOffset(dive *d, const QString &filename, offset_t offset);
+void removePictures(const std::vector<PictureListForDeletion> &pictures);
 
 } // namespace Command
 

commands/command_pictures.cpp

@@ -47,4 +47,110 @@ bool SetPictureOffset::workToBeDone()
 	return !!d;
 }
 
+// Filter out pictures that don't exist and sort them according to the backend
+static PictureListForDeletion filterPictureListForDeletion(const PictureListForDeletion &p)
+{
+	PictureListForDeletion res;
+	res.d = p.d;
+	res.filenames.reserve(p.filenames.size());
+	for (int i = 0; i < p.d->pictures.nr; ++i) {
+		std::string fn = p.d->pictures.pictures[i].filename;
+		if (std::find(p.filenames.begin(), p.filenames.end(), fn) != p.filenames.end())
+			res.filenames.push_back(fn);
+	}
+	return res;
+}
+
+// Helper function to remove pictures. Clears the passed-in vector.
+static std::vector<PictureListForAddition> removePictures(std::vector<PictureListForDeletion> &picturesToRemove)
+{
+	std::vector<PictureListForAddition> res;
+	for (const PictureListForDeletion &list: picturesToRemove) {
+		QVector<QString> filenames;
+		PictureListForAddition toAdd;
+		toAdd.d = list.d;
+		for (const std::string &fn: list.filenames) {
+			int idx = get_picture_idx(&list.d->pictures, fn.c_str());
+			if (idx < 0) {
+				fprintf(stderr, "removePictures(): picture disappeared!");
+				continue; // Huh? We made sure that this can't happen by filtering out non-existant pictures.
+			}
+			filenames.push_back(QString::fromStdString(fn));
+			toAdd.pics.emplace_back(list.d->pictures.pictures[idx]);
+			remove_from_picture_table(&list.d->pictures, idx);
+		}
+		if (!toAdd.pics.empty())
+			res.push_back(toAdd);
+		invalidate_dive_cache(list.d);
+		emit diveListNotifier.picturesRemoved(list.d, filenames);
+	}
+	picturesToRemove.clear();
+	return res;
+}
+
+// Helper function to add pictures. Clears the passed-in vector.
+static std::vector<PictureListForDeletion> addPictures(std::vector<PictureListForAddition> &picturesToAdd)
+{
+	// We play it extra safe here and again filter out those pictures that
+	// are already added to the dive. There should be no way for this to
+	// happen, as we checked that before.
+	std::vector<PictureListForDeletion> res;
+	for (const PictureListForAddition &list: picturesToAdd) {
+		QVector<PictureObj> picsForSignal;
+		PictureListForDeletion toRemove;
+		toRemove.d = list.d;
+		for (const PictureObj &pic: list.pics) {
+			int idx = get_picture_idx(&list.d->pictures, pic.filename.c_str()); // This should *not* already exist!
+			if (idx >= 0) {
+				fprintf(stderr, "addPictures(): picture disappeared!");
+				continue; // Huh? We made sure that this can't happen by filtering out existing pictures.
+			}
+			picsForSignal.push_back(pic);
+			add_picture(&list.d->pictures, pic.toCore());
+			toRemove.filenames.push_back(pic.filename);
+		}
+		if (!toRemove.filenames.empty())
+			res.push_back(toRemove);
+		invalidate_dive_cache(list.d);
+		emit diveListNotifier.picturesAdded(list.d, picsForSignal);
+	}
+	picturesToAdd.clear();
+	return res;
+}
+
+RemovePictures::RemovePictures(const std::vector<PictureListForDeletion> &pictures) : picturesToRemove(pictures)
+{
+	// Filter out the pictures that don't actually exist. In principle this shouldn't be necessary.
+	// Nevertheless, let's play it save. This has the additional benefit of sorting the pictures
+	// according to the backend if, for some reason, the caller passed the pictures in in random order.
+	picturesToRemove.reserve(pictures.size());
+	size_t count = 0;
+	for (const PictureListForDeletion &p: pictures) {
+		PictureListForDeletion filtered = filterPictureListForDeletion(p);
+		if (filtered.filenames.size())
+			picturesToRemove.push_back(p);
+		count += filtered.filenames.size();
+	}
+	if (count == 0) {
+		picturesToRemove.clear(); // This signals that nothing is to be done
+		return;
+	}
+	setText(Command::Base::tr("remove %n pictures(s)", "", count));
+}
+
+bool RemovePictures::workToBeDone()
+{
+	return !picturesToRemove.empty();
+}
+
+void RemovePictures::undo()
+{
+	picturesToRemove = addPictures(picturesToAdd);
+}
+
+void RemovePictures::redo()
+{
+	picturesToAdd = removePictures(picturesToRemove);
+}
+
 } // namespace Command

commands/command_pictures.h

@@ -5,6 +5,7 @@
 #define COMMAND_PICTURES_H
 
 #include "command_base.h"
+#include "command.h" // for PictureListForDeletion/Addition
 
 // We put everything in a namespace, so that we can shorten names without polluting the global namespace
 namespace Command {
@@ -22,5 +23,17 @@ private:
 	bool workToBeDone() override;
 };
 
+class RemovePictures final : public Base {
+public:
+	RemovePictures(const std::vector<PictureListForDeletion> &pictures);
+private:
+	std::vector<PictureListForDeletion> picturesToRemove; // for redo
+	std::vector<PictureListForAddition> picturesToAdd; // for undo
+
+	void undo() override;
+	void redo() override;
+	bool workToBeDone() override;
+};
+
 } // namespace Command
 #endif

core/picture.c

@@ -30,7 +30,7 @@ static bool picture_less_than(struct picture a, struct picture b)
 static MAKE_GROW_TABLE(picture_table, struct picture, pictures)
 static MAKE_GET_INSERTION_INDEX(picture_table, struct picture, pictures, picture_less_than)
 MAKE_ADD_TO(picture_table, struct picture, pictures)
-static MAKE_REMOVE_FROM(picture_table, pictures)
+MAKE_REMOVE_FROM(picture_table, pictures)
 MAKE_SORT(picture_table, struct picture, pictures, comp_pictures)
 //MAKE_REMOVE(picture_table, struct picture, picture)
 MAKE_CLEAR_TABLE(picture_table, pictures, picture)
@@ -66,13 +66,3 @@ int get_picture_idx(const struct picture_table *t, const char *filename)
 	}
 	return -1;
 }
-
-// Return true if picture was found and deleted
-bool remove_picture(struct picture_table *t, const char *filename)
-{
-	int idx = get_picture_idx(t, filename);
-	if (idx < 0)
-		return false;
-	remove_from_picture_table(t, idx);
-	return true;
-}

core/picture.h

@@ -33,7 +33,7 @@ extern void clear_picture_table(struct picture_table *);
 extern void add_to_picture_table(struct picture_table *, int idx, struct picture pic);
 extern void copy_pictures(const struct picture_table *s, struct picture_table *d);
 extern void add_picture(struct picture_table *, struct picture newpic);
-extern bool remove_picture(struct picture_table *, const char *filename);
+extern void remove_from_picture_table(struct picture_table *, int idx);
 extern int get_picture_idx(const struct picture_table *, const char *filename); /* Return -1 if not found */
 extern void sort_picture_table(struct picture_table *);
 

core/subsurface-qt/divelistnotifier.h

@@ -6,6 +6,7 @@
 #define DIVELISTNOTIFIER_H
 
 #include "core/dive.h"
+#include "core/pictureobj.h"
 
 #include <QObject>
 
@@ -118,6 +119,8 @@ signals:
 
 	// Picture (media) related signals
 	void pictureOffsetChanged(dive *d, QString filename, offset_t offset);
+	void picturesRemoved(dive *d, QVector<QString> filenames);
+	void picturesAdded(dive *d, QVector<PictureObj> pics);
 
 	// This signal is emited every time a command is executed.
 	// This is used to hide an old multi-dives-edited warning message.

desktop-widgets/tab-widgets/TabDivePhotos.cpp

@@ -84,7 +84,8 @@ QVector<QString> TabDivePhotos::getSelectedFilenames() const
 
 void TabDivePhotos::removeSelectedPhotos()
 {
-	DivePictureModel::instance()->removePictures(getSelectedFilenames());
+	QModelIndexList indices = ui->photosView->selectionModel()->selectedRows();
+	DivePictureModel::instance()->removePictures(indices);
 }
 
 void TabDivePhotos::openFolderOfSelectedFiles()

profile-widget/divepixmapitem.cpp

@@ -1,12 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "profile-widget/divepixmapitem.h"
 #include "profile-widget/animationfunctions.h"
-#include "qt-models/divepicturemodel.h"
 #include "core/pref.h"
 #include "core/qthelper.h"
 #include "core/settings/qPrefDisplay.h"
 #ifndef SUBSURFACE_MOBILE
 #include "desktop-widgets/preferences/preferencesdialog.h"
+#include "core/dive.h" // for displayed_dive
+#include "commands/command.h"
 #endif
 
 #include <QDesktopServices>
@@ -123,6 +124,8 @@ void DivePictureItem::mousePressEvent(QGraphicsSceneMouseEvent *event)
 void DivePictureItem::removePicture()
 {
 #ifndef SUBSURFACE_MOBILE
-	DivePictureModel::instance()->removePictures({ fileUrl });
+	struct dive *d = get_dive_by_uniq_id(displayed_dive.id);
+	if (d)
+		Command::removePictures({ { d, { fileUrl.toStdString() } } });
 #endif
 }

profile-widget/profilewidget2.cpp

@@ -169,9 +169,8 @@ ProfileWidget2::ProfileWidget2(QWidget *parent) : QGraphicsView(parent),
 	addActionShortcut(Qt::Key_Right, &ProfileWidget2::keyRightAction);
 
 	connect(Thumbnailer::instance(), &Thumbnailer::thumbnailChanged, this, &ProfileWidget2::updateThumbnail, Qt::QueuedConnection);
-	connect(DivePictureModel::instance(), &DivePictureModel::rowsInserted, this, &ProfileWidget2::plotPictures);
+	connect(&diveListNotifier, &DiveListNotifier::picturesRemoved, this, &ProfileWidget2::picturesRemoved);
-	connect(DivePictureModel::instance(), &DivePictureModel::picturesRemoved, this, &ProfileWidget2::removePictures);
+	connect(&diveListNotifier, &DiveListNotifier::picturesAdded, this, &ProfileWidget2::picturesAdded);
-	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);
@@ -2147,11 +2146,10 @@ void ProfileWidget2::plotPicturesInternal(const struct dive *d, bool synchronous
 }
 
 // Remove the pictures with the given filenames from the profile plot.
-// TODO: This does not check for the fact that the same image may be attributed
+void ProfileWidget2::picturesRemoved(dive *d, QVector<QString> fileUrls)
-// to different dives! Deleting the picture from one dive may therefore remove
-// it from the profile of a different dive.
-void ProfileWidget2::removePictures(const QVector<QString> &fileUrls)
 {
+	if (d->id != displayed_dive.id)
+		return;
 	// To remove the pictures, we use the std::remove_if() algorithm.
 	// std::remove_if() does not actually delete the elements, but moves
 	// them to the end of the given range. It returns an iterator to the
@@ -2165,6 +2163,25 @@ void ProfileWidget2::removePictures(const QVector<QString> &fileUrls)
 	calculatePictureYPositions();
 }
 
+void ProfileWidget2::picturesAdded(dive *d, QVector<PictureObj> pics)
+{
+	if (d->id != displayed_dive.id)
+		return;
+
+	for (const PictureObj &pic: pics) {
+		if (pic.offset.seconds > 0 && pic.offset.seconds <= d->duration.seconds) {
+			pictures.emplace_back(pic.offset, QString::fromStdString(pic.filename), scene(), false);
+			updateThumbnailXPos(pictures.back());
+		}
+	}
+
+	// Sort pictures by timestamp (and filename if equal timestamps).
+	// This will allow for proper location of the pictures on the profile plot.
+	std::sort(pictures.begin(), pictures.end());
+
+	calculatePictureYPositions();
+}
+
 void ProfileWidget2::profileChanged(dive *d)
 {
 	if (!d || d->id != displayed_dive.id)

profile-widget/profilewidget2.h

@@ -20,6 +20,7 @@
 #include "profile-widget/diveprofileitem.h"
 #include "core/display.h"
 #include "core/color.h"
+#include "core/pictureobj.h"
 #include "core/units.h"
 
 class RulerItem2;
@@ -110,7 +111,8 @@ slots: // Necessary to call from QAction's signals.
 	void setProfileState();
 #ifndef SUBSURFACE_MOBILE
 	void plotPictures();
-	void removePictures(const QVector<QString> &fileUrls);
+	void picturesRemoved(dive *d, QVector<QString> filenames);
+	void picturesAdded(dive *d, QVector<PictureObj> pics);
 	void setPlanState();
 	void setAddState();
 	void pointInserted(const QModelIndex &parent, int start, int end);

qt-models/divepicturemodel.cpp

@@ -1,16 +1,42 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "qt-models/divepicturemodel.h"
+#include "core/divelist.h" // for comp_dives
 #include "core/metrics.h"
-#include "core/divelist.h" // for mark_divelist_changed()
-#include "core/dive.h"
 #include "core/imagedownloader.h"
 #include "core/picture.h"
 #include "core/qthelper.h"
 #include "core/subsurface-qt/divelistnotifier.h"
+#include "commands/command.h"
 
 #include <QFileInfo>
 #include <QPainter>
 
+PictureEntry::PictureEntry(dive *dIn, const PictureObj &p) : d(dIn),
+	filename(p.filename),
+	offsetSeconds(p.offset.seconds),
+	length({ 0 })
+{
+}
+
+PictureEntry::PictureEntry(dive *dIn, const picture &p) : d(dIn),
+	filename(p.filename),
+	offsetSeconds(p.offset.seconds),
+	length({ 0 })
+{
+}
+
+// Note: it is crucial that this uses the same sorting as the core.
+// Therefore, we use the C strcmp functions [std::string::operator<()
+// should give the same result].
+bool PictureEntry::operator<(const PictureEntry &p2) const
+{
+	if (int cmp = comp_dives(d, p2.d))
+		return cmp < 0;
+	if (offsetSeconds != p2.offsetSeconds)
+		return offsetSeconds < p2.offsetSeconds;
+	return strcmp(filename.c_str(), p2.filename.c_str()) < 0;
+}
+
 DivePictureModel *DivePictureModel::instance()
 {
 	static DivePictureModel *self = new DivePictureModel();
@@ -23,6 +49,10 @@ DivePictureModel::DivePictureModel() : zoomLevel(0.0)
 		this, &DivePictureModel::updateThumbnail, Qt::QueuedConnection);
 	connect(&diveListNotifier, &DiveListNotifier::pictureOffsetChanged,
 		this, &DivePictureModel::pictureOffsetChanged);
+	connect(&diveListNotifier, &DiveListNotifier::picturesRemoved,
+		this, &DivePictureModel::picturesRemoved);
+	connect(&diveListNotifier, &DiveListNotifier::picturesAdded,
+		this, &DivePictureModel::picturesAdded);
 }
 
 void DivePictureModel::setZoomLevel(int level)
@@ -63,7 +93,7 @@ void DivePictureModel::updateDivePictures()
 		if (dive->selected) {
 			size_t first = pictures.size();
 			FOR_EACH_PICTURE(dive)
-				pictures.push_back({ dive, picture->filename, {}, picture->offset.seconds, {.seconds = 0}});
+				pictures.push_back(PictureEntry(dive, *picture));
 
 			// Sort pictures of this dive by offset.
 			// Thus, the list will be sorted by (dive, offset).
@@ -111,43 +141,51 @@ QVariant DivePictureModel::data(const QModelIndex &index, int role) const
 	return QVariant();
 }
 
-// Return true if we actually removed a picture
+void DivePictureModel::removePictures(const QModelIndexList &indices)
-static bool removePictureFromSelectedDive(const char *fileUrl)
 {
-	int i;
+	// Collect pictures to remove by dive
-	struct dive *dive;
+	std::vector<Command::PictureListForDeletion> pics;
-	for_each_dive (i, dive) {
+	for (const QModelIndex &idx: indices) {
-		if (dive->selected && remove_picture(&dive->pictures, fileUrl)) {
+		if (!idx.isValid())
-			invalidate_dive_cache(dive);
+			continue;
-			return true;
+		const PictureEntry &item = pictures[idx.row()];
-		}
+		// Check if we already have pictures for that dive.
+		auto it = find_if(pics.begin(), pics.end(),
+				  [&item](const Command::PictureListForDeletion &list)
+				  { return list.d == item.d; });
+		// If not found, add a new list
+		if (it == pics.end())
+			pics.push_back({ item.d, { item.filename }});
+		else
+			it->filenames.push_back(item.filename);
 	}
-	return false;
+	Command::removePictures(pics);
 }
 
-void DivePictureModel::removePictures(const QVector<QString> &fileUrlsIn)
+void DivePictureModel::picturesRemoved(dive *d, QVector<QString> filenamesIn)
 {
 	// Transform vector of QStrings into vector of std::strings
-	std::vector<std::string> fileUrls;
+	std::vector<std::string> filenames;
-	fileUrls.reserve(fileUrlsIn.size());
+	filenames.reserve(filenamesIn.size());
-	std::transform(fileUrlsIn.begin(), fileUrlsIn.end(), std::back_inserter(fileUrls),
+	std::transform(filenamesIn.begin(), filenamesIn.end(), std::back_inserter(filenames),
 		       [] (const QString &s) { return s.toStdString(); });
 
-	bool removed = false;
+	// Get range of pictures of the given dive.
-	for (const std::string &fileUrl: fileUrls)
+	// Note: we could be more efficient by either using a binary search or a two-level data structure.
-		removed |= removePictureFromSelectedDive(fileUrl.c_str());
+	auto from = std::find_if(pictures.begin(), pictures.end(), [d](const PictureEntry &e) { return e.d == d; });
-	if (!removed)
+	auto to = std::find_if(from, pictures.end(), [d](const PictureEntry &e) { return e.d != d; });
+	if (from == pictures.end())
 		return;
-	copy_dive(current_dive, &displayed_dive);
-	mark_divelist_changed(true);
 
-	for (size_t i = 0; i < pictures.size(); ++i) {
+	size_t fromIdx = from - pictures.begin();
+	size_t toIdx = to - pictures.begin();
+	for (size_t i = fromIdx; i < toIdx; ++i) {
 		// Find range [i j) of pictures to remove
-		if (std::find(fileUrls.begin(), fileUrls.end(), pictures[i].filename) == fileUrls.end())
+		if (std::find(filenames.begin(), filenames.end(), pictures[i].filename) == filenames.end())
 			continue;
 		size_t j;
-		for (j = i + 1; j < pictures.size(); ++j) {
+		for (j = i + 1; j < toIdx; ++j) {
-			if (std::find(fileUrls.begin(), fileUrls.end(), pictures[j].filename) == fileUrls.end())
+			if (std::find(filenames.begin(), filenames.end(), pictures[j].filename) == filenames.end())
 				break;
 		}
 
@@ -156,8 +194,48 @@ void DivePictureModel::removePictures(const QVector<QString> &fileUrlsIn)
 		beginRemoveRows(QModelIndex(), i, j - 1);
 		pictures.erase(pictures.begin() + i, pictures.begin() + j);
 		endRemoveRows();
+		toIdx -= j - i;
+	}
+	copy_dive(current_dive, &displayed_dive); // TODO: Remove once displayed_dive is moved to the planner
+}
+
+// Assumes that pics is sorted!
+void DivePictureModel::picturesAdded(dive *d, QVector<PictureObj> picsIn)
+{
+	// We only display pictures of selected dives
+	if (!d->selected || picsIn.empty())
+		return;
+
+	// Convert the picture-data into our own format
+	std::vector<PictureEntry> pics;
+	pics.reserve(picsIn.size());
+	for (int i = 0; i < picsIn.size(); ++i)
+		pics.push_back(PictureEntry(d, picsIn[i]));
+
+	// Insert batch-wise to avoid too many reloads
+	pictures.reserve(pictures.size() + pics.size());
+	auto from = pics.begin();
+	int dest = 0;
+	while (from != pics.end()) {
+		// Search for the insertion index. This supposes a lexicographical sort for the [dive, offset, filename] triple.
+		// TODO: currently this works, because all undo commands that manipulate the dive list also reset the selection
+		// and thus the model is rebuilt. However, we might catch the respective signals here and not rely on being
+		// called by the tab-widgets.
+		auto dest_it = std::lower_bound(pictures.begin() + dest, pictures.end(), *from);
+		int dest = dest_it - pictures.begin();
+		auto to = dest_it == pictures.end() ? pics.end() : from + 1; // If at the end - just add the rest
+		while (to != pics.end() && *to < *dest_it)
+			++to;
+		int batch_size = to - from;
+		beginInsertRows(QModelIndex(), dest, dest + batch_size - 1);
+		pictures.insert(pictures.begin() + dest, from, to);
+		// Get thumbnails of inserted pictures
+		for (auto it = pictures.begin() + dest; it < pictures.begin() + dest + batch_size; ++it)
+			it->image = Thumbnailer::instance()->fetchThumbnail(QString::fromStdString(it->filename), false);
+		endInsertRows();
+		from = to;
+		dest += batch_size;
 	}
-	emit picturesRemoved(fileUrlsIn);
 }
 
 int DivePictureModel::rowCount(const QModelIndex&) const

qt-models/divepicturemodel.h

@@ -3,6 +3,7 @@
 #define DIVEPICTUREMODEL_H
 
 #include "core/units.h"
+#include "core/pictureobj.h"
 
 #include <QAbstractTableModel>
 #include <QImage>
@@ -16,6 +17,9 @@ struct PictureEntry {
 	QImage image;
 	int offsetSeconds;
 	duration_t length;
+	PictureEntry(dive *, const PictureObj &);
+	PictureEntry(dive *, const picture &);
+	bool operator<(const PictureEntry &) const;
 };
 
 class DivePictureModel : public QAbstractTableModel {
@@ -26,13 +30,13 @@ public:
 	QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override;
 	int rowCount(const QModelIndex &parent = QModelIndex()) const override;
 	void updateDivePictures();
-	void removePictures(const QVector<QString> &fileUrls);
+	void removePictures(const QModelIndexList &);
-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);
+	void picturesRemoved(dive *d, QVector<QString> filenames);
+	void picturesAdded(dive *d, QVector<PictureObj> pics);
 private:
 	DivePictureModel();
 	std::vector<PictureEntry> pictures;