Change taglist_get_tagstring to support 'unlimited' tag list size

Previous taglist_get_tagstring signature/implementation did not allow
handling of cases where inputted buffer could not contain all tags.
New implementation allocates buffer based on pre-computed size allowing to
insert all tags in the returned string.

Added get_taglist_string in qthelper to handle conversion to QString
Added TestTagList with tests for taglist_get_tagstring

Signed-off-by: Jeremie Guichard <djebrest@gmail.com>

10 files changed, 140 insertions, 39 deletions

diff --git a/core/dive.c b/core/dive.c
index 1a26ac5d7..811c0270c 100644
--- a/core/dive.c
+++ b/core/dive.c
@@ -12,6 +12,7 @@
 #include "divelist.h"
 #include "qthelper.h"
 #include "metadata.h"
+#include "membuffer.h"
 
 /* one could argue about the best place to have this variable -
  * it's used in the UI, but it seems to make the most sense to have it
@@ -3020,30 +3021,28 @@ void taglist_cleanup(struct tag_entry **tag_list)
 	}
 }
 
-int taglist_get_tagstring(struct tag_entry *tag_list, char *buffer, int len)
+char *taglist_get_tagstring(struct tag_entry *tag_list)
 {
-	int i = 0;
-	struct tag_entry *tmp;
-	tmp = tag_list;
-	memset(buffer, 0, len);
+	bool first_tag = true;
+	struct membuffer b = { 0 };
+	struct tag_entry *tmp = tag_list;
 	while (tmp != NULL) {
-		int newlength = strlen(tmp->tag->name);
-		if (i > 0)
-			newlength += 2;
-		if ((i + newlength) < len) {
-			if (i > 0) {
-				strcpy(buffer + i, ", ");
-				strcpy(buffer + i + 2, tmp->tag->name);
+		if (!empty_string(tmp->tag->name)) {
+			if (first_tag) {
+				put_format(&b, "%s", tmp->tag->name);
+				first_tag = false;
 			} else {
-				strcpy(buffer, tmp->tag->name);
+				put_format(&b, ", %s", tmp->tag->name);
 			}
-		} else {
-			return i;
 		}
-		i += newlength;
 		tmp = tmp->next;
 	}
-	return i;
+	/* Ensures we do return null terminated empty string for:
+	 *  - empty tag list
+	 *  - tag list with empty tag only
+	 */
+	mb_cstring(&b);
+	return detach_buffer(&b);
 }
 
 static inline void taglist_free_divetag(struct divetag *tag)
diff --git a/core/dive.h b/core/dive.h
index 62fe31917..ca0325192 100644
--- a/core/dive.h
+++ b/core/dive.h
@@ -264,10 +264,13 @@ struct tag_entry *taglist_added(struct tag_entry *original_list, struct tag_entr
 void dump_taglist(const char *intro, struct tag_entry *tl);
 
 /*
- * Writes all divetags in tag_list to buffer, limited by the buffer's (len)gth.
- * Returns the characters written
+ * Writes all divetags form tag_list into internally allocated buffer
+ * Function returns pointer to allocated buffer
+ * Buffer contains comma separated list of tags names or null terminated string
+ *
+ * NOTE! The returned buffer must be freed once used.
  */
-int taglist_get_tagstring(struct tag_entry *tag_list, char *buffer, int len);
+char *taglist_get_tagstring(struct tag_entry *tag_list);
 
 /* cleans up a list: removes empty tags and duplicates */
 void taglist_cleanup(struct tag_entry **tag_list);
diff --git a/core/qthelper.cpp b/core/qthelper.cpp
index 1cef38184..d63ee3568 100644
--- a/core/qthelper.cpp
+++ b/core/qthelper.cpp
@@ -1282,6 +1282,14 @@ QString get_divepoint_gas_string(struct dive *d, const divedatapoint &p)
 	return get_gas_string(d->cylinder[idx].gasmix);
 }
 
+QString get_taglist_string(struct tag_entry *tag_list)
+{
+	char *buffer = taglist_get_tagstring(tag_list);
+	QString ret = QString::fromUtf8(buffer);
+	free(buffer);
+	return ret;
+}
+
 weight_t string_to_weight(const char *str)
 {
 	const char *end;
diff --git a/core/qthelper.h b/core/qthelper.h
index 978493e1c..c699c5653 100644
--- a/core/qthelper.h
+++ b/core/qthelper.h
@@ -30,6 +30,7 @@ bool gpsHasChanged(struct dive *dive, struct dive *master, const QString &gps_te
 QList<int> getDivesInTrip(dive_trip_t *trip);
 QString get_gas_string(struct gasmix gas);
 QString get_divepoint_gas_string(struct dive *d, const divedatapoint& dp);
+QString get_taglist_string(struct tag_entry *tag_list);
 void read_hashes();
 void write_hashes();
 void updateHash(struct picture *picture);
diff --git a/core/subsurface-qt/DiveObjectHelper.cpp b/core/subsurface-qt/DiveObjectHelper.cpp
index 093451ad3..4c1c66e21 100644
--- a/core/subsurface-qt/DiveObjectHelper.cpp
+++ b/core/subsurface-qt/DiveObjectHelper.cpp
@@ -192,9 +192,7 @@ QString DiveObjectHelper::notes() const
 
 QString DiveObjectHelper::tags() const
 {
-	static char buffer[256];
-	taglist_get_tagstring(m_dive->tag_list, buffer, 256);
-	return QString(buffer);
+	return get_taglist_string(m_dive->tag_list);
 }
 
 QString DiveObjectHelper::gas() const
diff --git a/desktop-widgets/tab-widgets/maintab.cpp b/desktop-widgets/tab-widgets/maintab.cpp
index 82e9b8abc..4f80454e8 100644
--- a/desktop-widgets/tab-widgets/maintab.cpp
+++ b/desktop-widgets/tab-widgets/maintab.cpp
@@ -417,7 +417,6 @@ void MainTab::updateDiveInfo(bool clear)
 	// for the trip in the Info tab, otherwise we show the info of the
 	// selected_dive
 	struct dive *prevd;
-	char buf[1024];
 
 	process_selected_dives();
 	process_all_dives(&displayed_dive, &prevd);
@@ -570,8 +569,7 @@ void MainTab::updateDiveInfo(bool clear)
 			ui.equipmentTab->setEnabled(true);
 			cylindersModel->updateDive();
 			weightModel->updateDive();
-			taglist_get_tagstring(displayed_dive.tag_list, buf, 1024);
-			ui.tagWidget->setText(QString(buf));
+			ui.tagWidget->setText(get_taglist_string(displayed_dive.tag_list));
 			if (current_dive) {
 				bool isManual = same_string(current_dive->dc.model, "manually added dive");
 				ui.depth->setVisible(isManual);
@@ -1347,13 +1345,10 @@ void MainTab::diffTaggedStrings(QString currentString, QString displayedString,
 
 void MainTab::on_tagWidget_textChanged()
 {
-	char buf[1024];
-
 	if (editMode == IGNORE || acceptingEdit == true)
 		return;
 
-	taglist_get_tagstring(displayed_dive.tag_list, buf, 1024);
-	if (same_string(buf, qPrintable(ui.tagWidget->toPlainText())))
+	if (get_taglist_string(displayed_dive.tag_list) == ui.tagWidget->toPlainText())
 		return;
 
 	markChangedWidget(ui.tagWidget);
@@ -1524,9 +1519,7 @@ void MainTab::showAndTriggerEditSelective(struct dive_components what)
 	if (what.divesite)
 		ui.location->setCurrentDiveSiteUuid(displayed_dive.dive_site_uuid);
 	if (what.tags) {
-		char buf[1024];
-		taglist_get_tagstring(displayed_dive.tag_list, buf, 1024);
-		ui.tagWidget->setText(QString(buf));
+		ui.tagWidget->setText(get_taglist_string(displayed_dive.tag_list));
 	}
 	if (what.cylinders) {
 		cylindersModel->updateDive();
diff --git a/qt-models/divetripmodel.cpp b/qt-models/divetripmodel.cpp
index 082d3212e..7df52c94a 100644
--- a/qt-models/divetripmodel.cpp
+++ b/qt-models/divetripmodel.cpp
@@ -422,12 +422,7 @@ QString DiveItem::displayWeightWithUnit() const
 QString DiveItem::displayTags() const
 {
 	struct dive *dive = get_dive_by_uniq_id(diveId);
-	if (!dive->tag_list)
-		return QString();
-
-	char buf[1024];
-	taglist_get_tagstring(dive->tag_list, buf, 1024);
-	return QString(buf);
+	return get_taglist_string(dive->tag_list);
 }
 
 int DiveItem::weight() const
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 2efacd706..a9c65bd09 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -87,6 +87,7 @@ TEST(TestGitStorage testgitstorage.cpp)
 TEST(TestPreferences testpreferences.cpp)
 TEST(TestPicture testpicture.cpp)
 TEST(TestMerge testmerge.cpp)
+TEST(TestTagList testtaglist.cpp)
 
 
 add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND}
@@ -102,6 +103,7 @@ add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND}
 	TestRenumber
 	TestPicture
 	TestMerge
+	TestTagList
 )
 
 # useful for debugging CMake issues
diff --git a/tests/testtaglist.cpp b/tests/testtaglist.cpp
new file mode 100644
index 000000000..afe5049f1
--- /dev/null
+++ b/tests/testtaglist.cpp
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "testtaglist.h"
+#include "core/dive.h"
+
+void TestTagList::initTestCase()
+{
+	taglist_init_global();
+}
+
+void TestTagList::cleanupTestCase()
+{
+	taglist_free(g_tag_list);
+	g_tag_list = NULL;
+}
+
+void TestTagList::testGetTagstringNoTags()
+{
+	struct tag_entry *tag_list = NULL;
+	char *tagstring = taglist_get_tagstring(tag_list);
+	QVERIFY(tagstring != NULL);
+	QCOMPARE(*tagstring, '\0');
+}
+
+void TestTagList::testGetTagstringSingleTag()
+{
+	struct tag_entry *tag_list = NULL;
+	taglist_add_tag(&tag_list, "A new tag");
+	char *tagstring = taglist_get_tagstring(tag_list);
+	QVERIFY(tagstring != NULL);
+	QCOMPARE(QString::fromUtf8(tagstring), QString::fromUtf8("A new tag"));
+	free(tagstring);
+}
+
+void TestTagList::testGetTagstringMultipleTags()
+{
+	struct tag_entry *tag_list = NULL;
+	taglist_add_tag(&tag_list, "A new tag");
+	taglist_add_tag(&tag_list, "A new tag 1");
+	taglist_add_tag(&tag_list, "A new tag 2");
+	taglist_add_tag(&tag_list, "A new tag 3");
+	taglist_add_tag(&tag_list, "A new tag 4");
+	taglist_add_tag(&tag_list, "A new tag 5");
+	char *tagstring = taglist_get_tagstring(tag_list);
+	QVERIFY(tagstring != NULL);
+	QCOMPARE(QString::fromUtf8(tagstring),
+		 QString::fromUtf8(
+			 "A new tag, "
+			 "A new tag 1, "
+			 "A new tag 2, "
+			 "A new tag 3, "
+			 "A new tag 4, "
+			 "A new tag 5"));
+	free(tagstring);
+}
+
+void TestTagList::testGetTagstringWithAnEmptyTag()
+{
+	struct tag_entry *tag_list = NULL;
+	taglist_add_tag(&tag_list, "A new tag");
+	taglist_add_tag(&tag_list, "A new tag 1");
+	taglist_add_tag(&tag_list, "");
+	char *tagstring = taglist_get_tagstring(tag_list);
+	QVERIFY(tagstring != NULL);
+	QCOMPARE(QString::fromUtf8(tagstring),
+		 QString::fromUtf8(
+			 "A new tag, "
+			 "A new tag 1"));
+	free(tagstring);
+}
+
+void TestTagList::testGetTagstringEmptyTagOnly()
+{
+	struct tag_entry *tag_list = NULL;
+	taglist_add_tag(&tag_list, "");
+	char *tagstring = taglist_get_tagstring(tag_list);
+	QVERIFY(tagstring != NULL);
+	QCOMPARE(QString::fromUtf8(tagstring),
+		 QString::fromUtf8(""));
+	free(tagstring);
+}
+
+QTEST_GUILESS_MAIN(TestTagList)
diff --git a/tests/testtaglist.h b/tests/testtaglist.h
new file mode 100644
index 000000000..7e6e94742
--- /dev/null
+++ b/tests/testtaglist.h
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef TESTTAGLIST_H
+#define TESTTAGLIST_H
+
+#include <QtTest>
+
+class TestTagList : public QObject {
+	Q_OBJECT
+private slots:
+	void initTestCase();
+	void cleanupTestCase();
+
+	void testGetTagstringNoTags();
+	void testGetTagstringSingleTag();
+	void testGetTagstringMultipleTags();
+	void testGetTagstringWithAnEmptyTag();
+	void testGetTagstringEmptyTagOnly();
+};
+
+#endif