More tolerant when parsing GPS coordinates.

Refactored the parsing logic to make it more solid (no more guessing) and
more flexible (support more formats).
Added a test for checking that.
Fixed a few warnings.

[Dirk Hohndel: some changes to coding style]

Signed-off-by: Patrick Valsecchi <patrick@thus.ch>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

CMakeLists.txt

@@ -235,11 +235,13 @@ ADD_DEPENDENCIES(subsurface_interface subsurface_generated_ui)
 ADD_DEPENDENCIES(subsurface_generated_ui version)
 ADD_DEPENDENCIES(subsurface_corelib version)
 
-ENABLE_TESTING()
+MACRO(test NAME FILE)
-ADD_EXECUTABLE(TestUnitConversion tests/testunitconversion.cpp)
+    ADD_EXECUTABLE(${NAME} tests/${FILE})
-TARGET_LINK_LIBRARIES(TestUnitConversion subsurface_corelib ${QT_TEST_LIBRARIES}  ${SUBSURFACE_LINK_LIBRARIES} -lzip -ldivecomputer)
+    TARGET_LINK_LIBRARIES(${NAME} subsurface_corelib ${QT_TEST_LIBRARIES}  ${SUBSURFACE_LINK_LIBRARIES} -lzip -ldivecomputer)
-ADD_TEST(NAME TestUnitConversion COMMAND TestUnitConversion)
+    ADD_TEST(NAME ${NAME} COMMAND ${NAME})
+ENDMACRO()
 
-ADD_EXECUTABLE(TestProfile tests/testprofile.cpp)
+ENABLE_TESTING()
-TARGET_LINK_LIBRARIES(TestProfile subsurface_corelib ${QT_TEST_LIBRARIES} ${SUBSURFACE_LINK_LIBRARIES} -lzip -ldivecomputer)
+test(TestUnitConversion testunitconversion.cpp)
-ADD_TEST(NAME TestProfile COMMAND TestProfile)
+test(TestProfile testprofile.cpp)
+test(TestGpsCoords testgpscoords.cpp)

Documentation/user-manual.txt

@@ -277,6 +277,9 @@ Southern hemisphere latitudes are given with a *S*, e.g. S30°, or with a
 negative value, e.g. -30.22496. Similarly western longitudes are given with a
 *W*, e.g. W07°, or with a negative value, e.g. -7.34323.
 
+Some keyboards don't have the degree sign (°). It can be replaced by a d like
+that: N30d W20d.
+
 Please note that GPS coordinates of a dive site are linked to the Location
 name - so adding coordinates to dives that do not have a location description
 will cause unexpected behaviour (Subsurface will think that all of these

qthelper.cpp

@@ -1,19 +1,17 @@
 #include "qthelper.h"
-#include "qt-gui.h"
 #include "gettextfromc.h"
-#include "dive.h"
 #include "statistics.h"
 #include <exif.h>
 #include "file.h"
 #include <QFile>
 #include <QRegExp>
 #include <QDir>
-#include <QMap>
 #include <QDebug>
 #include <QSettings>
 #include <libxslt/documents.h>
 
 #define translate(_context, arg) trGettext(arg)
+static const QString DEGREE_SIGNS("dD" UTF8_DEGREE);
 
 QString weight_string(int weight_in_grams)
 {
@@ -47,10 +45,10 @@ QString printGPSCoords(int lat, int lon)
 	lonh = lon >= 0 ? translate("gettextFromC", "E") : translate("gettextFromC", "W");
 	lat = abs(lat);
 	lon = abs(lon);
-	latdeg = lat / 1000000;
+	latdeg = lat / 1000000U;
-	londeg = lon / 1000000;
+	londeg = lon / 1000000U;
-	latmin = (lat % 1000000) * 60;
+	latmin = (lat % 1000000U) * 60U;
-	lonmin = (lon % 1000000) * 60;
+	lonmin = (lon % 1000000U) * 60U;
 	latsec = (latmin % 1000000) * 60;
 	lonsec = (lonmin % 1000000) * 60;
 	result.sprintf("%u%s%02d\'%06.3f\"%s %u%s%02d\'%06.3f\"%s",
@@ -59,111 +57,112 @@ QString printGPSCoords(int lat, int lon)
 	return result;
 }
 
+static bool parseCoord(const QString& txt, int& pos, const QString& positives,
+		       const QString& negatives, const QString& others,
+		       double& value)
+{
+	bool numberDefined = false, degreesDefined = false,
+		minutesDefined = false, secondsDefined = false;
+	double number = 0.0;
+	int posBeforeNumber = pos;
+	int sign = 0;
+	value = 0.0;
+	while (pos < txt.size()) {
+		if (txt[pos].isDigit()) {
+			if (numberDefined)
+				return false;
+			QRegExp numberRe("(\\d+(?:[\\.,]\\d+)?).*");
+			if (!numberRe.exactMatch(txt.mid(pos)))
+				return false;
+			number = numberRe.cap(1).toDouble();
+			numberDefined = true;
+			posBeforeNumber = pos;
+			pos += numberRe.cap(1).size() - 1;
+		} else if (positives.indexOf(txt[pos].toUpper()) >= 0) {
+			if (sign != 0)
+				return false;
+			sign = 1;
+			if (degreesDefined || numberDefined) {
+				//sign after the degrees =>
+				//at the end of the coordinate
+				++pos;
+				break;
+			}
+		} else if (negatives.indexOf(txt[pos].toUpper()) >= 0) {
+			if (sign != 0) {
+				if (others.indexOf(txt[pos]) >= 0)
+					//special case for the '-' sign => next coordinate
+					break;
+				return false;
+			}
+			sign = -1;
+			if (degreesDefined || numberDefined) {
+				//sign after the degrees =>
+				//at the end of the coordinate
+				++pos;
+				break;
+			}
+		} else if (others.indexOf(txt[pos].toUpper()) >= 0) {
+			//we are at the next coordinate.
+			break;
+		} else if (DEGREE_SIGNS.indexOf(txt[pos]) >= 0) {
+			if (!numberDefined)
+				return false;
+			if (degreesDefined) {
+				//next coordinate => need to put back the number
+				pos = posBeforeNumber;
+				numberDefined = false;
+				break;
+			}
+			value += number;
+			numberDefined = false;
+			degreesDefined = true;
+		} else if (txt[pos] == '\'') {
+			if (!numberDefined || minutesDefined)
+				return false;
+			value += number / 60.0;
+			numberDefined = false;
+			minutesDefined = true;
+		} else if (txt[pos] == '"') {
+			if (!numberDefined || secondsDefined)
+				return false;
+			value += number / 3600.0;
+			numberDefined = false;
+			secondsDefined = true;
+		} else if (txt[pos] == ' ' || txt[pos] == '\t') {
+			//ignore spaces
+		} else {
+			return false;
+		}
+		++pos;
+	}
+	if (!degreesDefined && numberDefined) {
+		value = number; //just a single number => degrees
+		numberDefined = false;
+		degreesDefined = true;
+	}
+	if (!degreesDefined || numberDefined)
+		return false;
+	if (sign == -1) value *= -1.0;
+	return true;
+}
+
 bool parseGpsText(const QString &gps_text, double *latitude, double *longitude)
 {
-	enum {
+	const QString trimmed = gps_text.trimmed();
-		ISO6709D,
+	if (trimmed.isEmpty()) {
-		SECONDS,
-		MINUTES,
-		DECIMAL
-	} gpsStyle = ISO6709D;
-	int eastWest = 4;
-	int northSouth = 1;
-	QString trHemisphere[4];
-	trHemisphere[0] = translate("gettextFromC", "N");
-	trHemisphere[1] = translate("gettextFromC", "S");
-	trHemisphere[2] = translate("gettextFromC", "E");
-	trHemisphere[3] = translate("gettextFromC", "W");
-	QString regExp;
-	/* an empty string is interpreted as 0.0,0.0 and therefore "no gps location" */
-	if (gps_text.trimmed().isEmpty()) {
 		*latitude = 0.0;
 		*longitude = 0.0;
 		return true;
 	}
-	// trying to parse all formats in one regexp might be possible, but it seems insane
+	int pos = 0;
-	// so handle the four formats we understand separately
+	static const QString POS_LAT = QString("+N") + translate("gettextFromC", "N");
-
+	static const QString NEG_LAT = QString("-S") + translate("gettextFromC", "S");
-	// ISO 6709 Annex D representation
+	static const QString POS_LON = QString("+E") + translate("gettextFromC", "E");
-	// http://en.wikipedia.org/wiki/ISO_6709#Representation_at_the_human_interface_.28Annex_D.29
+	static const QString NEG_LON = QString("-W") + translate("gettextFromC", "W");
-	// e.g. 52°49'02.388"N 1°36'17.388"E
+	return parseCoord(gps_text, pos, POS_LAT, NEG_LAT, POS_LON + NEG_LON, *latitude) &&
-	if (gps_text.at(0).isDigit() && (gps_text.count(",") % 2) == 0) {
+		parseCoord(gps_text, pos, POS_LON, NEG_LON, "", *longitude) &&
-		gpsStyle = ISO6709D;
+		pos == gps_text.size();
-		regExp = QString("(\\d+)[" UTF8_DEGREE "\\s](\\d+)[\'\\s](\\d+)([,\\.](\\d+))?[\"\\s]([NS%1%2])"
-				 "\\s*(\\d+)[" UTF8_DEGREE "\\s](\\d+)[\'\\s](\\d+)([,\\.](\\d+))?[\"\\s]([EW%3%4])")
-				 .arg(trHemisphere[0])
-				 .arg(trHemisphere[1])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3]);
-	} else if (gps_text.count(QChar('"')) == 2) {
-		gpsStyle = SECONDS;
-		regExp = QString("\\s*([NS%1%2])\\s*(\\d+)[" UTF8_DEGREE "\\s]+(\\d+)[\'\\s]+(\\d+)([,\\.](\\d+))?[^EW%3%4]*"
-				 "([EW%5%6])\\s*(\\d+)[" UTF8_DEGREE "\\s]+(\\d+)[\'\\s]+(\\d+)([,\\.](\\d+))?")
-				 .arg(trHemisphere[0])
-				 .arg(trHemisphere[1])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3]);
-	} else if (gps_text.count(QChar('\'')) == 2) {
-		gpsStyle = MINUTES;
-		regExp = QString("\\s*([NS%1%2])\\s*(\\d+)[" UTF8_DEGREE "\\s]+(\\d+)([,\\.](\\d+))?[^EW%3%4]*"
-				 "([EW%5%6])\\s*(\\d+)[" UTF8_DEGREE "\\s]+(\\d+)([,\\.](\\d+))?")
-				 .arg(trHemisphere[0])
-				 .arg(trHemisphere[1])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3]);
-	} else {
-		gpsStyle = DECIMAL;
-		regExp = QString("\\s*([-NS%1%2]?)\\s*(\\d+)[,\\.](\\d+)[^-EW%3%4\\d]*([-EW%5%6]?)\\s*(\\d+)[,\\.](\\d+)")
-				 .arg(trHemisphere[0])
-				 .arg(trHemisphere[1])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3])
-				 .arg(trHemisphere[2])
-				 .arg(trHemisphere[3]);
-	}
-	QRegExp r(regExp);
-	if (r.indexIn(gps_text) != -1) {
-		// qDebug() << "Hemisphere" << r.cap(1) << "deg" << r.cap(2) << "min" << r.cap(3) << "decimal" << r.cap(4);
-		// qDebug() << "Hemisphere" << r.cap(5) << "deg" << r.cap(6) << "min" << r.cap(7) << "decimal" << r.cap(8);
-		switch (gpsStyle) {
-		case ISO6709D:
-			*latitude = r.cap(1).toInt() + r.cap(2).toInt() / 60.0 +
-				    (r.cap(3) + QString(".") + r.cap(5)).toDouble() / 3600.0;
-			*longitude = r.cap(7).toInt() + r.cap(8).toInt() / 60.0 +
-				     (r.cap(9) + QString(".") + r.cap(11)).toDouble() / 3600.0;
-			northSouth = 6;
-			eastWest = 12;
-			break;
-		case SECONDS:
-			*latitude = r.cap(2).toInt() + r.cap(3).toInt() / 60.0 +
-				    (r.cap(4) + QString(".") + r.cap(6)).toDouble() / 3600.0;
-			*longitude = r.cap(8).toInt() + r.cap(9).toInt() / 60.0 +
-				     (r.cap(10) + QString(".") + r.cap(12)).toDouble() / 3600.0;
-			eastWest = 7;
-			break;
-		case MINUTES:
-			*latitude = r.cap(2).toInt() + (r.cap(3) + QString(".") + r.cap(5)).toDouble() / 60.0;
-			*longitude = r.cap(7).toInt() + (r.cap(8) + QString(".") + r.cap(10)).toDouble() / 60.0;
-			eastWest = 6;
-			break;
-		case DECIMAL:
-		default:
-			*latitude = (r.cap(2) + QString(".") + r.cap(3)).toDouble();
-			*longitude = (r.cap(5) + QString(".") + r.cap(6)).toDouble();
-			break;
-		}
-		if (r.cap(northSouth) == "S" || r.cap(northSouth) == trHemisphere[1] || r.cap(northSouth) == "-")
-			*latitude *= -1.0;
-		if (r.cap(eastWest) == "W" || r.cap(eastWest) == trHemisphere[3] || r.cap(eastWest) == "-")
-			*longitude *= -1.0;
-		// qDebug("%s -> %8.5f / %8.5f", gps_text.toLocal8Bit().data(), *latitude, *longitude);
-		return true;
-	}
-	return false;
 }
 
 bool gpsHasChanged(struct dive *dive, struct dive *master, const QString &gps_text, bool *parsed_out)

tests/testgpscoords.cpp

@@ -0,0 +1,95 @@
+#include <dive.h>
+#include "testgpscoords.h"
+
+//unit under test
+extern bool parseGpsText(const QString &gps_text, double *latitude, double *longitude);
+
+
+void TestGpsCoords::testISO6709DParse()
+{
+	testParseOK("52°49'02.388\"N 1°36'17.388\"E",
+		coord2double(52, 49, 2.388), coord2double(1, 36, 17.388));
+}
+
+void TestGpsCoords::testNegativeISO6709DParse()
+{
+	testParseOK("52°49'02.388\"S 1°36'17.388\"W",
+		coord2double(-52, -49, -2.388), coord2double(-1, -36, -17.388));
+}
+
+void TestGpsCoords::testSpaceISO6709DParse()
+{
+	testParseOK("52 ° 49 ' 02.388 \" N  1 ° 36 ' 17.388 \" E",
+		coord2double(52, 49, 2.388), coord2double(1, 36, 17.388));
+}
+
+void TestGpsCoords::testSecondsParse()
+{
+	testParseOK("N52°49'02.388\" E1°36'17.388\"",
+		coord2double(52, 49, 2.388), coord2double(1, 36, 17.388));
+}
+
+void TestGpsCoords::testSpaceSecondsParse()
+{
+	testParseOK(" N 52 ° 49 ' 02.388 \"  E 1 ° 36 ' 17.388 \"",
+		coord2double(52, 49, 2.388), coord2double(1, 36, 17.388));
+}
+
+void TestGpsCoords::testNegativeSecondsParse()
+{
+	testParseOK("-52°49'02.388\" -1°36'17.388\"",
+		coord2double(-52, -49, -2.388), coord2double(-1, -36, -17.388));
+}
+
+void TestGpsCoords::testMinutesParse()
+{
+	testParseOK("N52°49.03' E1d36.23'",
+		coord2double(52, 49.03), coord2double(1, 36.23));
+}
+
+void TestGpsCoords::testSpaceMinutesParse()
+{
+	testParseOK(" N 52 ° 49.03 '  E 1 ° 36.23 ' ",
+		coord2double(52, 49.03), coord2double(1, 36.23));
+}
+
+void TestGpsCoords::testMinutesInversedParse()
+{
+	testParseOK("2° 53.836' N 73° 32.839' E",
+		coord2double(2, 53.836), coord2double(73, 32.839));
+}
+
+void TestGpsCoords::testDecimalParse()
+{
+	testParseOK("N52.83° E1.61",
+		coord2double(52.83), coord2double(1.61));
+}
+
+void TestGpsCoords::testDecimalInversedParse()
+{
+	testParseOK("52.83N 1.61E",
+		coord2double(52.83), coord2double(1.61));
+}
+
+void TestGpsCoords::testSpaceDecimalParse()
+{
+	testParseOK(" N 52.83  E 1.61 ",
+		coord2double(52.83), coord2double(1.61));
+}
+
+
+void TestGpsCoords::testParseOK(const QString &txt, double expectedLat,
+	double expectedLon)
+{
+	double actualLat, actualLon;
+	QVERIFY(parseGpsText(txt, &actualLat, &actualLon));
+	QCOMPARE(actualLat, expectedLat);
+	QCOMPARE(actualLon, expectedLon);
+}
+
+double TestGpsCoords::coord2double(double deg, double min, double sec)
+{
+	return deg + min / 60.0 + sec / 3600.0;
+}
+
+QTEST_MAIN(TestGpsCoords)

tests/testgpscoords.h

@@ -0,0 +1,28 @@
+#ifndef TESTGPSCOORDS_H
+#define TESTGPSCOORDS_H
+
+#include <QtTest>
+
+class TestGpsCoords : public QObject {
+Q_OBJECT
+private slots:
+	void testISO6709DParse();
+	void testNegativeISO6709DParse();
+	void testSpaceISO6709DParse();
+	void testSecondsParse();
+	void testSpaceSecondsParse();
+	void testNegativeSecondsParse();
+	void testMinutesParse();
+	void testSpaceMinutesParse();
+	void testMinutesInversedParse();
+	void testDecimalParse();
+	void testSpaceDecimalParse();
+	void testDecimalInversedParse();
+
+private:
+	static void testParseOK(const QString &txt, double expectedLat,
+		double expectedLon);
+	static double coord2double(double deg, double min = 0.0, double sec = 0.0);
+};
+
+#endif