core: turn picture-table into std::vector<>

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

Subsurface-mobile.pro

@@ -66,7 +66,6 @@ SOURCES += subsurface-mobile-main.cpp \
 	core/parse-xml.cpp \
 	core/parse.cpp \
 	core/picture.cpp \
-	core/pictureobj.cpp \
 	core/sample.cpp \
 	core/import-suunto.cpp \
 	core/import-shearwater.cpp \
@@ -217,7 +216,6 @@ HEADERS += \
 	core/units.h \
 	core/version.h \
 	core/picture.h \
-	core/pictureobj.h \
 	core/planner.h \
 	core/divesite.h \
 	core/checkcloudconnection.h \

backend-shared/exportfuncs.cpp

@@ -280,14 +280,14 @@ void export_depths(const char *filename, bool selected_only)
 		if (selected_only && !dive->selected)
 			continue;
 
-		FOR_EACH_PICTURE (dive) {
+		for (auto &picture: dive->pictures) {
 			depth_t depth;
 			for (auto &s: dive->dcs[0].samples) {
-				if ((int32_t)s.time.seconds > picture->offset.seconds)
+				if ((int32_t)s.time.seconds > picture.offset.seconds)
 					break;
 				depth = s.depth;
 			}
-			put_format(&buf, "%s\t%.1f", picture->filename, get_depth_units(depth.mm, NULL, &unit));
+			put_format(&buf, "%s\t%.1f", picture.filename.c_str(), get_depth_units(depth.mm, NULL, &unit));
 			put_format(&buf, "%s\n", unit);
 		}
 	}

commands/command.h

@@ -4,7 +4,7 @@
 
 #include "core/divelog.h"
 #include "core/equipment.h"
-#include "core/pictureobj.h"
+#include "core/picture.h"
 #include "core/taxonomy.h"
 #include <QVector>
 #include <QAction>
@@ -144,7 +144,7 @@ struct PictureListForDeletion {
 };
 struct PictureListForAddition {
 	dive *d;
-	std::vector<PictureObj> pics;
+	std::vector<picture> pics;
 };
 void setPictureOffset(dive *d, const QString &filename, offset_t offset);
 void removePictures(const std::vector<PictureListForDeletion> &pictures);

commands/command_pictures.cpp

@@ -2,15 +2,16 @@
 
 #include "command_pictures.h"
 #include "core/errorhelper.h"
+#include "core/range.h"
 #include "core/subsurface-qt/divelistnotifier.h"
 #include "qt-models/divelocationmodel.h"
 
 namespace Command {
 
-static picture *dive_get_picture(const dive *d, const QString &fn)
+static picture *dive_get_picture(dive *d, const QString &fn)
 {
-	int idx = get_picture_idx(&d->pictures, qPrintable(fn));
+	int idx = get_picture_idx(d->pictures, fn.toStdString());
-	return idx < 0 ? nullptr : &d->pictures.pictures[idx];
+	return idx < 0 ? nullptr : &d->pictures[idx];
 }
 
 SetPictureOffset::SetPictureOffset(dive *dIn, const QString &filenameIn, offset_t offsetIn) :
@@ -33,7 +34,7 @@ void SetPictureOffset::redo()
 
 	// 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);
+	std::sort(d->pictures.begin(), d->pictures.end());
 	emit diveListNotifier.pictureOffsetChanged(d, filename, newOffset);
 	invalidate_dive_cache(d);
 }
@@ -55,10 +56,9 @@ static PictureListForDeletion filterPictureListForDeletion(const PictureListForD
 	PictureListForDeletion res;
 	res.d = p.d;
 	res.filenames.reserve(p.filenames.size());
-	for (int i = 0; i < p.d->pictures.nr; ++i) {
+	for (auto &pic: p.d->pictures) {
-		std::string fn = p.d->pictures.pictures[i].filename;
+		if (range_contains(p.filenames, pic.filename))
-		if (std::find(p.filenames.begin(), p.filenames.end(), fn) != p.filenames.end())
+			res.filenames.push_back(pic.filename);
-			res.filenames.push_back(fn);
 	}
 	return res;
 }
@@ -72,14 +72,14 @@ static std::vector<PictureListForAddition> removePictures(std::vector<PictureLis
 		PictureListForAddition toAdd;
 		toAdd.d = list.d;
 		for (const std::string &fn: list.filenames) {
-			int idx = get_picture_idx(&list.d->pictures, fn.c_str());
+			int idx = get_picture_idx(list.d->pictures, fn);
 			if (idx < 0) {
 				report_info("removePictures(): picture disappeared!");
 				continue; // Huh? We made sure that this can't happen by filtering out non-existent pictures.
 			}
 			filenames.push_back(QString::fromStdString(fn));
-			toAdd.pics.emplace_back(list.d->pictures.pictures[idx]);
+			toAdd.pics.emplace_back(list.d->pictures[idx]);
-			remove_from_picture_table(&list.d->pictures, idx);
+			list.d->pictures.erase(list.d->pictures.begin() + idx);
 		}
 		if (!toAdd.pics.empty())
 			res.push_back(toAdd);
@@ -98,17 +98,17 @@ static std::vector<PictureListForDeletion> addPictures(std::vector<PictureListFo
 	// happen, as we checked that before.
 	std::vector<PictureListForDeletion> res;
 	for (const PictureListForAddition &list: picturesToAdd) {
-		QVector<PictureObj> picsForSignal;
+		QVector<picture> picsForSignal;
 		PictureListForDeletion toRemove;
 		toRemove.d = list.d;
-		for (const PictureObj &pic: list.pics) {
+		for (const picture &pic: list.pics) {
-			int idx = get_picture_idx(&list.d->pictures, pic.filename.c_str()); // This should *not* already exist!
+			int idx = get_picture_idx(list.d->pictures, pic.filename); // This should *not* already exist!
 			if (idx >= 0) {
 				report_info("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());
+			add_picture(list.d->pictures, pic);
 			toRemove.filenames.push_back(pic.filename);
 		}
 		if (!toRemove.filenames.empty())
@@ -164,7 +164,7 @@ AddPictures::AddPictures(const std::vector<PictureListForAddition> &pictures) :
 		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); });
+		auto it = std::find_if(p.pics.begin(), p.pics.end(), [](const picture &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;

core/CMakeLists.txt

@@ -138,8 +138,6 @@ set(SUBSURFACE_CORE_LIB_SRCS
 	parse.h
 	picture.cpp
 	picture.h
-	pictureobj.cpp
-	pictureobj.h
 	planner.cpp
 	planner.h
 	plannernotes.cpp

core/dive.cpp

@@ -49,10 +49,9 @@ dive::dive() : dcs(1)
 	id = dive_getUniqID();
 }
 
-static void free_dive_structures(struct dive *d);
 dive::~dive()
 {
-	free_dive_structures(this);
+	fulltext_unregister(this);	// TODO: this is a layering violation. Remove.
 }
 
 dive::dive(dive &&) = default;
@@ -163,24 +162,6 @@ static void copy_dc_renumber(struct dive *d, const struct dive *s, const int cyl
 	}
 }
 
-static void free_dive_structures(struct dive *d)
-{
-	if (!d)
-		return;
-	fulltext_unregister(d);
-	/* free the strings */
-	d->buddy.clear();
-	d->diveguide.clear();
-	d->notes.clear();
-	d->suit.clear();
-	/* free tags, additional dive computers, and pictures */
-	d->tags.clear();
-	d->cylinders.clear();
-	d->weightsystems.clear();
-	clear_picture_table(&d->pictures);
-	free(d->pictures.pictures);
-}
-
 /* copy_dive makes duplicates of many components of a dive;
  * in order not to leak memory, we need to free those.
  * copy_dive doesn't play with the divetrip and forward/backward pointers
@@ -189,7 +170,7 @@ void clear_dive(struct dive *d)
 {
 	if (!d)
 		return;
-	free_dive_structures(d);
+	fulltext_unregister(d);
 	*d = dive();
 }
 
@@ -198,15 +179,11 @@ void clear_dive(struct dive *d)
  * any impact on the source */
 void copy_dive(const struct dive *s, struct dive *d)
 {
-	clear_dive(d);
+	/* simply copy things over, but then clear fulltext cache and dive cache. */
-	/* simply copy things over, but then make actual copies of the
+	fulltext_unregister(d);
-	 * relevant components that are referenced through pointers,
-	 * so all the strings and the structured lists */
 	*d = *s;
-	memset(&d->pictures, 0, sizeof(d->pictures));
 	d->full_text = NULL;
 	invalidate_dive_cache(d);
-	copy_pictures(&s->pictures, &d->pictures);
 }
 
 static void copy_dive_onedc(const struct dive *s, const struct divecomputer &sdc, struct dive *d)
@@ -2336,7 +2313,7 @@ struct dive *merge_dives(const struct dive *a, const struct dive *b, int offset,
 	MERGE_NONZERO(res, a, b, current);
 	MERGE_NONZERO(res, a, b, surge);
 	MERGE_NONZERO(res, a, b, chill);
-	copy_pictures(a->pictures.nr ? &a->pictures : &b->pictures, &res->pictures);
+	res->pictures = !a->pictures.empty() ? a->pictures : b->pictures;
 	res->tags = taglist_merge(a->tags, b->tags);
 	/* if we get dives without any gas / cylinder information in an import, make sure
 	 * that there is at leatst one entry in the cylinder map for that dive */

core/dive.h

@@ -50,7 +50,7 @@ struct dive {
 	tag_list tags;
 	std::vector<divecomputer> dcs; // Attn: pointers to divecomputers are not stable!
 	int id = 0; // unique ID for this dive
-	struct picture_table pictures = { };
+	picture_table pictures;
 	unsigned char git_id[20] = {};
 	bool notrip = false; /* Don't autogroup this dive to a trip */
 	bool selected = false;

core/load-git.cpp

@@ -46,7 +46,7 @@ struct git_parser_state {
 	std::string filter_constraint_range_mode;
 	bool filter_constraint_negate = false;
 	std::string filter_constraint_data;
-	struct picture active_pic = { 0 };
+	struct picture active_pic;
 	struct dive_site *active_site = nullptr;
 	std::unique_ptr<filter_preset> active_filter;
 	struct divelog *log = nullptr;
@@ -1048,7 +1048,7 @@ static void parse_settings_fingerprint(char *line, struct git_parser_state *stat
 
 static void parse_picture_filename(char *, struct git_parser_state *state)
 {
-	state->active_pic.filename = get_first_converted_string_c(state);
+	state->active_pic.filename = get_first_converted_string(state);
 }
 
 static void parse_picture_gps(char *line, struct git_parser_state *state)
@@ -1756,7 +1756,7 @@ static int parse_picture_entry(struct git_parser_state *state, const git_tree_en
 	state->active_pic.offset.seconds = offset;
 
 	for_each_line(blob, picture_parser, state);
-	add_picture(&state->active_dive->pictures, state->active_pic);
+	add_picture(state->active_dive->pictures, std::move(state->active_pic));
 	git_blob_free(blob);
 
 	/* add_picture took ownership of the data -

core/parse-xml.cpp

@@ -1271,7 +1271,7 @@ static void try_to_fill_dive(struct dive *dive, const char *name, char *buf, str
 	if (match_dc_data_fields(&dive->dcs[0], name, buf, state))
 		return;
 
-	if (MATCH("filename.picture", utf8_string, &state->cur_picture.filename))
+	if (MATCH("filename.picture", utf8_string_std, &state->cur_picture.filename))
 		return;
 	if (MATCH("offset.picture", offsettime, &state->cur_picture.offset))
 		return;

core/parse.cpp

@@ -70,10 +70,10 @@ void event_end(struct parser_state *state)
 	struct divecomputer *dc = get_dc(state);
 	if (state->cur_event.type == 123) {
 		struct picture pic;
-		pic.filename = strdup(state->cur_event.name.c_str());
+		pic.filename = state->cur_event.name;
 		/* theoretically this could fail - but we didn't support multi year offsets */
 		pic.offset.seconds = state->cur_event.time.seconds;
-		add_picture(&state->cur_dive->pictures, pic); /* Takes ownership. */
+		add_picture(state->cur_dive->pictures, std::move(pic));
 	} else {
 		/* At some point gas change events did not have any type. Thus we need to add
 		 * one on import, if we encounter the type one missing.
@@ -307,7 +307,7 @@ void picture_start(struct parser_state *state)
 
 void picture_end(struct parser_state *state)
 {
-	add_picture(&state->cur_dive->pictures, state->cur_picture);
+	add_picture(state->cur_dive->pictures, std::move(state->cur_picture));
 	/* dive_add_picture took ownership, we can just clear out copy of the data */
 	state->cur_picture = picture();
 }

core/parse.h

@@ -59,7 +59,7 @@ struct parser_state {
 	location_t cur_location;
 	struct dive_trip *cur_trip = nullptr;			/* owning */
 	struct sample *cur_sample = nullptr;			/* non-owning */
-	struct picture cur_picture { 0 };			/* owning */
+	struct picture cur_picture;				/* owning */
 	std::unique_ptr<filter_preset> cur_filter;		/* owning */
 	std::string fulltext;					/* owning */
 	std::string fulltext_string_mode;			/* owning */

core/picture.cpp

@@ -4,71 +4,28 @@
 #if !defined(SUBSURFACE_MOBILE)
 #include "metadata.h"
 #endif
+#include "range.h"
 #include "subsurface-string.h"
-#include "table.h"
 #include <stdlib.h>
 #include <string.h>
 
-static void free_picture(struct picture picture)
+bool picture::operator<(const struct picture &p) const
 {
-	free(picture.filename);
+	return std::tie(offset.seconds, filename) <
-	picture.filename = NULL;
+	       std::tie(p.offset.seconds, p.filename);
 }
 
-static int comp_pictures(struct picture a, struct picture b)
+void add_picture(picture_table &t, struct picture newpic)
 {
-	if (a.offset.seconds < b.offset.seconds)
+	auto it = std::lower_bound(t.begin(), t.end(), newpic);
-		return -1;
+	t.insert(it, std::move(newpic));
-	if (a.offset.seconds > b.offset.seconds)
-		return 1;
-	return strcmp(a.filename ?: "", b.filename ?: "");
 }
 
-static bool picture_less_than(struct picture a, struct picture b)
+int get_picture_idx(const picture_table &t, const std::string &filename)
 {
-	return comp_pictures(a, b) < 0;
-}
 
-/* picture table functions */
+	return index_of_if(t, [&filename] (const picture &p)
-//static MAKE_GET_IDX(picture_table, struct picture, pictures)
+			   { return p.filename == filename; });
-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)
-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)
-
-/* Add a clone of a picture to the end of a picture table.
- * Cloned means that the filename-string is copied. */
-static void add_cloned_picture(struct picture_table *t, struct picture pic)
-{
-	pic.filename = copy_string(pic.filename);
-	int idx = picture_table_get_insertion_index(t, pic);
-	add_to_picture_table(t, idx, pic);
-}
-
-void copy_pictures(const struct picture_table *s, struct picture_table *d)
-{
-	int i;
-	clear_picture_table(d);
-	for (i = 0; i < s->nr; i++)
-		add_cloned_picture(d, s->pictures[i]);
-}
-
-void add_picture(struct picture_table *t, struct picture newpic)
-{
-	int idx = picture_table_get_insertion_index(t, newpic);
-	add_to_picture_table(t, idx, newpic);
-}
-
-int get_picture_idx(const struct picture_table *t, const char *filename)
-{
-	for (int i = 0; i < t->nr; ++i) {
-		if (same_string(t->pictures[i].filename, filename))
-			return i;
-	}
-	return -1;
 }
 
 #if !defined(SUBSURFACE_MOBILE)
@@ -113,37 +70,37 @@ static struct dive *nearest_selected_dive(timestamp_t timestamp)
 
 // only add pictures that have timestamps between 30 minutes before the dive and
 // 30 minutes after the dive ends
-#define D30MIN (30 * 60)
+static constexpr timestamp_t d30min = 30 * 60;
 static bool dive_check_picture_time(const struct dive *d, timestamp_t timestamp)
 {
-	return time_from_dive(d, timestamp) < D30MIN;
+	return time_from_dive(d, timestamp) < d30min;
 }
 
 /* Creates a picture and indicates the dive to which this picture should be added.
  * The caller is responsible for actually adding the picture to the dive.
- * If no appropriate dive was found, no picture is created and NULL is returned.
+ * If no appropriate dive was found, no picture is created and null is returned.
  */
-struct picture *create_picture(const char *filename, timestamp_t shift_time, bool match_all, struct dive **dive)
+std::pair<std::optional<picture>, dive *> create_picture(const std::string &filename, timestamp_t shift_time, bool match_all)
 {
 	struct metadata metadata;
 	timestamp_t timestamp;
 
-	get_metadata(filename, &metadata);
+	get_metadata(filename.c_str(), &metadata);
 	timestamp = metadata.timestamp + shift_time;
-	*dive = nearest_selected_dive(timestamp);
+	struct dive *dive = nearest_selected_dive(timestamp);
 
-	if (!*dive)
+	if (!dive)
-		return NULL;
+		return { {}, nullptr };
-	if (get_picture_idx(&(*dive)->pictures, filename) >= 0)
+	if (get_picture_idx(dive->pictures, filename) >= 0)
-		return NULL;
+		return { {}, nullptr };
-	if (!match_all && !dive_check_picture_time(*dive, timestamp))
+	if (!match_all && !dive_check_picture_time(dive, timestamp))
-		return NULL;
+		return { {}, nullptr };
 
-	struct picture *picture = (struct picture *)malloc(sizeof(struct picture));
+	struct picture picture;
-	picture->filename = strdup(filename);
+	picture.filename = filename;
-	picture->offset.seconds = metadata.timestamp - (*dive)->when + shift_time;
+	picture.offset.seconds = metadata.timestamp - dive->when + shift_time;
-	picture->location = metadata.location;
+	picture.location = metadata.location;
-	return picture;
+	return { picture, dive };
 }
 
 bool picture_check_valid_time(timestamp_t timestamp, timestamp_t shift_time)

core/picture.h

@@ -1,47 +1,35 @@
 // SPDX-License-Identifier: GPL-2.0
+// picture (more precisely media) related strutures and functions
 #ifndef PICTURE_H
 #define PICTURE_H
 
-// picture (more precisely media) related strutures and functions
 #include "units.h"
-#include <stddef.h> // For NULL
+#include <optional>
+#include <string>
+#include <vector>
 
 struct dive;
 
 struct picture {
-	char *filename = nullptr;
+	std::string filename;
 	offset_t offset;
 	location_t location;
+	bool operator<(const picture &) const;
 };
 
-/* loop through all pictures of a dive */
-#define FOR_EACH_PICTURE(_dive)								\
-	if ((_dive) && (_dive)->pictures.nr)						\
-		for (struct picture *picture = (_dive)->pictures.pictures;		\
-		     picture < (_dive)->pictures.pictures + (_dive)->pictures.nr;	\
-		     picture++)
-
 /* Table of pictures. Attention: this stores pictures,
- * *not* pointers to pictures. This has two crucial consequences:
+ * *not* pointers to pictures. This means that
- * 1) Pointers to pictures are not stable. They may be
+ * pointers to pictures are not stable. They are
- *    invalidated if the table is reallocated.
+ * invalidated if the table is reallocated.
- * 2) add_to_picture_table(), etc. take ownership of the
+ */
- *    picture. Notably of the filename. */
+using picture_table = std::vector<picture>;
-struct picture_table {
-	int nr, allocated;
-	struct picture *pictures;
-};
 
 /* picture table functions */
-extern void clear_picture_table(struct picture_table *);
+extern void add_to_picture_table(picture_table &, int idx, struct picture pic);
-extern void add_to_picture_table(struct picture_table *, int idx, struct picture pic);
+extern void add_picture(picture_table &, struct picture newpic);
-extern void copy_pictures(const struct picture_table *s, struct picture_table *d);
+extern int get_picture_idx(const picture_table &, const std::string &filename); /* Return -1 if not found */
-extern void add_picture(struct picture_table *, struct picture newpic);
-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 *);
 
-extern struct picture *create_picture(const char *filename, timestamp_t shift_time, bool match_all, struct dive **dive);
+extern std::pair<std::optional<picture>, dive *> create_picture(const std::string &filename, timestamp_t shift_time, bool match_all);
 extern bool picture_check_valid_time(timestamp_t timestamp, timestamp_t shift_time);
 
 #endif // PICTURE_H

core/pictureobj.cpp

@@ -1,27 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include "pictureobj.h"
-#include "qthelper.h"
-
-PictureObj::PictureObj() : offset({ 0 }), location({ 0 })
-{
-}
-
-PictureObj::PictureObj(const picture &pic) : filename(pic.filename), offset(pic.offset), location(pic.location)
-{
-}
-
-picture PictureObj::toCore() const
-{
-	return picture {
-		strdup(filename.c_str()),
-		offset,
-		location
-	};
-}
-
-bool PictureObj::operator<(const PictureObj &p2) const
-{
-	if (offset.seconds != p2.offset.seconds)
-		return offset.seconds < p2.offset.seconds;
-	return strcmp(filename.c_str(), p2.filename.c_str()) < 0;
-}

core/pictureobj.h

@@ -1,25 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#ifndef PICTUREOBJ_H
-#define PICTUREOBJ_H
-
-// A tiny helper class that represents a struct picture of the core
-// It does, however, keep the filename as a std::string so that C++ code
-// doesn't have do its own memory-management.
-
-#include "core/units.h"
-#include "core/picture.h"
-#include <string>
-
-struct PictureObj {
-	std::string filename;
-	offset_t offset;
-	location_t location;
-
-	PictureObj();			// Initialize to empty picture.
-	PictureObj(const picture &pic); // Create from core struct picture.
-	picture toCore() const;		// Turn into core structure. Caller responsible for freeing.
-	bool operator<(const PictureObj &p2) const;
-};
-
-#endif // PICTUREOBJ_H

core/qthelper.cpp

@@ -333,24 +333,12 @@ std::string move_away(const std::string &old_path)
 	return newPath;
 }
 
-std::string get_file_name(const char *fileName)
+std::string get_file_name(const std::string &fileName)
 {
-	QFileInfo fileInfo(fileName);
+	QFileInfo fileInfo(fileName.c_str());
 	return fileInfo.fileName().toStdString();
 }
 
-void copy_image_and_overwrite(const char *cfileName, const char *path, const char *cnewName)
-{
-	QString fileName(cfileName);
-	QString newName(path);
-	newName += cnewName;
-	QFile file(newName);
-	if (file.exists())
-		file.remove();
-	if (!QFile::copy(fileName, newName))
-		report_info("copy of %s to %s failed", cfileName, qPrintable(newName));
-}
-
 static bool lessThan(const QPair<QString, int> &a, const QPair<QString, int> &b)
 {
 	return a.second < b.second;
@@ -1174,9 +1162,9 @@ QStringList videoExtensionFilters()
 	return filters;
 }
 
-const char *local_file_path(struct picture *picture)
+std::string local_file_path(const struct picture &picture)
 {
-	return copy_qstring(localFilePath(picture->filename));
+	return localFilePath(QString::fromStdString(picture.filename)).toStdString();
 }
 
 QString get_gas_string(struct gasmix gas)

core/qthelper.h

@@ -98,7 +98,7 @@ extern QString (*changesCallback)();
 void uiNotification(const QString &msg);
 std::string get_changes_made();
 std::string subsurface_user_agent();
-std::string get_file_name(const char *fileName);
+std::string get_file_name(const std::string &fileName);
 std::string move_away(const std::string &path);
 
 #if defined __APPLE__
@@ -110,8 +110,7 @@ std::string move_away(const std::string &path);
 bool canReachCloudServer(struct git_info *);
 void updateWindowTitle();
 void subsurface_mkdir(const char *dir);
-void copy_image_and_overwrite(const char *cfileName, const char *path, const char *cnewName);
+std::string local_file_path(const struct picture &picture);
-const char *local_file_path(struct picture *picture);
 char *hashfile_name_string();
 enum deco_mode decoMode(bool in_planner);
 void parse_seabear_header(const char *filename, struct xml_params *params);

core/save-git.cpp

@@ -612,15 +612,15 @@ static int save_one_divecomputer(git_repository *repo, struct dir *tree, struct
 	return ret;
 }
 
-static int save_one_picture(git_repository *repo, struct dir *dir, struct picture *pic)
+static int save_one_picture(git_repository *repo, struct dir *dir, const struct picture &pic)
 {
-	int offset = pic->offset.seconds;
+	int offset = pic.offset.seconds;
 	membuffer buf;
 	char sign = '+';
 	unsigned h;
 
-	show_utf8(&buf, "filename ", pic->filename, "\n");
+	show_utf8(&buf, "filename ", pic.filename.c_str(), "\n");
-	put_location(&buf, &pic->location, "gps ", "\n");
+	put_location(&buf, &pic.location, "gps ", "\n");
 
 	/* Picture loading will load even negative offsets.. */
 	if (offset < 0) {
@@ -637,11 +637,10 @@ static int save_one_picture(git_repository *repo, struct dir *dir, struct pictur
 
 static int save_pictures(git_repository *repo, struct dir *dir, struct dive *dive)
 {
-	if (dive->pictures.nr > 0) {
+	if (!dive->pictures.empty()) {
 		dir = mktree(repo, dir, "Pictures");
-		FOR_EACH_PICTURE(dive) {
+		for (auto &picture: dive->pictures)
 			save_one_picture(repo, dir, picture);
-		}
 	}
 	return 0;
 }

core/save-html.cpp

@@ -21,6 +21,7 @@
 
 #include <stdio.h>
 #include <string.h>
+#include <QFile>
 
 static void write_attribute(struct membuffer *b, const char *att_name, const char *value, const char *separator)
 {
@@ -31,13 +32,24 @@ static void write_attribute(struct membuffer *b, const char *att_name, const cha
 	put_format(b, "\"%s", separator);
 }
 
+static void copy_image_and_overwrite(const std::string &cfileName, const std::string &path, const std::string &cnewName)
+{
+	QString fileName = QString::fromStdString(cfileName);
+	std::string newName = path + cnewName;
+	QFile file(QString::fromStdString(newName));
+	if (file.exists())
+		file.remove();
+	if (!QFile::copy(fileName, QString::fromStdString(newName)))
+		report_info("copy of %s to %s failed", cfileName.c_str(), newName.c_str());
+}
+
 static void save_photos(struct membuffer *b, const char *photos_dir, const struct dive *dive)
 {
-	if (dive->pictures.nr <= 0)
+	if (dive->pictures.empty())
 		return;
 
 	const char *separator = "\"photos\":[";
-	FOR_EACH_PICTURE(dive) {
+	for (auto &picture: dive->pictures) {
 		put_string(b, separator);
 		separator = ", ";
 		std::string fname = get_file_name(local_file_path(picture));

core/save-xml.cpp

@@ -456,13 +456,13 @@ static void save_dc(struct membuffer *b, struct dive *dive, struct divecomputer
 	put_format(b, "  </divecomputer>\n");
 }
 
-static void save_picture(struct membuffer *b, struct picture *pic)
+static void save_picture(struct membuffer *b, const struct picture &pic)
 {
 	put_string(b, "  <picture filename='");
-	put_quoted(b, pic->filename, true, false);
+	put_quoted(b, pic.filename.c_str(), true, false);
 	put_string(b, "'");
-	if (pic->offset.seconds) {
+	if (pic.offset.seconds) {
-		int offset = pic->offset.seconds;
+		int offset = pic.offset.seconds;
 		char sign = '+';
 		if (offset < 0) {
 			sign = '-';
@@ -470,7 +470,7 @@ static void save_picture(struct membuffer *b, struct picture *pic)
 		}
 		put_format(b, " offset='%c%u:%02u min'", sign, FRACTION_TUPLE(offset, 60));
 	}
-	put_location(b, &pic->location, " gps='","'");
+	put_location(b, &pic.location, " gps='","'");
 
 	put_string(b, "/>\n");
 }
@@ -528,7 +528,7 @@ void save_one_dive_to_mb(struct membuffer *b, struct dive *dive, bool anonymize)
 	/* Save the dive computer data */
 	for (auto &dc: dive->dcs)
 		save_dc(b, dive, &dc);
-	FOR_EACH_PICTURE(dive)
+	for (auto &picture: dive->pictures)
 		save_picture(b, picture);
 	put_format(b, "</dive>\n");
 }

core/subsurface-qt/divelistnotifier.h

@@ -6,7 +6,6 @@
 #define DIVELISTNOTIFIER_H
 
 #include "core/dive.h"
-#include "core/pictureobj.h"
 
 #include <QObject>
 
@@ -133,7 +132,7 @@ 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);
+	void picturesAdded(dive *d, QVector<picture> pics);
 
 	// Devices related signals
 	void deviceEdited();

desktop-widgets/divelistview.cpp

@@ -835,18 +835,15 @@ void DiveListView::matchImagesToDives(const QStringList &fileNames)
 	// 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;
+		auto [pic, d] = create_picture(fileName.toStdString(), shiftDialog.amount(), shiftDialog.matchAll());
-		picture *pic = create_picture(qPrintable(fileName), shiftDialog.amount(), shiftDialog.matchAll(), &d);
 		if (!pic)
 			continue;
-		PictureObj pObj(*pic);
-		free(pic);
 
-		auto it = std::find_if(pics.begin(), pics.end(), [d](const Command::PictureListForAddition &l) { return l.d == d; });
+		auto it = std::find_if(pics.begin(), pics.end(), [dive=d](const Command::PictureListForAddition &l) { return l.d == dive; });
 		if (it == pics.end())
-			pics.push_back(Command::PictureListForAddition { d, { std::move(pObj) } });
+			pics.push_back(Command::PictureListForAddition { d, { std::move(*pic) } });
 		else
-			it->pics.push_back(std::move(pObj));
+			it->pics.push_back(std::move(*pic));
 	}
 
 	if (pics.empty())

desktop-widgets/findmovedimagesdialog.cpp

@@ -188,8 +188,8 @@ void FindMovedImagesDialog::on_scanButton_clicked()
 	struct dive *dive;
 	for_each_dive (i, dive)
 		if (!onlySelected || dive->selected)
-			FOR_EACH_PICTURE(dive)
+			for (auto &picture: dive->pictures)
-				imagePaths.append(QString(picture->filename));
+				imagePaths.append(QString::fromStdString(picture.filename));
 	stopScanning = 0;
 	QFuture<QVector<Match>> future = QtConcurrent::run(
 			// Note that we capture everything but "this" by copy to avoid dangling references.

profile-widget/profilewidget2.cpp

@@ -1077,8 +1077,10 @@ void ProfileWidget2::updateDurationLine(PictureEntry &e)
 
 // This function is called asynchronously by the thumbnailer if a thumbnail
 // was fetched from disk or freshly calculated.
-void ProfileWidget2::updateThumbnail(QString filename, QImage thumbnail, duration_t duration)
+void ProfileWidget2::updateThumbnail(QString filenameIn, QImage thumbnail, duration_t duration)
 {
+	std::string filename = filenameIn.toStdString();
+
 	// Find the picture with the given filename
 	auto it = std::find_if(pictures.begin(), pictures.end(), [&filename](const PictureEntry &e)
 			       { return e.filename == filename; });
@@ -1101,7 +1103,7 @@ void ProfileWidget2::updateThumbnail(QString filename, QImage thumbnail, duratio
 }
 
 // Create a PictureEntry object and add its thumbnail to the scene if profile pictures are shown.
-ProfileWidget2::PictureEntry::PictureEntry(offset_t offsetIn, const QString &filenameIn, ProfileWidget2 *profile, bool synchronous) : offset(offsetIn),
+ProfileWidget2::PictureEntry::PictureEntry(offset_t offsetIn, const std::string &filenameIn, ProfileWidget2 *profile, bool synchronous) : offset(offsetIn),
 	duration(duration_t {0}),
 	filename(filenameIn),
 	thumbnail(new DivePictureItem)
@@ -1110,9 +1112,9 @@ ProfileWidget2::PictureEntry::PictureEntry(offset_t offsetIn, const QString &fil
 	int size = Thumbnailer::defaultThumbnailSize();
 	scene->addItem(thumbnail.get());
 	thumbnail->setVisible(prefs.show_pictures_in_profile);
-	QImage img = Thumbnailer::instance()->fetchThumbnail(filename, synchronous).scaled(size, size, Qt::KeepAspectRatio);
+	QImage img = Thumbnailer::instance()->fetchThumbnail(QString::fromStdString(filename), synchronous).scaled(size, size, Qt::KeepAspectRatio);
 	thumbnail->setPixmap(QPixmap::fromImage(img));
-	thumbnail->setFileUrl(filename);
+	thumbnail->setFileUrl(QString::fromStdString(filename));
 	connect(thumbnail.get(), &DivePictureItem::removePicture, profile, &ProfileWidget2::removePicture);
 }
 
@@ -1205,13 +1207,15 @@ void ProfileWidget2::plotPicturesInternal(const struct dive *d, bool synchronous
 	if (currentState == EDIT || currentState == PLAN)
 		return;
 
+	if (!d)
+		return;
+
 	// Fetch all pictures of the dive, but consider only those that are within the dive time.
 	// For each picture, create a PictureEntry object in the pictures-vector.
 	// emplace_back() constructs an object at the end of the vector. The parameters are passed directly to the constructor.
-	// Note that FOR_EACH_PICTURE handles d being null gracefully.
+	for (auto &picture: d->pictures) {
-	FOR_EACH_PICTURE(d) {
+		if (picture.offset.seconds > 0 && picture.offset.seconds <= d->duration.seconds)
-		if (picture->offset.seconds > 0 && picture->offset.seconds <= d->duration.seconds)
+			pictures.emplace_back(picture.offset, picture.filename, this, synchronous);
-			pictures.emplace_back(picture->offset, QString(picture->filename), this, synchronous);
 	}
 	if (pictures.empty())
 		return;
@@ -1240,16 +1244,16 @@ void ProfileWidget2::picturesRemoved(dive *d, QVector<QString> fileUrls)
 	// (c.f. erase-remove idiom: https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom)
 	auto it = std::remove_if(pictures.begin(), pictures.end(), [&fileUrls](const PictureEntry &e)
 			// Check whether filename of entry is in list of provided filenames
-			{ return std::find(fileUrls.begin(), fileUrls.end(), e.filename) != fileUrls.end(); });
+			{ return std::find(fileUrls.begin(), fileUrls.end(), QString::fromStdString(e.filename)) != fileUrls.end(); });
 	pictures.erase(it, pictures.end());
 	calculatePictureYPositions();
 }
 
-void ProfileWidget2::picturesAdded(dive *d, QVector<PictureObj> pics)
+void ProfileWidget2::picturesAdded(dive *d, QVector<picture> pics)
 {
-	for (const PictureObj &pic: pics) {
+	for (const picture &pic: pics) {
 		if (pic.offset.seconds > 0 && pic.offset.seconds <= d->duration.seconds) {
-			pictures.emplace_back(pic.offset, QString::fromStdString(pic.filename), this, false);
+			pictures.emplace_back(pic.offset, pic.filename, this, false);
 			updateThumbnailXPos(pictures.back());
 		}
 	}
@@ -1302,11 +1306,13 @@ void ProfileWidget2::dropEvent(QDropEvent *event)
 }
 
 #ifndef SUBSURFACE_MOBILE
-void ProfileWidget2::pictureOffsetChanged(dive *dIn, QString filename, offset_t offset)
+void ProfileWidget2::pictureOffsetChanged(dive *dIn, QString filenameIn, offset_t offset)
 {
 	if (dIn != d)
 		return; // Picture of a different dive than the one shown changed.
 
+	std::string filename = filenameIn.toStdString(); // TODO: can we move std::string through Qt's signal/slot system?
+
 	// Calculate time in dive where picture was dropped and whether the new position is during the dive.
 	bool duringDive = d && offset.seconds > 0 && offset.seconds < d->duration.seconds;
 

profile-widget/profilewidget2.h

@@ -17,7 +17,6 @@
 //  * It needs to be dynamic, things should *flow* on it, not just appear / disappear.
 //  */
 #include "profile-widget/divelineitem.h"
-#include "core/pictureobj.h"
 #include "core/units.h"
 #include "core/subsurface-qt/divelistnotifier.h"
 
@@ -78,7 +77,7 @@ slots: // Necessary to call from QAction's signals.
 #ifndef SUBSURFACE_MOBILE
 	void plotPictures();
 	void picturesRemoved(dive *d, QVector<QString> filenames);
-	void picturesAdded(dive *d, QVector<PictureObj> pics);
+	void picturesAdded(dive *d, QVector<picture> pics);
 	void pointsReset();
 	void pointInserted(const QModelIndex &parent, int start, int end);
 	void pointsRemoved(const QModelIndex &, int start, int end);
@@ -163,11 +162,11 @@ private:
 	struct PictureEntry {
 		offset_t offset;
 		duration_t duration;
-		QString filename;
+		std::string filename;
 		std::unique_ptr<DivePictureItem> thumbnail;
 		// For videos with known duration, we represent the duration of the video by a line
 		std::unique_ptr<QGraphicsRectItem> durationLine;
-		PictureEntry (offset_t offsetIn, const QString &filenameIn, ProfileWidget2 *profile, bool synchronous);
+		PictureEntry (offset_t offsetIn, const std::string &filenameIn, ProfileWidget2 *profile, bool synchronous);
 		bool operator< (const PictureEntry &e) const;
 	};
 	void updateThumbnailXPos(PictureEntry &e);

qt-models/divepicturemodel.cpp

@@ -13,13 +13,6 @@
 #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),
@@ -91,8 +84,8 @@ void DivePictureModel::updateDivePictures()
 
 	for (struct dive *dive: getDiveSelection()) {
 		size_t first = pictures.size();
-		FOR_EACH_PICTURE(dive)
+		for (auto &picture: dive->pictures)
-			pictures.push_back(PictureEntry(dive, *picture));
+			pictures.push_back(PictureEntry(dive, picture));
 
 		// Sort pictures of this dive by offset.
 		// Thus, the list will be sorted by (dive, offset).
@@ -197,7 +190,7 @@ void DivePictureModel::picturesRemoved(dive *d, QVector<QString> filenamesIn)
 }
 
 // Assumes that pics is sorted!
-void DivePictureModel::picturesAdded(dive *d, QVector<PictureObj> picsIn)
+void DivePictureModel::picturesAdded(dive *d, QVector<picture> picsIn)
 {
 	// We only display pictures of selected dives
 	if (!d->selected || picsIn.empty())
@@ -206,7 +199,7 @@ void DivePictureModel::picturesAdded(dive *d, QVector<PictureObj> picsIn)
 	// Convert the picture-data into our own format
 	std::vector<PictureEntry> pics;
 	pics.reserve(picsIn.size());
-	for (const PictureObj &pic: picsIn)
+	for (const picture &pic: picsIn)
 		pics.push_back(PictureEntry(d, pic));
 
 	// Insert batch-wise to avoid too many reloads

qt-models/divepicturemodel.h

@@ -2,8 +2,7 @@
 #ifndef DIVEPICTUREMODEL_H
 #define DIVEPICTUREMODEL_H
 
-#include "core/units.h"
+#include "core/picture.h"
-#include "core/pictureobj.h"
 
 #include <QAbstractTableModel>
 #include <QImage>
@@ -17,7 +16,6 @@ struct PictureEntry {
 	QImage image;
 	int offsetSeconds;
 	duration_t length;
-	PictureEntry(dive *, const PictureObj &);
 	PictureEntry(dive *, const picture &);
 	bool operator<(const PictureEntry &) const;
 };
@@ -36,7 +34,7 @@ public slots:
 	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);
+	void picturesAdded(dive *d, QVector<picture> pics);
 private:
 	DivePictureModel();
 	std::vector<PictureEntry> pictures;

qt-models/divetripmodel.cpp

@@ -147,8 +147,8 @@ static int countPhotos(const struct dive *d)
 	const int bufperiod = 120; // A 2-min buffer period. Photos within 2 min of dive are assumed as
 	int diveTotaltime = dive_endtime(d) - d->when;	// taken during the dive, not before/after.
 	int pic_offset, icon_index = 0;
-	FOR_EACH_PICTURE (d) {			// Step through each of the pictures for this dive:
+	for (auto &picture: d->pictures) {	// Step through each of the pictures for this dive:
-		pic_offset = picture->offset.seconds;
+		pic_offset = picture.offset.seconds;
 		if  ((pic_offset < -bufperiod) | (pic_offset > diveTotaltime+bufperiod)) {
 			icon_index |= 0x02;	// If picture is before/after the dive
 						//  then set the appropriate bit ...
@@ -378,7 +378,7 @@ QVariant DiveTripModelBase::diveData(const struct dive *d, int column, int role)
 		case PHOTOS:
 			// If there are photos, show one of the three photo icons: fish= photos during dive;
 			// sun=photos before/after dive; sun+fish=photos during dive as well as before/after
-			if (d->pictures.nr > 0)
+			if (!d->pictures.empty())
 				return getPhotoIcon(countPhotos(d));
 			break;
 		}

tests/testpicture.cpp

@@ -24,45 +24,43 @@ void TestPicture::initTestCase()
 
 void TestPicture::addPicture()
 {
-	struct dive *dive, *dive1, *dive2;
-	struct picture *pic1, *pic2;
 	verbose = 1;
 
 	QCOMPARE(parse_file(SUBSURFACE_TEST_DATA "/dives/test44.xml", &divelog), 0);
-	dive = get_dive(0);
+	struct dive *dive = get_dive(0);
 	// Pictures will be added to selected dives
 	dive->selected = true;
 	QVERIFY(dive != NULL);
 	// So far no picture in dive
-	QVERIFY(dive->pictures.nr == 0);
+	QVERIFY(dive->pictures.size() == 0);
 
-	pic1 = create_picture(SUBSURFACE_TEST_DATA "/dives/images/wreck.jpg", 0, false, &dive1);
+	{
-	pic2 = create_picture(SUBSURFACE_TEST_DATA "/dives/images/data_after_EOI.jpg", 0, false, &dive2);
+		auto [pic1, dive1] = create_picture(SUBSURFACE_TEST_DATA "/dives/images/wreck.jpg", 0, false);
-	QVERIFY(pic1 != NULL);
+		auto [pic2, dive2] = create_picture(SUBSURFACE_TEST_DATA "/dives/images/data_after_EOI.jpg", 0, false);
-	QVERIFY(pic2 != NULL);
+		QVERIFY(pic1);
-	QVERIFY(dive1 == dive);
+		QVERIFY(pic2);
-	QVERIFY(dive2 == dive);
+		QVERIFY(dive1 == dive);
+		QVERIFY(dive2 == dive);
 
-	add_picture(&dive->pictures, *pic1);
+		add_picture(dive->pictures, std::move(*pic1));
-	add_picture(&dive->pictures, *pic2);
+		add_picture(dive->pictures, std::move(*pic2));
-	free(pic1);
+	}
-	free(pic2);
 
 	// Now there are two pictures
-	QVERIFY(dive->pictures.nr == 2);
+	QVERIFY(dive->pictures.size() == 2);
-	pic1 = &dive->pictures.pictures[0];
+	const picture &pic1 = dive->pictures[0];
-	pic2 = &dive->pictures.pictures[1];
+	const picture &pic2 = dive->pictures[1];
 	// 1st appearing at time 21:01
 	// 2nd appearing at time 22:01
-	QVERIFY(pic1->offset.seconds == 1261);
+	QVERIFY(pic1.offset.seconds == 1261);
-	QVERIFY(pic1->location.lat.udeg == 47934500);
+	QVERIFY(pic1.location.lat.udeg == 47934500);
-	QVERIFY(pic1->location.lon.udeg == 11334500);
+	QVERIFY(pic1.location.lon.udeg == 11334500);
-	QVERIFY(pic2->offset.seconds == 1321);
+	QVERIFY(pic2.offset.seconds == 1321);
 
-	learnPictureFilename(pic1->filename, PIC1_NAME);
+	learnPictureFilename(QString::fromStdString(pic1.filename), PIC1_NAME);
-	learnPictureFilename(pic2->filename, PIC2_NAME);
+	learnPictureFilename(QString::fromStdString(pic2.filename), PIC2_NAME);
-	QCOMPARE(localFilePath(pic1->filename), QString(PIC1_NAME));
+	QCOMPARE(localFilePath(QString::fromStdString(pic1.filename)), QString(PIC1_NAME));
-	QCOMPARE(localFilePath(pic2->filename), QString(PIC2_NAME));
+	QCOMPARE(localFilePath(QString::fromStdString(pic2.filename)), QString(PIC2_NAME));
 }
 
 QTEST_GUILESS_MAIN(TestPicture)