PositionSource property fixes.

PositionSource has a couple of property signal issues which stem from
the fact that the order of property evaluation is not well defined
during component construction.  Fix emission of signals when the
underlying position info source changes.  When a new position info
source is created the updatInterval, preferredPositioningMethods and
supportedPositioningMethods change notification signals are emitted if
necessary.

Construction of a default position info source is delayed until after
the component has been constructed.  This saves creating a default
position info source in the constructor and then (possibly) replacing
it when the name property is set.

Fix possibility of setting unsupported position methods as preferred.
When setting the preferred positioning methods call the base class
implementation first, to ensure that only supported positioning methods
are set, prior to acting on the new preferred positioning methods.

Change-Id: Ie402f87ca7d82b52fd4678c40867894d3b122942
Reviewed-by: Alex <alexander.blasche@digia.com>

6 files changed, 188 insertions, 80 deletions

diff --git a/src/imports/location/qdeclarativepositionsource.cpp b/src/imports/location/qdeclarativepositionsource.cpp
index 3e9a8a01..d4605a42 100644
--- a/src/imports/location/qdeclarativepositionsource.cpp
+++ b/src/imports/location/qdeclarativepositionsource.cpp
@@ -116,24 +116,9 @@ QT_BEGIN_NAMESPACE
 */
 
 QDeclarativePositionSource::QDeclarativePositionSource()
-    : m_positionSource(0), m_positioningMethod(QDeclarativePositionSource::NoPositioningMethod),
-      m_nmeaFile(0), m_active(false), m_singleUpdate(false), m_updateInterval(0),
-      m_sourceError(UnknownSourceError)
+:   m_positionSource(0), m_preferredPositioningMethods(NoPositioningMethod), m_nmeaFile(0),
+    m_active(false), m_singleUpdate(false), m_updateInterval(0), m_sourceError(UnknownSourceError)
 {
-    m_positionSource = QGeoPositionInfoSource::createDefaultSource(this);
-    if (m_positionSource) {
-        connect(m_positionSource, SIGNAL(positionUpdated(QGeoPositionInfo)),
-                this, SLOT(positionUpdateReceived(QGeoPositionInfo)));
-        connect(m_positionSource, SIGNAL(error(QGeoPositionInfoSource::Error)),
-                this, SLOT(sourceErrorReceived(QGeoPositionInfoSource::Error)));
-        m_positioningMethod = supportedPositioningMethods();
-    }
-#ifdef QDECLARATIVE_POSITION_DEBUG
-    if (m_positionSource)
-        qDebug("QDeclarativePositionSource QGeoPositionInfoSource::createDefaultSource SUCCESS");
-    else
-        qDebug("QDeclarativePositionSource QGeoPositionInfoSource::createDefaultSource FAILURE");
-#endif
 }
 
 QDeclarativePositionSource::~QDeclarativePositionSource()
@@ -149,9 +134,13 @@ QDeclarativePositionSource::~QDeclarativePositionSource()
     This property holds the unique internal name for the plugin currently
     providing position information.
 
-    Setting the property causes the PositionSource to use a particular backend
-    plugin. If the PositionSource is active at the time that the plugin property
-    is changed, it will become inactive.
+    Setting the property causes the PositionSource to use a particular positioning provider.  If
+    the PositionSource is active at the time that the name property is changed, it will become
+    inactive.  If the specified positioning provider cannot be loaded the position source will
+    become invalid.
+
+    Changing the name property may cause the \l {updateInterval}, \l {supportedPositioningMethods}
+    and \l {preferredPositioningMethods} properties to change as well.
 */
 
 
@@ -163,23 +152,51 @@ QString QDeclarativePositionSource::name() const
         return QString();
 }
 
-void QDeclarativePositionSource::setName(const QString &name)
+void QDeclarativePositionSource::setName(const QString &newName)
 {
-    if (m_positionSource && m_positionSource->sourceName() == name)
+    if (m_positionSource && m_positionSource->sourceName() == newName)
         return;
 
+    const QString previousName = name();
+    int previousUpdateInterval = updateInterval();
+    PositioningMethods previousPositioningMethods = supportedPositioningMethods();
+    PositioningMethods previousPreferredPositioningMethods = preferredPositioningMethods();
+
     delete m_positionSource;
-    m_positionSource = QGeoPositionInfoSource::createSource(name, this);
+    if (newName.isEmpty())
+        m_positionSource = QGeoPositionInfoSource::createDefaultSource(this);
+    else
+        m_positionSource = QGeoPositionInfoSource::createSource(newName, this);
+
     if (m_positionSource) {
         connect(m_positionSource, SIGNAL(positionUpdated(QGeoPositionInfo)),
                 this, SLOT(positionUpdateReceived(QGeoPositionInfo)));
         connect(m_positionSource, SIGNAL(error(QGeoPositionInfoSource::Error)),
                 this, SLOT(sourceErrorReceived(QGeoPositionInfoSource::Error)));
-        m_positioningMethod = supportedPositioningMethods();
+
+        m_positionSource->setUpdateInterval(m_updateInterval);
+        m_positionSource->setPreferredPositioningMethods(
+            static_cast<QGeoPositionInfoSource::PositioningMethods>(int(m_preferredPositioningMethods)));
     }
+
+    if (previousUpdateInterval != updateInterval())
+        emit updateIntervalChanged();
+
+    if (previousPreferredPositioningMethods != preferredPositioningMethods())
+        emit preferredPositioningMethodsChanged();
+
+    if (previousPositioningMethods != supportedPositioningMethods())
+        emit supportedPositioningMethodsChanged();
+
     emit validityChanged();
-    m_active = false;
-    emit this->nameChanged();
+
+    if (m_active) {
+        m_active = false;
+        emit activeChanged();
+    }
+
+    if (previousName != name())
+        emit nameChanged();
 }
 
 /*!
@@ -219,6 +236,9 @@ void QDeclarativePositionSource::setNmeaSource(const QUrl &nmeaSource)
         return;
     m_nmeaFileName = localFileName;
     m_nmeaSource = nmeaSource;
+
+    PositioningMethods previousPositioningMethods = supportedPositioningMethods();
+
     // The current position source needs to be deleted
     // because QNmeaPositionInfoSource can be bound only to a one file.
     delete m_positionSource;
@@ -255,11 +275,11 @@ void QDeclarativePositionSource::setNmeaSource(const QUrl &nmeaSource)
             emit activeChanged();
         }
     }
-    if (m_positioningMethod != supportedPositioningMethods()) {
-        m_positioningMethod = supportedPositioningMethods();
+
+    if (previousPositioningMethods != supportedPositioningMethods())
         emit supportedPositioningMethodsChanged();
-    }
-    emit this->nmeaSourceChanged();
+
+    emit nmeaSourceChanged();
 }
 
 /*!
@@ -267,14 +287,22 @@ void QDeclarativePositionSource::setNmeaSource(const QUrl &nmeaSource)
 */
 void QDeclarativePositionSource::setUpdateInterval(int updateInterval)
 {
-    if (m_updateInterval == updateInterval)
-        return;
-
-    m_updateInterval = updateInterval;
     if (m_positionSource) {
-        m_positionSource->setUpdateInterval(updateInterval);
+        int previousUpdateInterval = m_positionSource->updateInterval();
+
+        m_updateInterval = updateInterval;
+
+        if (previousUpdateInterval != updateInterval) {
+            m_positionSource->setUpdateInterval(updateInterval);
+            if (previousUpdateInterval != m_positionSource->updateInterval())
+                emit updateIntervalChanged();
+        }
+    } else {
+        if (m_updateInterval != updateInterval) {
+            m_updateInterval = updateInterval;
+            emit updateIntervalChanged();
+        }
     }
-    emit updateIntervalChanged();
 }
 
 /*!
@@ -299,27 +327,19 @@ QUrl QDeclarativePositionSource::nmeaSource() const
 }
 
 /*!
-    \qmlproperty bool PositionSource::updateInterval
+    \qmlproperty int PositionSource::updateInterval
 
     This property holds the desired interval between updates (milliseconds).
 
     \sa {QGeoPositionInfoSource::updateInterval()}
-
 */
 
 int QDeclarativePositionSource::updateInterval() const
 {
-    if (m_positionSource) {
-        int ival = m_positionSource->updateInterval();
-        if (m_updateInterval != ival) {
-            QDeclarativePositionSource *me = const_cast<QDeclarativePositionSource *>(this);
-            me->m_updateInterval = ival;
-            emit me->updateIntervalChanged();
-        }
-        return ival;
-    } else {
+    if (!m_positionSource)
         return m_updateInterval;
-    }
+
+    return m_positionSource->updateInterval();
 }
 
 /*!
@@ -366,16 +386,32 @@ QDeclarativePositionSource::PositioningMethods QDeclarativePositionSource::suppo
 
 */
 
-void QDeclarativePositionSource::setPreferredPositioningMethods(QGeoPositionInfoSource::PositioningMethods methods)
+void QDeclarativePositionSource::setPreferredPositioningMethods(PositioningMethods methods)
 {
-    m_positionSource->setPreferredPositioningMethods(methods);
-    emit preferredPositioningMethodsChanged();
+    if (m_positionSource) {
+        PositioningMethods previousPreferredPositioningMethods = preferredPositioningMethods();
+
+        m_preferredPositioningMethods = methods;
+
+        if (previousPreferredPositioningMethods != methods) {
+            m_positionSource->setPreferredPositioningMethods(
+                static_cast<QGeoPositionInfoSource::PositioningMethods>(int(methods)));
+            if (previousPreferredPositioningMethods != m_positionSource->preferredPositioningMethods())
+                emit preferredPositioningMethodsChanged();
+        }
+    } else {
+        if (m_preferredPositioningMethods != methods) {
+            m_preferredPositioningMethods = methods;
+            emit preferredPositioningMethodsChanged();
+        }
+    }
 }
 
 QDeclarativePositionSource::PositioningMethods QDeclarativePositionSource::preferredPositioningMethods() const
 {
     if (m_positionSource) {
-        QGeoPositionInfoSource::PositioningMethods methods = m_positionSource->preferredPositioningMethods();
+        QGeoPositionInfoSource::PositioningMethods methods =
+            m_positionSource->preferredPositioningMethods();
         if ( (methods & QGeoPositionInfoSource::AllPositioningMethods) == methods) {
             return QDeclarativePositionSource::AllPositioningMethods;
         } else if (methods & QGeoPositionInfoSource::SatellitePositioningMethods) {
@@ -384,7 +420,7 @@ QDeclarativePositionSource::PositioningMethods QDeclarativePositionSource::prefe
             return QDeclarativePositionSource::NonSatellitePositioningMethod;
         }
     }
-    return QDeclarativePositionSource::NoPositioningMethod;
+    return m_preferredPositioningMethods;
 }
 
 /*!
@@ -399,14 +435,13 @@ QDeclarativePositionSource::PositioningMethods QDeclarativePositionSource::prefe
 
 void QDeclarativePositionSource::start()
 {
-    if (m_positionSource) {
-        // Safe to set, setting zero means using default value
-        m_positionSource->setUpdateInterval(m_updateInterval);
-        m_positionSource->startUpdates();
-        if (!m_active) {
-            m_active = true;
-            emit activeChanged();
-        }
+    if (!m_positionSource)
+        return;
+
+    m_positionSource->startUpdates();
+    if (!m_active) {
+        m_active = true;
+        emit activeChanged();
     }
 }
 
@@ -553,6 +588,45 @@ QDeclarativePositionSource::SourceError QDeclarativePositionSource::sourceError(
     return m_sourceError;
 }
 
+void QDeclarativePositionSource::componentComplete()
+{
+    if (!m_positionSource) {
+        int previousUpdateInterval = updateInterval();
+        PositioningMethods previousPositioningMethods = supportedPositioningMethods();
+        PositioningMethods previousPreferredPositioningMethods = preferredPositioningMethods();
+
+        m_positionSource = QGeoPositionInfoSource::createDefaultSource(this);
+        if (m_positionSource) {
+            connect(m_positionSource, SIGNAL(positionUpdated(QGeoPositionInfo)),
+                    this, SLOT(positionUpdateReceived(QGeoPositionInfo)));
+            connect(m_positionSource, SIGNAL(error(QGeoPositionInfoSource::Error)),
+                    this, SLOT(sourceErrorReceived(QGeoPositionInfoSource::Error)));
+
+            m_positionSource->setUpdateInterval(m_updateInterval);
+            m_positionSource->setPreferredPositioningMethods(
+                static_cast<QGeoPositionInfoSource::PositioningMethods>(int(m_preferredPositioningMethods)));
+        }
+
+        if (previousUpdateInterval != updateInterval())
+            emit updateIntervalChanged();
+
+        if (previousPreferredPositioningMethods != preferredPositioningMethods())
+            emit preferredPositioningMethodsChanged();
+
+        if (previousPositioningMethods != supportedPositioningMethods())
+            emit supportedPositioningMethodsChanged();
+
+        emit validityChanged();
+
+        if (m_active) {
+            m_active = false;
+            emit activeChanged();
+        }
+
+        emit nameChanged();
+    }
+}
+
 /*!
     \internal
 */
diff --git a/src/imports/location/qdeclarativepositionsource_p.h b/src/imports/location/qdeclarativepositionsource_p.h
index 122a734b..dea18b62 100644
--- a/src/imports/location/qdeclarativepositionsource_p.h
+++ b/src/imports/location/qdeclarativepositionsource_p.h
@@ -42,18 +42,17 @@
 #ifndef QDECLARATIVEPOSITIONSOURCE_H
 #define QDECLARATIVEPOSITIONSOURCE_H
 
-#include <QtCore>
-#include <QDateTime>
-#include <qgeopositioninfosource.h>
-#include <qgeopositioninfo.h>
-#include <QtQml/qqml.h>
 #include "qdeclarativeposition_p.h"
 
+#include <QtCore/QObject>
+#include <QtQml/QQmlParserStatus>
+#include <QtLocation/QGeoPositionInfoSource>
+
 QT_BEGIN_NAMESPACE
 
 class QFile;
 
-class QDeclarativePositionSource : public QObject
+class QDeclarativePositionSource : public QObject, public QQmlParserStatus
 {
     Q_OBJECT
 
@@ -68,6 +67,8 @@ class QDeclarativePositionSource : public QObject
     Q_PROPERTY(QString name READ name WRITE setName NOTIFY nameChanged)
     Q_ENUMS(PositioningMethod)
 
+    Q_INTERFACES(QQmlParserStatus)
+
 public:
     enum PositioningMethod {
         NoPositioningMethod = 0,
@@ -86,13 +87,12 @@ public:
     };
     Q_ENUMS(SourceError)
 
-
     QDeclarativePositionSource();
     ~QDeclarativePositionSource();
     void setNmeaSource(const QUrl &nmeaSource);
     void setUpdateInterval(int updateInterval);
     void setActive(bool active);
-    void setPreferredPositioningMethods(QGeoPositionInfoSource::PositioningMethods methods);
+    void setPreferredPositioningMethods(PositioningMethods methods);
 
     QString name() const;
     void setName(const QString &name);
@@ -106,6 +106,10 @@ public:
     PositioningMethods preferredPositioningMethods() const;
     SourceError sourceError() const;
 
+    // Virtuals from QQmlParserStatus
+    void classBegin() { }
+    void componentComplete();
+
 public Q_SLOTS:
     void update();
     void start();
@@ -128,8 +132,8 @@ private Q_SLOTS:
     void sourceErrorReceived(const QGeoPositionInfoSource::Error error);
 private:
     QGeoPositionInfoSource *m_positionSource;
-    PositioningMethods m_positioningMethod;
     QDeclarativePosition m_position;
+    PositioningMethods m_preferredPositioningMethods;
     QFile *m_nmeaFile;
     QString m_nmeaFileName;
     QUrl m_nmeaSource;
diff --git a/src/plugins/position/geoclue/qgeopositioninfosource_geocluemaster.cpp b/src/plugins/position/geoclue/qgeopositioninfosource_geocluemaster.cpp
index 8b1ec29c..920c5be8 100644
--- a/src/plugins/position/geoclue/qgeopositioninfosource_geocluemaster.cpp
+++ b/src/plugins/position/geoclue/qgeopositioninfosource_geocluemaster.cpp
@@ -354,11 +354,12 @@ void QGeoPositionInfoSourceGeoclueMaster::setUpdateInterval(int msec)
 
 void QGeoPositionInfoSourceGeoclueMaster::setPreferredPositioningMethods(PositioningMethods methods)
 {
-    if (methods == m_methods)
+    PositioningMethods previousPreferredPositioningMethods = preferredPositioningMethods();
+    QGeoPositionInfoSource::setPreferredPositioningMethods(methods);
+    if (previousPreferredPositioningMethods == preferredPositioningMethods())
         return;
-    m_methods = methods;
 
-    switch (methods) {
+    switch (preferredPositioningMethods()) {
     case SatellitePositioningMethods:
         m_preferredResources = GEOCLUE_RESOURCE_GPS;
         m_preferredAccuracy = GEOCLUE_ACCURACY_LEVEL_DETAILED;
@@ -375,10 +376,11 @@ void QGeoPositionInfoSourceGeoclueMaster::setPreferredPositioningMethods(Positio
         qWarning("GeoPositionInfoSourceGeoClueMaster unknown preferred method.");
         return;
     }
-    QGeoPositionInfoSource::setPreferredPositioningMethods(methods);
+
 #ifdef Q_LOCATION_GEOCLUE_DEBUG
     qDebug() << "QGeoPositionInfoSourceGeoclueMaster requested to set methods to, and set them to: " << methods << m_preferredResources;
 #endif
+
     m_lastPositionIsFresh = false;
     m_lastVelocityIsFresh = false;
     int status = configurePositionSource();
diff --git a/src/plugins/position/maemo/qgeopositioninfosource_maemo.cpp b/src/plugins/position/maemo/qgeopositioninfosource_maemo.cpp
index 14a9a676..f68d67fc 100644
--- a/src/plugins/position/maemo/qgeopositioninfosource_maemo.cpp
+++ b/src/plugins/position/maemo/qgeopositioninfosource_maemo.cpp
@@ -135,7 +135,7 @@ void QGeoPositionInfoSourceMaemo::setUpdateInterval(int msec)
 void QGeoPositionInfoSourceMaemo::setPreferredPositioningMethods(PositioningMethods sources)
 {
     QGeoPositionInfoSource::setPreferredPositioningMethods(sources);
-    dbusComm->sendConfigRequest(dbusComm->CommandSetMethods, sources, 0);
+    dbusComm->sendConfigRequest(dbusComm->CommandSetMethods, preferredPositioningMethods(), 0);
 }
 
 
diff --git a/src/plugins/position/npe_backend/qgeopositioninfosource_npe_backend.cpp b/src/plugins/position/npe_backend/qgeopositioninfosource_npe_backend.cpp
index 49ecf583..da46cce0 100644
--- a/src/plugins/position/npe_backend/qgeopositioninfosource_npe_backend.cpp
+++ b/src/plugins/position/npe_backend/qgeopositioninfosource_npe_backend.cpp
@@ -115,7 +115,7 @@ void QGeoPositionInfoSourceNpeBackend::setPreferredPositioningMethods(Positionin
 {
     QGeoPositionInfoSource::setPreferredPositioningMethods(sources);
     LocationdStrings::PositionInfo::PositioningMethods positioningMethod;
-    switch (sources){
+    switch (preferredPositioningMethods()){
     case QGeoPositionInfoSource::SatellitePositioningMethods:
         positioningMethod = LocationdStrings::PositionInfo::Satellite;
         break;
diff --git a/tests/auto/declarative_core/tst_positionsource.qml b/tests/auto/declarative_core/tst_positionsource.qml
index 4b809d26..46540fcf 100644
--- a/tests/auto/declarative_core/tst_positionsource.qml
+++ b/tests/auto/declarative_core/tst_positionsource.qml
@@ -49,8 +49,26 @@ TestCase {
     name: "PositionSource"
 
     PositionSource { id: defaultSource }
+    PositionSource
+    {
+        id: activeDefaultSource
+        active: true
+    }
+
     SignalSpy { id: defaultSourceSpy; target: defaultSource; signalName: "positionChanged" }
 
+    function test_activeDefaultSource() {
+        wait(0);
+        verify(activeDefaultSource.name !== "");
+        compare(activeDefaultSource.active, true);
+    }
+
+    function test_invalidSource() {
+        activeDefaultSource.name = "invalid_positioning_source";
+        verify(!activeDefaultSource.active);
+        verify(!activeDefaultSource.valid);
+    }
+
     function test_defaults() {
         // at least the test.source plugin should be available
         verify(defaultSource.name != "");
@@ -70,15 +88,25 @@ TestCase {
     function test_setplugin() {
         testingSourcePluginSpy.clear();
 
-        testSetSource.name = "test.source";
-        compare(testingSourcePluginSpy.count, 1);
+        // On construction, if the provided source name is invalid, the default source will be
+        // used. Test that the source is valid as expected.
+        compare(testSetSource.name, "test.source");
+        verify(testSetSource.valid);
+
+        // Test that setting name to "" will still use the default.
+        testSetSource.name = "";
+        compare(testingSourcePluginSpy.count, 0);
         compare(testSetSource.name, "test.source");
+        verify(testSetSource.valid);
 
         testSetSource.name = "test.source";
-        compare(testingSourcePluginSpy.count, 1);
+        compare(testingSourcePluginSpy.count, 0);
+        compare(testSetSource.name, "test.source");
+        verify(testSetSource.valid);
 
         testSetSource.name = "bogus";
-        compare(testingSourcePluginSpy.count, 2);
+        compare(testingSourcePluginSpy.count, 1);
+        verify(!testSetSource.valid);
     }
 
     PositionSource { id: testingSource; name: "test.source"; updateInterval: 1000 }