Merge pull request #861 from ispyisail/fix/bugtracker-248-split-with-spaces

Partial fix for bugtracker #248: second-instance file arguments are lost

sources/qet.cpp

@@ -546,12 +546,40 @@ QString QET::joinWithSpaces(const QStringList &string_list) {
 QStringList QET::splitWithSpaces(const QString &string) {
 	// les chaines sont separees par des espaces non echappes
 	// = avec un nombre nul ou pair de backslashes devant
-
+	//
-	QStringList escaped_strings = string.split(QRegularExpression("[^\\]?(?:\\\\)* "),Qt::SkipEmptyParts);
+	// This was a QRegularExpression("[^\\]?(?:\\\\)* ") split, which never
-
+	// worked: "[^\]" opens a character class whose "\]" is an escaped
+	// bracket, so the class is never closed and the pattern is invalid.
+	// QRegularExpression::isValid() was false, QString::split() warned
+	// "invalid QRegularExpression object" and returned an EMPTY list for
+	// every input -- so a second instance's file arguments were always
+	// dropped (bugtracker #248), not just ones containing spaces.
+	//
+	// A correct pattern is not expressible here either: the separator is a
+	// space preceded by an even-length run of backslashes, and PCRE2 has no
+	// variable-length lookbehind. Scanning explicitly is both correct and
+	// easier to read than the alternatives.
 	QStringList returned_list;
-	foreach(QString escaped_string, escaped_strings) {
+	QString current;
-		returned_list << QET::unescapeSpaces(escaped_string);
+	int backslashes = 0;
+	for (const QChar &c : string) {
+		if (c == QLatin1Char('\\')) {
+			++backslashes;
+			current += c;
+			continue;
+		}
+		if (c == QLatin1Char(' ') && backslashes % 2 == 0) {
+			if (!current.isEmpty()) {
+				returned_list << QET::unescapeSpaces(current);
+			}
+			current.clear();
+		} else {
+			current += c;
+		}
+		backslashes = 0;
+	}
+	if (!current.isEmpty()) {
+		returned_list << QET::unescapeSpaces(current);
 	}
 	return(returned_list);
 }

tests/qttest/CMakeLists.txt

@@ -100,3 +100,16 @@ add_executable(
 add_test(NAME tst_smart_device COMMAND tst_smart_device)
 target_include_directories(tst_smart_device PRIVATE ${QET_DIR}/sources)
 target_link_libraries(tst_smart_device PRIVATE Qt::Test Qt::Sql)
+
+# qet.cpp carries the SingleApplication argument wire format
+# (joinWithSpaces/splitWithSpaces); it pulls in qeticons and shortcutmanager,
+# so those are compiled alongside rather than linking the whole application.
+add_executable(
+  tst_qetstrings
+  tst_qetstrings.cpp
+  ${QET_DIR}/sources/qet.cpp
+  ${QET_DIR}/sources/qeticons.cpp
+  ${QET_DIR}/sources/shortcutmanager.cpp)
+add_test(NAME tst_qetstrings COMMAND tst_qetstrings)
+target_include_directories(tst_qetstrings PRIVATE ${QET_DIR}/sources)
+target_link_libraries(tst_qetstrings PRIVATE Qt::Test Qt::Widgets Qt::Xml)

tests/qttest/tst_qetstrings.cpp

@@ -0,0 +1,54 @@
+#include <QtTest>
+
+#include "qet.h"
+
+/**
+	QET::joinWithSpaces() / QET::splitWithSpaces() are the wire format for the
+	SingleApplication message a secondary instance sends to the running one
+	(main.cpp: "launched-with-args: " + joinWithSpaces(...), received by
+	QETApp::receiveMessage()). If the round trip loses arguments, opening a
+	file while QET is already running silently does nothing.
+
+	splitWithSpaces() used to split on QRegularExpression("[^\\]?(?:\\\\)* "),
+	which is not a valid pattern: "[^\\]" opens a character class whose "\\]"
+	is an escaped bracket, so the class never closes. QRegularExpression
+	reported isValid() == false and QString::split() returned an empty list for
+	every input -- bugtracker #248.
+*/
+class tst_qetstrings : public QObject
+{
+	Q_OBJECT
+
+private slots:
+	void roundTrips_data()
+	{
+		QTest::addColumn<QStringList>("input");
+
+		QTest::newRow("single plain")        << QStringList{"one.qet"};
+		QTest::newRow("two plain")           << QStringList{"one.qet", "two.qet"};
+		QTest::newRow("space in name")       << QStringList{"my file.qet"};
+		QTest::newRow("space then plain")    << QStringList{"my file.qet", "other.qet"};
+		QTest::newRow("spaces in path")      << QStringList{"/home/a b/c d.qet", "/tmp/x.qet"};
+		QTest::newRow("backslash in name")   << QStringList{"back\\slash.qet"};
+		QTest::newRow("trailing backslash")  << QStringList{"trailing\\"};
+		QTest::newRow("mixed")               << QStringList{"a b", "c\\d", "e"};
+	}
+
+	/// What the IPC actually needs: whatever went in comes back out.
+	void roundTrips()
+	{
+		QFETCH(QStringList, input);
+		QCOMPARE(QET::splitWithSpaces(QET::joinWithSpaces(input)), input);
+	}
+
+	/// The specific regression: the old implementation returned an empty list
+	/// for every input, so this passed nothing on to openFiles().
+	void splitIsNotEmptyForPlainArguments()
+	{
+		QVERIFY(!QET::splitWithSpaces(QStringLiteral("one.qet")).isEmpty());
+		QCOMPARE(QET::splitWithSpaces(QStringLiteral("a.qet b.qet")).count(), 2);
+	}
+};
+
+QTEST_APPLESS_MAIN(tst_qetstrings)
+#include "tst_qetstrings.moc"