QDeclarativePositionSource: fix start/stop/update logic

The logic of active state updates was broken.
For example, calling update() and then stop() was immediately switching
to inactive state, while the position source was still waiting for a
single position request to complete.
This patch fixes this and some other logic errors and adds several unit
tests to cover different scenarios of calling these methods.

Change-Id: I5cea19c711744a1b57dd0427a41119d789f4033b
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

5 files changed, 243 insertions, 20 deletions

diff --git a/src/positioningquick/qdeclarativepositionsource.cpp b/src/positioningquick/qdeclarativepositionsource.cpp
index 07108dce..9453a15b 100644
--- a/src/positioningquick/qdeclarativepositionsource.cpp
+++ b/src/positioningquick/qdeclarativepositionsource.cpp
@@ -114,8 +114,6 @@ QT_BEGIN_NAMESPACE
 */
 
 QDeclarativePositionSource::QDeclarativePositionSource()
-:   m_positionSource(0), m_preferredPositioningMethods(AllPositioningMethods),
-    m_active(false), m_singleUpdate(false), m_updateInterval(0), m_sourceError(NoError)
 {
 }
 
@@ -257,10 +255,13 @@ void QDeclarativePositionSource::handleUpdateTimeout()
     if (m_singleUpdate) {
         m_singleUpdate = false;
 
-        // only singleUpdate based timeouts change activity
-        // continuous updates may resume again (see QGeoPositionInfoSource::startUpdates())
-        m_active = false;
-        emit activeChanged();
+        if (!m_regularUpdates) {
+            // only singleUpdate based timeouts change activity
+            // continuous updates may resume again
+            // (see QGeoPositionInfoSource::startUpdates())
+            m_active = false;
+            emit activeChanged();
+        }
     }
 }
 
@@ -446,12 +447,14 @@ QDeclarativePositionSource::PositioningMethods QDeclarativePositionSource::prefe
 
 void QDeclarativePositionSource::start()
 {
-    if (m_positionSource)
+    if (m_positionSource) {
         m_positionSource->startUpdates();
 
-    if (!m_active) {
-        m_active = true;
-        emit activeChanged();
+        m_regularUpdates = true;
+        if (!m_active) {
+            m_active = true;
+            emit activeChanged();
+        }
     }
 }
 
@@ -475,9 +478,9 @@ void QDeclarativePositionSource::start()
 void QDeclarativePositionSource::update(int timeout)
 {
     if (m_positionSource) {
+        m_singleUpdate = true;
         if (!m_active) {
             m_active = true;
-            m_singleUpdate = true;
             emit activeChanged();
         }
         // Use default timeout value. Set active before calling the
@@ -501,7 +504,8 @@ void QDeclarativePositionSource::stop()
 {
     if (m_positionSource) {
         m_positionSource->stopUpdates();
-        if (m_active) {
+        m_regularUpdates = false;
+        if (m_active && !m_singleUpdate) {
             m_active = false;
             emit activeChanged();
         }
@@ -560,9 +564,14 @@ void QDeclarativePositionSource::positionUpdateReceived(const QGeoPositionInfo &
     setPosition(update);
 
     if (m_singleUpdate && m_active) {
-        m_active = false;
+        // we need to reset m_singleUpdate because we got one
         m_singleUpdate = false;
-        emit activeChanged();
+        if (!m_regularUpdates) {
+            // but we change the active state only if the regular updates are
+            // also stopped
+            m_active = false;
+            emit activeChanged();
+        }
     }
 }
 
diff --git a/src/positioningquick/qdeclarativepositionsource_p.h b/src/positioningquick/qdeclarativepositionsource_p.h
index f4883a8a..e60ad47a 100644
--- a/src/positioningquick/qdeclarativepositionsource_p.h
+++ b/src/positioningquick/qdeclarativepositionsource_p.h
@@ -163,14 +163,15 @@ private:
     static QDeclarativePluginParameter *parameter_at(QQmlListProperty<QDeclarativePluginParameter> *prop, qsizetype index);
     static void parameter_clear(QQmlListProperty<QDeclarativePluginParameter> *prop);
 
-    QGeoPositionInfoSource *m_positionSource;
+    QGeoPositionInfoSource *m_positionSource = nullptr;
     QDeclarativePosition m_position;
-    PositioningMethods m_preferredPositioningMethods;
+    PositioningMethods m_preferredPositioningMethods = AllPositioningMethods;
     QString m_providerName;
-    bool m_active;
-    bool m_singleUpdate;
-    int m_updateInterval;
-    SourceError m_sourceError;
+    bool m_active = false;
+    bool m_singleUpdate = false;
+    bool m_regularUpdates = false;
+    int m_updateInterval = 0;
+    SourceError m_sourceError = NoError;
     QList<QDeclarativePluginParameter *> m_parameters;
     bool m_componentComplete = false;
     bool m_parametersInitialized = false;
diff --git a/tests/auto/CMakeLists.txt b/tests/auto/CMakeLists.txt
index 0fbe1c03..d7eb2df2 100644
--- a/tests/auto/CMakeLists.txt
+++ b/tests/auto/CMakeLists.txt
@@ -78,6 +78,7 @@ if(TARGET Qt::Quick AND NOT ANDROID)
     add_subdirectory(declarative_positioning_core)
     add_subdirectory(declarative_geolocation)
     add_subdirectory(qdeclarativeposition)
+    add_subdirectory(qdeclarativepositionsource)
 endif()
 if(NOT ANDROID)
     add_subdirectory(positionplugin)
diff --git a/tests/auto/qdeclarativepositionsource/CMakeLists.txt b/tests/auto/qdeclarativepositionsource/CMakeLists.txt
new file mode 100644
index 00000000..5015162d
--- /dev/null
+++ b/tests/auto/qdeclarativepositionsource/CMakeLists.txt
@@ -0,0 +1,8 @@
+qt_internal_add_test(tst_qdeclarativepositionsource
+    SOURCES
+        tst_qdeclarativepositionsource.cpp
+    LIBRARIES
+        Qt::Core
+        Qt::Positioning
+        Qt::PositioningQuickPrivate
+)
diff --git a/tests/auto/qdeclarativepositionsource/tst_qdeclarativepositionsource.cpp b/tests/auto/qdeclarativepositionsource/tst_qdeclarativepositionsource.cpp
new file mode 100644
index 00000000..7ae4d3db
--- /dev/null
+++ b/tests/auto/qdeclarativepositionsource/tst_qdeclarativepositionsource.cpp
@@ -0,0 +1,204 @@
+/****************************************************************************
+**
+** Copyright (C) 2021 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the test suite of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:GPL-EXCEPT$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+#include <QtTest/QtTest>
+#include <QtPositioningQuick/private/qdeclarativepositionsource_p.h>
+
+QT_USE_NAMESPACE
+
+class tst_DeclarativePositionSource : public QObject
+{
+    Q_OBJECT
+
+private slots:
+    void init();
+
+    void updateAfterStart();
+    void startAfterUpdate();
+    void stopAfterUpdate();
+    void startStopAfterUpdate();
+    void updateTimedOut();
+    void updateWithStartTimedOut();
+    void startUpdateStopWithNoIntervals();
+
+private:
+    std::unique_ptr<QDeclarativePositionSource> m_positionSource = nullptr;
+};
+
+void tst_DeclarativePositionSource::init()
+{
+    // create a fresh instance of QDeclarativePositionSource before each test
+    m_positionSource.reset(new QDeclarativePositionSource);
+    m_positionSource->componentComplete(); // simulate QML loading
+}
+
+void tst_DeclarativePositionSource::updateAfterStart()
+{
+    // When update() is called after start(), it should not invalidate any
+    // state. The active state must still be true when the single update() is
+    // completed.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->start();
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->update(1100);
+    QTest::qWait(1500); // wait for the single update to complete
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->stop();
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::startAfterUpdate()
+{
+    // When start() is called after update(), the position source should remain
+    // active even when the signle update is completed.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->update(1100);
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->start();
+    QTest::qWait(1500); // wait for the single update to complete
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->stop();
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::stopAfterUpdate()
+{
+    // When stop() is called after update(), and the update() is still in
+    // progress, the position source should remain active until the update()
+    // is completed.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->update(1100);
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->stop();
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    QTest::qWait(1500); // wait for the single update to complete
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::startStopAfterUpdate()
+{
+    // Quite artificial example. Calling start() and stop() after update(),
+    // while still waiting for the update() to complete, should still result in
+    // the position source to be active until the update() is completed.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->update(1100);
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->start();
+    m_positionSource->stop();
+
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    QTest::qWait(1500); // wait for the single update to complete
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::updateTimedOut()
+{
+    // This test checks that we reset to inactive state when the single update()
+    // request times out without providing the position info
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->update(200); // to small timeout -> will return an error
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    QTRY_COMPARE_WITH_TIMEOUT(m_positionSource->sourceError(),
+                              QDeclarativePositionSource::UpdateTimeoutError, 300);
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::updateWithStartTimedOut()
+{
+    // This test checks that if single update() times out, but the regular
+    // updates are running, we still remain in active state.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->start();
+
+    m_positionSource->update(200); // to small timeout -> will return an error
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    QTRY_COMPARE_WITH_TIMEOUT(m_positionSource->sourceError(),
+                              QDeclarativePositionSource::UpdateTimeoutError, 300);
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    m_positionSource->stop();
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+void tst_DeclarativePositionSource::startUpdateStopWithNoIntervals()
+{
+    // This test checks that a sequence of calls start() -> update() -> stop()
+    // without any waits between them will result in expected behavior.
+    // Specifically, the position source should remain active until it gets
+    // the position response.
+
+    m_positionSource->setName("test.source");
+
+    QCOMPARE(m_positionSource->isActive(), false);
+
+    m_positionSource->start();
+    m_positionSource->update(1100);
+    QCOMPARE(m_positionSource->isActive(), true);
+    m_positionSource->stop();
+    QCOMPARE(m_positionSource->isActive(), true);
+
+    QTest::qWait(1500); // wait for the single update to complete
+    QCOMPARE(m_positionSource->isActive(), false);
+}
+
+QTEST_MAIN(tst_DeclarativePositionSource)
+
+#include "tst_qdeclarativepositionsource.moc"