[Bug #2934] Geo Lookup - support for remote dive sites

Some remote dive sites have no populated places (towns, cities)
nearby. For such sites, we now fall back to looking up
unpopulated place names, such as the reef or island name.

Also some code refactorisation:
the actual network access is now encapsulated in its own
function removing some duplicated code handling in the
reverseGeoLookup function and making it more readable.

Furthermore, reverseGeoLookup() was completely refactored as
most of its functionality was due to legacy requirements; the
current code-base only calls this function from a single
location and only with an empty taxonomy_data object. This
makes the function more focussed and much simpler and more
readable.

Finally, a resource leak in reverseGeocde introduced in
4f3b26f9b6296273e37ec317bc68f32f94f546dc was fixed.

Signed-off-by: Michael Werle <micha@michaelwerle.com>

5 files changed, 97 insertions, 121 deletions

diff --git a/core/divesitehelpers.cpp b/core/divesitehelpers.cpp
index b2bb400a7..3f3c4be31 100644
--- a/core/divesitehelpers.cpp
+++ b/core/divesitehelpers.cpp
@@ -22,7 +22,8 @@
 #include <QTimer>
 #include <memory>
 
-void reverseGeoLookup(degrees_t latitude, degrees_t longitude, taxonomy_data *taxonomy)
+/** Performs a REST get request to a service returning a JSON object. */
+static QJsonObject doAsyncRESTGetRequest(const QString& url, int msTimeout)
 {
 	// By making the QNetworkAccessManager static and local to this function,
 	// only one manager exists for all geo-lookups and it is only initialized
@@ -30,16 +31,13 @@ void reverseGeoLookup(degrees_t latitude, degrees_t longitude, taxonomy_data *ta
 	static QNetworkAccessManager rgl;
 	QNetworkRequest request;
 	QEventLoop loop;
-	QString geonamesURL("http://api.geonames.org/findNearbyPlaceNameJSON?lang=%1&lat=%2&lng=%3&radius=50&username=dirkhh");
-	QString geonamesOceanURL("http://api.geonames.org/oceanJSON?lang=%1&lat=%2&lng=%3&radius=50&username=dirkhh");
 	QTimer timer;
 
 	request.setRawHeader("Accept", "text/json");
 	request.setRawHeader("User-Agent", getUserAgent().toUtf8());
 	QObject::connect(&timer, SIGNAL(timeout()), &loop, SLOT(quit()));
 
-	// first check the findNearbyPlaces API from geonames - that should give us country, state, city
-	request.setUrl(geonamesURL.arg(getUiLanguage().section(QRegExp("[-_ ]"), 0, 0)).arg(latitude.udeg / 1000000.0).arg(longitude.udeg / 1000000.0));
+	request.setUrl(url);
 
 	// By using a std::unique_ptr<>, we can exit from the function at any point
 	// and the reply will be freed. Likewise, when overwriting the pointer with
@@ -48,119 +46,95 @@ void reverseGeoLookup(degrees_t latitude, degrees_t longitude, taxonomy_data *ta
 	std::unique_ptr<QNetworkReply> reply(rgl.get(request));
 	timer.setSingleShot(true);
 	QObject::connect(&*reply, SIGNAL(finished()), &loop, SLOT(quit()));
-	timer.start(5000);   // 5 secs. timeout
+	timer.start(msTimeout);
 	loop.exec();
 
-	if (timer.isActive()) {
-		timer.stop();
-		if (reply->error() > 0) {
-			report_error("got error accessing geonames.org: %s", qPrintable(reply->errorString()));
-			return;
-		}
-		int v = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-		if (v < 200 || v >= 300)
-			return;
-		QByteArray fullReply = reply->readAll();
-		QJsonParseError errorObject;
-		QJsonDocument jsonDoc = QJsonDocument::fromJson(fullReply, &errorObject);
-		if (errorObject.error != QJsonParseError::NoError) {
-			report_error("error parsing geonames.org response: %s", qPrintable(errorObject.errorString()));
-			return;
-		}
-		QJsonObject obj = jsonDoc.object();
-		QVariant geoNamesObject = obj.value("geonames").toVariant();
-		QVariantList geoNames = geoNamesObject.toList();
-		if (geoNames.count() > 0) {
-			QVariantMap firstData = geoNames.at(0).toMap();
-			int ri = 0, l3 = -1, lt = -1;
-			if (taxonomy->category == NULL) {
-				taxonomy->category = alloc_taxonomy();
-			} else {
-				// clear out the data (except for the ocean data)
-				int ocean;
-				if ((ocean = taxonomy_index_for_category(taxonomy, TC_OCEAN)) > 0) {
-					taxonomy->category[0] = taxonomy->category[ocean];
-					taxonomy->nr = 1;
-				} else {
-					// ocean is -1 if there is no such entry, and we didn't copy above
-					// if ocean is 0, so the following gets us the correct count
-					taxonomy->nr = ocean + 1;
-				}
-			}
-			// get all the data - OCEAN is special, so start at COUNTRY
-			for (int j = TC_COUNTRY; j < TC_NR_CATEGORIES; j++) {
-				if (firstData[taxonomy_api_names[j]].isValid()) {
-					taxonomy->category[ri].category = j;
-					taxonomy->category[ri].origin = taxonomy_origin::GEOCODED;
-					free((void *)taxonomy->category[ri].value);
-					taxonomy->category[ri].value = copy_qstring(firstData[taxonomy_api_names[j]].toString());
-					ri++;
-				}
-			}
-			taxonomy->nr = ri;
-			l3 = taxonomy_index_for_category(taxonomy, TC_ADMIN_L3);
-			lt = taxonomy_index_for_category(taxonomy, TC_LOCALNAME);
-			if (l3 == -1 && lt != -1) {
-				// basically this means we did get a local name (what we call town), but just like most places
-				// we didn't get an adminName_3 - which in some regions is the actual city that town belongs to,
-				// then we copy the town into the city
-				taxonomy->category[ri].value = copy_string(taxonomy->category[lt].value);
-				taxonomy->category[ri].origin = taxonomy_origin::GEOCOPIED;
-				taxonomy->category[ri].category = TC_ADMIN_L3;
-				taxonomy->nr++;
-			}
-		} else {
-			report_error("geonames.org did not provide reverse lookup information");
-			qDebug() << "no reverse geo lookup; geonames returned\n" << fullReply;
-		}
-	} else {
-		report_error("timeout accessing geonames.org");
+	if (!timer.isActive()) {
+		report_error("timeout accessing %s", qPrintable(url));
 		QObject::disconnect(&*reply, SIGNAL(finished()), &loop, SLOT(quit()));
 		reply->abort();
+		return QJsonObject{};
 	}
-	// next check the oceans API to figure out the body of water
-	request.setUrl(geonamesOceanURL.arg(getUiLanguage().section(QRegExp("[-_ ]"), 0, 0)).arg(latitude.udeg / 1000000.0).arg(longitude.udeg / 1000000.0));
-	reply.reset(rgl.get(request)); // Note: frees old reply.
-	QObject::connect(&*reply, SIGNAL(finished()), &loop, SLOT(quit()));
-	timer.start(5000);   // 5 secs. timeout
-	loop.exec();
-	if (timer.isActive()) {
-		timer.stop();
-		if (reply->error() > 0) {
-			report_error("got error accessing oceans API of geonames.org: %s", qPrintable(reply->errorString()));
-			return;
-		}
-		int v = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-		if (v < 200 || v >= 300)
-			return;
-		QByteArray fullReply = reply->readAll();
-		QJsonParseError errorObject;
-		QJsonDocument jsonDoc = QJsonDocument::fromJson(fullReply, &errorObject);
-		if (errorObject.error != QJsonParseError::NoError) {
-			report_error("error parsing geonames.org response: %s", qPrintable(errorObject.errorString()));
-			return;
-		}
-		QJsonObject obj = jsonDoc.object();
-		QVariant oceanObject = obj.value("ocean").toVariant();
-		QVariantMap oceanName = oceanObject.toMap();
-		if (oceanName["name"].isValid()) {
-			int idx;
-			if (taxonomy->category == NULL)
-				taxonomy->category = alloc_taxonomy();
-			idx = taxonomy_index_for_category(taxonomy, TC_OCEAN);
-			if (idx == -1)
-				idx = taxonomy->nr;
-			if (idx < TC_NR_CATEGORIES) {
-				taxonomy->category[idx].category = TC_OCEAN;
-				taxonomy->category[idx].origin = taxonomy_origin::GEOCODED;
-				taxonomy->category[idx].value = copy_qstring(oceanName["name"].toString());
-				if (idx == taxonomy->nr)
-					taxonomy->nr++;
+
+	timer.stop();
+	if (reply->error() > 0) {
+		report_error("got error accessing %s: %s", qPrintable(url), qPrintable(reply->errorString()));
+		return QJsonObject{};
+	}
+	int v = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+	if (v < 200 || v >= 300) {
+		return QJsonObject{};
+	}
+	QByteArray fullReply = reply->readAll();
+	QJsonParseError errorObject;
+	QJsonDocument jsonDoc = QJsonDocument::fromJson(fullReply, &errorObject);
+	if (errorObject.error != QJsonParseError::NoError) {
+		report_error("error parsing JSON response: %s", qPrintable(errorObject.errorString()));
+		return QJsonObject{};
+	}
+	// Success, return JSON response from server
+	return jsonDoc.object();
+}
+
+/// Performs a reverse-geo-lookup of the coordinates and returns the taxonomy data.
+taxonomy_data reverseGeoLookup(degrees_t latitude, degrees_t longitude)
+{
+	const QString geonamesNearbyURL = QStringLiteral("http://api.geonames.org/findNearbyJSON?lang=%1&lat=%2&lng=%3&radius=50&username=dirkhh");
+	const QString geonamesNearbyPlaceNameURL = QStringLiteral("http://api.geonames.org/findNearbyPlaceNameJSON?lang=%1&lat=%2&lng=%3&radius=50&username=dirkhh");
+	const QString geonamesOceanURL = QStringLiteral("http://api.geonames.org/oceanJSON?lang=%1&lat=%2&lng=%3&radius=50&username=dirkhh");
+
+	QString url;
+	QJsonObject obj;
+	taxonomy_data taxonomy = { 0, alloc_taxonomy() };
+
+	// check the oceans API to figure out the body of water
+	url = geonamesOceanURL.arg(getUiLanguage().section(QRegExp("[-_ ]"), 0, 0)).arg(latitude.udeg / 1000000.0).arg(longitude.udeg / 1000000.0);
+	obj = doAsyncRESTGetRequest(url, 5000); // 5 secs. timeout
+	QVariantMap oceanName = obj.value("ocean").toVariant().toMap();
+	if (oceanName["name"].isValid()) {
+		taxonomy.category[0].category = TC_OCEAN;
+		taxonomy.category[0].origin = taxonomy_origin::GEOCODED;
+		taxonomy.category[0].value = copy_qstring(oceanName["name"].toString());
+		taxonomy.nr = 1;
+	}
+
+	// check the findNearbyPlaces API from geonames - that should give us country, state, city
+	url = geonamesNearbyPlaceNameURL.arg(getUiLanguage().section(QRegExp("[-_ ]"), 0, 0)).arg(latitude.udeg / 1000000.0).arg(longitude.udeg / 1000000.0);
+	obj = doAsyncRESTGetRequest(url, 5000); // 5 secs. timeout
+	QVariantList geoNames = obj.value("geonames").toVariant().toList();
+	if (geoNames.count() == 0) {
+		// check the findNearby API from geonames if the previous search came up empty - that should give us country, state, location
+		url = geonamesNearbyURL.arg(getUiLanguage().section(QRegExp("[-_ ]"), 0, 0)).arg(latitude.udeg / 1000000.0).arg(longitude.udeg / 1000000.0);
+		obj = doAsyncRESTGetRequest(url, 5000); // 5 secs. timeout
+		geoNames = obj.value("geonames").toVariant().toList();
+	}
+	if (geoNames.count() > 0) {
+		QVariantMap firstData = geoNames.at(0).toMap();
+
+		// fill out all the data - start at COUNTRY since we already got OCEAN above
+		for (int idx = TC_COUNTRY; idx < TC_NR_CATEGORIES; idx++) {
+			if (firstData[taxonomy_api_names[idx]].isValid()) {
+				taxonomy.category[taxonomy.nr].category = idx;
+				taxonomy.category[taxonomy.nr].origin = taxonomy_origin::GEOCODED;
+				taxonomy.category[taxonomy.nr].value = copy_qstring(firstData[taxonomy_api_names[idx]].toString());
+				taxonomy.nr++;
 			}
 		}
+		int l3 = taxonomy_index_for_category(&taxonomy, TC_ADMIN_L3);
+		int lt = taxonomy_index_for_category(&taxonomy, TC_LOCALNAME);
+		if (l3 == -1 && lt != -1) {
+			// basically this means we did get a local name (what we call town), but just like most places
+			// we didn't get an adminName_3 - which in some regions is the actual city that town belongs to,
+			// then we copy the town into the city
+			taxonomy.category[taxonomy.nr].category = TC_ADMIN_L3;
+			taxonomy.category[taxonomy.nr].origin = taxonomy_origin::GEOCOPIED;
+			taxonomy.category[taxonomy.nr].value = copy_string(taxonomy.category[lt].value);
+			taxonomy.nr++;
+		}
 	} else {
-		report_error("timeout accessing geonames.org");
-		QObject::disconnect(&*reply, SIGNAL(finished()), &loop, SLOT(quit()));
-		reply->abort();
+		report_error("geonames.org did not provide reverse lookup information");
+		//qDebug() << "no reverse geo lookup; geonames returned\n" << fullReply;
 	}
+
+	return taxonomy;
 }
diff --git a/core/divesitehelpers.h b/core/divesitehelpers.h
index a894da60d..1b4895564 100644
--- a/core/divesitehelpers.h
+++ b/core/divesitehelpers.h
@@ -5,8 +5,8 @@
 #include "taxonomy.h"
 #include "units.h"
 
-// Perform a reverse geo-lookup and put data in the provided taxonomy field.
-// Original data with the exception of OCEAN will be overwritten.
-void reverseGeoLookup(degrees_t latitude, degrees_t longitude, taxonomy_data *taxonomy);
+/// Performs a reverse geo-lookup and returns the data.
+/// It is up to the caller to merge the data with any existing data.
+taxonomy_data reverseGeoLookup(degrees_t latitude, degrees_t longitude);
 
 #endif // DIVESITEHELPERS_H
diff --git a/core/taxonomy.c b/core/taxonomy.c
index 9592843f0..1747a78cf 100644
--- a/core/taxonomy.c
+++ b/core/taxonomy.c
@@ -42,7 +42,7 @@ void free_taxonomy(struct taxonomy_data *t)
 	}
 }
 
-void copy_taxonomy(struct taxonomy_data *orig, struct taxonomy_data *copy)
+void copy_taxonomy(const struct taxonomy_data *orig, struct taxonomy_data *copy)
 {
 	if (orig->category == NULL) {
 		free_taxonomy(copy);
@@ -63,7 +63,7 @@ void copy_taxonomy(struct taxonomy_data *orig, struct taxonomy_data *copy)
 	}
 }
 
-int taxonomy_index_for_category(struct taxonomy_data *t, enum taxonomy_category cat)
+int taxonomy_index_for_category(const struct taxonomy_data *t, enum taxonomy_category cat)
 {
 	for (int i = 0; i < t->nr; i++)
 		if (t->category[i].category == cat)
diff --git a/core/taxonomy.h b/core/taxonomy.h
index 5f7f0cf43..3af5160be 100644
--- a/core/taxonomy.h
+++ b/core/taxonomy.h
@@ -41,8 +41,8 @@ struct taxonomy_data {
 
 struct taxonomy *alloc_taxonomy();
 void free_taxonomy(struct taxonomy_data *t);
-void copy_taxonomy(struct taxonomy_data *orig, struct taxonomy_data *copy);
-int taxonomy_index_for_category(struct taxonomy_data *t, enum taxonomy_category cat);
+void copy_taxonomy(const struct taxonomy_data *orig, struct taxonomy_data *copy);
+int taxonomy_index_for_category(const struct taxonomy_data *t, enum taxonomy_category cat);
 const char *taxonomy_get_country(struct taxonomy_data *t);
 void taxonomy_set_country(struct taxonomy_data *t, const char *country, enum taxonomy_origin origin);
 
diff --git a/desktop-widgets/locationinformation.cpp b/desktop-widgets/locationinformation.cpp
index 01a1a4390..780390198 100644
--- a/desktop-widgets/locationinformation.cpp
+++ b/desktop-widgets/locationinformation.cpp
@@ -317,10 +317,12 @@ void LocationInformationWidget::reverseGeocode()
 	location_t location = parseGpsText(ui.diveSiteCoordinates->text());
 	if (!ds || !has_location(&location))
 		return;
-	taxonomy_data taxonomy = { 0, 0 };
-	reverseGeoLookup(location.lat, location.lon, &taxonomy);
-	if (ds != diveSite)
+	taxonomy_data taxonomy = reverseGeoLookup(location.lat, location.lon);
+	if (ds != diveSite) {
+		free_taxonomy(&taxonomy);
 		return;
+	}
+	// This call transfers ownership of the taxonomy memory into an EditDiveSiteTaxonomy object
 	Command::editDiveSiteTaxonomy(ds, taxonomy);
 }