undo: make adding of pictures undoable

This one is a bit hairy, because two things might happen if the
picture has a geo location:
- A dive gets a newly generated dive site set.
- The dive site of a dive is edited.
Therefore the undo command has to store keep track of that.
Oh my.

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

commands/command.cpp

@@ -372,4 +372,9 @@ void removePictures(const std::vector<PictureListForDeletion> &pictures)
 	execute(new RemovePictures(pictures));
 }
 
+void addPictures(const std::vector<PictureListForAddition> &pictures)
+{
+	execute(new AddPictures(pictures));
+}
+
 } // namespace Command

commands/command.h

@@ -131,6 +131,7 @@ struct PictureListForAddition {
 };
 void setPictureOffset(dive *d, const QString &filename, offset_t offset);
 void removePictures(const std::vector<PictureListForDeletion> &pictures);
+void addPictures(const std::vector<PictureListForAddition> &pictures);
 
 } // namespace Command
 

commands/command_pictures.cpp

@@ -2,6 +2,7 @@
 
 #include "command_pictures.h"
 #include "core/subsurface-qt/divelistnotifier.h"
+#include "qt-models/divelocationmodel.h"
 
 namespace Command {
 
@@ -153,4 +154,87 @@ void RemovePictures::redo()
 	picturesToAdd = removePictures(picturesToRemove);
 }
 
+AddPictures::AddPictures(const std::vector<PictureListForAddition> &pictures) : picturesToAdd(pictures)
+{
+	// Sort the pictures according to the backend-rules. Moreover see if we have to set / create divesites.
+	size_t count = 0;
+	for (PictureListForAddition &p: picturesToAdd) {
+		count += p.pics.size();
+		std::sort(p.pics.begin(), p.pics.end());
+
+		// Find a picture with a location
+		auto it = std::find_if(p.pics.begin(), p.pics.end(), [](const PictureObj &p) { return has_location(&p.location); });
+		if (it != p.pics.end()) {
+			// There is a dive with a location, we might want to modify the dive accordingly.
+			struct dive_site *ds = p.d->dive_site;
+			if (!ds) {
+				// This dive doesn't yet have a dive site -> add a new dive site.
+				dive_site *ds = alloc_dive_site_with_gps("", &it->location);
+				sitesToAdd.emplace_back(ds);
+				sitesToSet.push_back({ p.d, ds });
+			} else if (!dive_site_has_gps_location(ds)) {
+				// This dive has a dive site, but without coordinates. Let's add them.
+				sitesToEdit.push_back({ ds, it->location });
+			}
+		}
+	}
+
+	if (count == 0) {
+		picturesToAdd.clear(); // This signals that nothing is to be done
+		return;
+	}
+	setText(Command::Base::tr("add %n pictures(s)", "", count));
+}
+
+bool AddPictures::workToBeDone()
+{
+	return !picturesToAdd.empty();
+}
+
+void AddPictures::swapDiveSites()
+{
+	for (DiveSiteEntry &entry: sitesToSet) {
+		dive_site *ds = entry.d->dive_site;
+		if (ds)
+			unregister_dive_from_dive_site(entry.d); // the dive-site pointer in the dive is now NULL
+		std::swap(ds, entry.ds);
+		if (ds)
+			add_dive_to_dive_site(entry.d, ds);
+		emit diveListNotifier.divesChanged(QVector<dive *>{ entry.d }, DiveField::DIVESITE);
+	}
+
+	for (DiveSiteEditEntry &entry: sitesToEdit) {
+		std::swap(entry.ds->location, entry.location);
+		emit diveListNotifier.diveSiteChanged(entry.ds, LocationInformationModel::LOCATION); // Inform frontend of changed dive site.
+	}
+}
+
+void AddPictures::undo()
+{
+	swapDiveSites();
+	picturesToAdd = removePictures(picturesToRemove);
+
+	// Remove dive sites
+	for (dive_site *siteToRemove: sitesToRemove) {
+		int idx = unregister_dive_site(siteToRemove);
+		sitesToAdd.emplace_back(siteToRemove);
+		emit diveListNotifier.diveSiteDeleted(siteToRemove, idx); // Inform frontend of removed dive site.
+	}
+	sitesToRemove.clear();
+}
+
+void AddPictures::redo()
+{
+	// Add dive sites
+	for (OwningDiveSitePtr &siteToAdd: sitesToAdd) {
+		sitesToRemove.push_back(siteToAdd.get());
+		int idx = register_dive_site(siteToAdd.release()); // Return ownership to backend.
+		emit diveListNotifier.diveSiteAdded(sitesToRemove.back(), idx); // Inform frontend of new dive site.
+	}
+	sitesToAdd.clear();
+
+	swapDiveSites();
+	picturesToRemove = addPictures(picturesToAdd);
+}
+
 } // namespace Command

commands/command_pictures.h

@@ -35,5 +35,31 @@ private:
 	bool workToBeDone() override;
 };
 
+class AddPictures final : public Base {
+public:
+	AddPictures(const std::vector<PictureListForAddition> &pictures);
+private:
+	struct DiveSiteEntry {
+		dive *d;
+		dive_site *ds;
+	};
+	struct DiveSiteEditEntry {
+		dive_site *ds;
+		location_t location;
+	};
+	std::vector<PictureListForAddition> picturesToAdd; // for redo
+	std::vector<OwningDiveSitePtr> sitesToAdd; //for redo
+	std::vector<PictureListForDeletion> picturesToRemove; // for undo
+	std::vector<dive_site *> sitesToRemove; // for undo
+	std::vector<DiveSiteEntry> sitesToSet; // for redo and undo
+	std::vector<DiveSiteEditEntry> sitesToEdit; // for redo and undo
+
+	void swapDiveSites();
+
+	void undo() override;
+	void redo() override;
+	bool workToBeDone() override;
+};
+
 } // namespace Command
 #endif

core/dive.c

@@ -3497,20 +3497,6 @@ void set_git_prefs(const char *prefs)
 		git_prefs.pp_graphs.po2 = 1;
 }
 
-void dive_set_geodata_from_picture(struct dive *dive, struct picture *picture, struct dive_site_table *table)
-{
-	struct dive_site *ds = dive->dive_site;
-	if (!dive_site_has_gps_location(ds) && has_location(&picture->location)) {
-		if (ds) {
-			ds->location = picture->location;
-		} else {
-			ds = create_dive_site_with_gps("", &picture->location, table);
-			add_dive_to_dive_site(dive, ds);
-			invalidate_dive_cache(dive);
-		}
-	}
-}
-
 /* clones a dive and moves given dive computer to front */
 struct dive *make_first_dc(const struct dive *d, int dc_number)
 {

core/dive.h

@@ -204,8 +204,6 @@ extern enum divemode_t get_current_divemode(const struct divecomputer *dc, int t
 extern struct event *get_next_divemodechange(const struct event **evd, bool update_pointer);
 extern enum divemode_t get_divemode_at_time(const struct divecomputer *dc, int dtime, const struct event **ev_dmc);
 
-extern void dive_set_geodata_from_picture(struct dive *dive, struct picture *picture, struct dive_site_table *table);
-
 extern bool has_gaschange_event(const struct dive *dive, const struct divecomputer *dc, int idx);
 extern int explicit_first_cylinder(const struct dive *dive, const struct divecomputer *dc);
 extern int get_depth_at_time(const struct divecomputer *dc, unsigned int time);

desktop-widgets/divelistview.cpp

@@ -891,20 +891,27 @@ void DiveListView::matchImagesToDives(QStringList fileNames)
 		return;
 	updateLastImageTimeOffset(shiftDialog.amount());
 
+	// Create the data structure of pictures to be added: a list of pictures per dive.
+	std::vector<Command::PictureListForAddition> pics;
 	for (const QString &fileName: fileNames) {
 		struct dive *d;
 		picture *pic = create_picture(qPrintable(fileName), shiftDialog.amount(), shiftDialog.matchAll(), &d);
 		if (!pic)
 			continue;
-		add_picture(&d->pictures, *pic);
+		PictureObj pObj(*pic);
-		dive_set_geodata_from_picture(d, pic, &dive_site_table);
-		invalidate_dive_cache(d);
 		free(pic);
+
+		auto it = std::find_if(pics.begin(), pics.end(), [d](const Command::PictureListForAddition &l) { return l.d == d; });
+		if (it == pics.end())
+			pics.push_back(Command::PictureListForAddition { d, { pObj } });
+		else
+			it->pics.push_back(pObj);
 	}
 
-	mark_divelist_changed(true);
+	if (pics.empty())
-	copy_dive(current_dive, &displayed_dive);
+		return;
-	DivePictureModel::instance()->updateDivePictures();
+
+	Command::addPictures(pics);
 }
 
 void DiveListView::loadWebImages()