Safer and easier navigation APIs for plugins

Each QDeclarativeNavigator will create and manage its own
QAbstractNavigator using the QNavigationManagerEngine, this way
the QNavigationManagerEngine doesn't have to manage the sessions
itself based on a const reference.

Change-Id: Ibe006524969b4f8100e71ea188d951072b846c6c
Reviewed-by: Paolo Angelelli <paolo.angelelli@qt.io>

7 files changed, 153 insertions, 133 deletions

diff --git a/src/location/labs/qdeclarativenavigator.cpp b/src/location/labs/qdeclarativenavigator.cpp
index 7dba341e..b06681d9 100644
--- a/src/location/labs/qdeclarativenavigator.cpp
+++ b/src/location/labs/qdeclarativenavigator.cpp
@@ -175,7 +175,7 @@ QT_BEGIN_NAMESPACE
 */
 
 QDeclarativeNavigatorPrivate::QDeclarativeNavigatorPrivate(QParameterizableObject *q_)
-    : q(q_)
+    : q(q_), m_params(new QDeclarativeNavigatorParams)
 {
 }
 
@@ -203,7 +203,8 @@ void QDeclarativeNavigator::componentComplete()
 {
     d_ptr->m_completed = true;
     // Children have been completed
-    d_ptr->m_parameters = quickChildren<QGeoMapParameter>();
+    for (auto param : quickChildren<QGeoMapParameter>())
+        d_ptr->m_params->m_parameters.push_back(param);
     if (d_ptr->m_plugin && d_ptr->m_plugin->isAttached())
         pluginReady();
 }
@@ -215,12 +216,12 @@ QDeclarativeGeoServiceProvider *QDeclarativeNavigator::plugin() const
 
 void QDeclarativeNavigator::setMap(QDeclarativeGeoMap *map)
 {
-    if (d_ptr->m_map || !map) // set once prop
+    if (d_ptr->m_params->m_map || !map) // set once prop
         return;
 
-    d_ptr->m_map = map;
+    d_ptr->m_params->m_map = map;
     QDeclarativeNavigatorPrivate *dptr = d_ptr.data();
-    connect(map, &QObject::destroyed,
+    connect(map, &QObject::destroyed, this,
             [this, dptr]() {
                 this->mapChanged();
                 dptr->updateReadyState();
@@ -231,21 +232,21 @@ void QDeclarativeNavigator::setMap(QDeclarativeGeoMap *map)
 
 QDeclarativeGeoMap *QDeclarativeNavigator::map() const
 {
-    return d_ptr->m_map;
+    return d_ptr->m_params->m_map;
 }
 
 void QDeclarativeNavigator::setRoute(QDeclarativeGeoRoute *route)
 {
-    if (d_ptr->m_route == route) // This isn't set-once
+    if (d_ptr->m_params->m_route == route) // This isn't set-once
         return;
 
-    const bool isReady = d_ptr->m_navigationManager && d_ptr->m_navigationManager->ready();
+    const bool isReady = d_ptr->m_navigator && d_ptr->m_navigator->ready();
     const bool isActive = active();
     if (isReady && isActive)
         setActive(false); // Stop current session
 
-    d_ptr->m_route = route;
-    d_ptr->m_geoRoute = route ? route->route() : QGeoRoute();
+    d_ptr->m_params->m_route = route;
+    d_ptr->m_params->m_geoRoute = route ? route->route() : QGeoRoute();
     if (route) {
         connect(route, &QObject::destroyed,
                 [this]() {
@@ -260,15 +261,15 @@ void QDeclarativeNavigator::setRoute(QDeclarativeGeoRoute *route)
 
 QDeclarativeGeoRoute *QDeclarativeNavigator::route() const
 {
-    return d_ptr->m_route;
+    return d_ptr->m_params->m_route;
 }
 
 void QDeclarativeNavigator::setPositionSource(QDeclarativePositionSource *positionSource)
 {
-    if (d_ptr->m_positionSource || !positionSource) // set once prop
+    if (d_ptr->m_params->m_positionSource || !positionSource) // set once prop
         return;
 
-    d_ptr->m_positionSource = positionSource;
+    d_ptr->m_params->m_positionSource = positionSource;
     QDeclarativeNavigatorPrivate *dptr = d_ptr.data();
     QObject::connect(positionSource, &QObject::destroyed,
             [this, dptr]() {
@@ -282,18 +283,14 @@ void QDeclarativeNavigator::setPositionSource(QDeclarativePositionSource *positi
 
 QDeclarativePositionSource *QDeclarativeNavigator::positionSource() const
 {
-    return d_ptr->m_positionSource;
+    return d_ptr->m_params->m_positionSource;
 }
 
-QNavigationManager *QDeclarativeNavigator::navigationManager() const
-{
-    return d_ptr->m_navigationManager;
-}
 
 bool QDeclarativeNavigator::navigatorReady() const
 {
-    if (d_ptr->m_navigationManager)
-        return d_ptr->m_navigationManager->ready();
+    if (d_ptr->m_navigator)
+        return d_ptr->m_navigator->ready();
     return d_ptr->m_ready;
 }
 
@@ -314,14 +311,14 @@ void QDeclarativeNavigator::setTrackPositionSource(bool trackPositionSource)
 
 QDeclarativeGeoRoute *QDeclarativeNavigator::currentRoute() const
 {
-    if (!d_ptr->m_ready || !d_ptr->m_navigationManager->active())
-        return d_ptr->m_route.data();
+    if (!d_ptr->m_ready || !d_ptr->m_navigator->active())
+        return d_ptr->m_params->m_route.data();
     return d_ptr->m_currentRoute.data();
 }
 
 int QDeclarativeNavigator::currentSegment() const
 {
-    if (!d_ptr->m_ready || !d_ptr->m_navigationManager->active())
+    if (!d_ptr->m_ready || !d_ptr->m_navigator->active())
         return 0;
     return d_ptr->m_currentSegment;
 }
@@ -369,8 +366,8 @@ void QDeclarativeNavigator::start()
         return;
     }
 
-    if (!d_ptr->m_navigationManager->active())
-        d_ptr->m_active = d_ptr->m_navigationManager->start();
+    if (!d_ptr->m_navigator->active())
+        d_ptr->m_active = d_ptr->m_navigator->start();
 }
 
 void QDeclarativeNavigator::stop()
@@ -380,8 +377,8 @@ void QDeclarativeNavigator::stop()
         return;
     }
 
-    if (d_ptr->m_navigationManager->active())
-        d_ptr->m_active = d_ptr->m_navigationManager->stop();
+    if (d_ptr->m_navigator->active())
+        d_ptr->m_active = d_ptr->m_navigator->stop();
 }
 
 void QDeclarativeNavigator::pluginReady()
@@ -397,20 +394,23 @@ void QDeclarativeNavigator::pluginReady()
 
 bool QDeclarativeNavigator::ensureEngine()
 {
-    if (d_ptr->m_navigationManager)
+    if (d_ptr->m_navigator)
         return true;
     if (!d_ptr->m_completed || !d_ptr->m_plugin->isAttached())
         return false;
 
-    d_ptr->m_navigationManager = d_ptr->m_plugin->sharedGeoServiceProvider()->navigationManager();
-    if (d_ptr->m_navigationManager) {
-        d_ptr->m_navigationManager->setNavigator(d_ptr.data());
-        d_ptr->m_navigationManager->setParameters(d_ptr->m_parameters);
-        connect(d_ptr->m_navigationManager, &QNavigationManager::waypointReached, this, &QDeclarativeNavigator::waypointReached);
-        connect(d_ptr->m_navigationManager, &QNavigationManager::destinationReached, this, &QDeclarativeNavigator::destinationReached);
-        connect(d_ptr->m_navigationManager, &QNavigationManager::currentRouteChanged, this, &QDeclarativeNavigator::onCurrentRouteChanged);
-        connect(d_ptr->m_navigationManager, &QNavigationManager::currentSegmentChanged, this, &QDeclarativeNavigator::onCurrentSegmentChanged);
-        connect(d_ptr->m_navigationManager, &QNavigationManager::activeChanged, this, [this](bool active){
+    auto manager = d_ptr->m_plugin->sharedGeoServiceProvider()->navigationManager();
+    if (manager) {
+        d_ptr->m_navigator.reset(manager->createNavigator(d_ptr->m_params));
+        if (!d_ptr->m_navigator)
+            return false;
+        d_ptr->m_navigator->setLocale(manager->locale());
+        d_ptr->m_navigator->setMeasurementSystem(manager->measurementSystem());
+        connect(d_ptr->m_navigator.get(), &QAbstractNavigator::waypointReached, this, &QDeclarativeNavigator::waypointReached);
+        connect(d_ptr->m_navigator.get(), &QAbstractNavigator::destinationReached, this, &QDeclarativeNavigator::destinationReached);
+        connect(d_ptr->m_navigator.get(), &QAbstractNavigator::currentRouteChanged, this, &QDeclarativeNavigator::onCurrentRouteChanged);
+        connect(d_ptr->m_navigator.get(), &QAbstractNavigator::currentSegmentChanged, this, &QDeclarativeNavigator::onCurrentSegmentChanged);
+        connect(d_ptr->m_navigator.get(), &QAbstractNavigator::activeChanged, this, [this](bool active){
             d_ptr->m_active = active;
             emit activeChanged(active);
         });
@@ -422,10 +422,10 @@ bool QDeclarativeNavigator::ensureEngine()
 
 void QDeclarativeNavigator::updateReadyState() {
     const bool oldReady = d_ptr->m_ready;
-    if (!d_ptr->m_navigationManager)
+    if (!d_ptr->m_navigator)
         d_ptr->m_ready = false;
     else
-        d_ptr->m_ready = d_ptr->m_navigationManager->ready();
+        d_ptr->m_ready = d_ptr->m_navigator->ready();
 
     if (oldReady != d_ptr->m_ready)
         emit navigatorReadyChanged(d_ptr->m_ready);
diff --git a/src/location/labs/qdeclarativenavigator_p.h b/src/location/labs/qdeclarativenavigator_p.h
index 3c83ec94..13884c41 100644
--- a/src/location/labs/qdeclarativenavigator_p.h
+++ b/src/location/labs/qdeclarativenavigator_p.h
@@ -107,7 +107,6 @@ public:
     void setPositionSource(QDeclarativePositionSource *positionSource);
     QDeclarativePositionSource *positionSource() const;
 
-    QNavigationManager *navigationManager() const;
     bool navigatorReady() const;
 
     void setTrackPositionSource(bool trackPositionSource);
diff --git a/src/location/labs/qdeclarativenavigator_p_p.h b/src/location/labs/qdeclarativenavigator_p_p.h
index 3710054e..6c07a0ca 100644
--- a/src/location/labs/qdeclarativenavigator_p_p.h
+++ b/src/location/labs/qdeclarativenavigator_p_p.h
@@ -63,23 +63,29 @@ class QDeclarativePositionSource;
 class QGeoMapParameter;
 class QDeclarativeGeoRouteSegment;
 class QParameterizableObject;
+class QAbstractNavigator;
 
-class Q_LOCATION_PRIVATE_EXPORT QDeclarativeNavigatorPrivate
+class Q_LOCATION_PRIVATE_EXPORT QDeclarativeNavigatorParams
+{
+public:
+    QPointer<QDeclarativeGeoMap> m_map;
+    QPointer<QDeclarativeGeoRoute> m_route;
+    QGeoRoute m_geoRoute;
+    QPointer<QDeclarativePositionSource> m_positionSource;
+    QList<QPointer<QGeoMapParameter>> m_parameters;
+};
+
+class QDeclarativeNavigatorPrivate
 {
 public:
     QDeclarativeNavigatorPrivate(QParameterizableObject *q_);
 
     void updateReadyState();
-
     QParameterizableObject *q = nullptr;
-    QNavigationManager *m_navigationManager = nullptr;
+    QSharedPointer<QDeclarativeNavigatorParams> m_params;
+    QScopedPointer<QAbstractNavigator> m_navigator;
     QDeclarativeGeoServiceProvider *m_plugin = nullptr;
-    QPointer<QDeclarativeGeoMap> m_map;
-    QPointer<QDeclarativeGeoRoute> m_route;
-    QGeoRoute m_geoRoute;
-    QPointer<QDeclarativePositionSource> m_positionSource;
     QPointer<QDeclarativeGeoRoute> m_currentRoute;
-    QList<QGeoMapParameter *> m_parameters;
     int m_currentSegment = 0;
     bool m_active = false;
     bool m_completed = false;
diff --git a/src/location/maps/qnavigationmanager.cpp b/src/location/maps/qnavigationmanager.cpp
index 5372e3a5..5cae6a04 100644
--- a/src/location/maps/qnavigationmanager.cpp
+++ b/src/location/maps/qnavigationmanager.cpp
@@ -46,8 +46,6 @@ public:
     ~QNavigationManagerPrivate();
 
     QNavigationManagerEngine *engine = nullptr;
-    QDeclarativeNavigatorPrivate *navigator = nullptr;
-    QList<QGeoMapParameter *> parameters;
 
 private:
     Q_DISABLE_COPY(QNavigationManagerPrivate)
@@ -89,16 +87,6 @@ bool QNavigationManager::isInitialized() const
     return d_ptr->engine->isInitialized();
 }
 
-void QNavigationManager::setNavigator(QDeclarativeNavigatorPrivate *navigator)
-{
-    d_ptr->navigator = navigator;
-}
-
-QDeclarativeNavigatorPrivate *QNavigationManager::declarativeNavigator() const
-{
-    return d_ptr->navigator;
-}
-
 void QNavigationManager::setLocale(const QLocale &locale)
 {
     d_ptr->engine->setLocale(locale);
@@ -109,34 +97,19 @@ QLocale QNavigationManager::locale() const
     return d_ptr->engine->locale();
 }
 
-void QNavigationManager::setParameters(const QList<QGeoMapParameter *> &parameters)
-{
-    d_ptr->parameters = parameters;
-}
-
-QList<QGeoMapParameter *> QNavigationManager::parameters() const
-{
-    return d_ptr->parameters;
-}
-
-bool QNavigationManager::ready() const
-{
-    return d_ptr->engine->ready(*d_ptr->navigator, d_ptr->parameters);
-}
-
-bool QNavigationManager::start()
+void QNavigationManager::setMeasurementSystem(QLocale::MeasurementSystem system)
 {
-    return d_ptr->engine->start(*d_ptr->navigator, d_ptr->parameters);
+    d_ptr->engine->setMeasurementSystem(system);
 }
 
-bool QNavigationManager::stop()
+QLocale::MeasurementSystem QNavigationManager::measurementSystem() const
 {
-    return d_ptr->engine->stop(*d_ptr->navigator);
+    return d_ptr->engine->measurementSystem();
 }
 
-bool QNavigationManager::active() const
+QAbstractNavigator *QNavigationManager::createNavigator(const QSharedPointer<QDeclarativeNavigatorParams> &navigator)
 {
-    return d_ptr->engine->active(*d_ptr->navigator);
+    return d_ptr->engine->createNavigator(navigator);
 }
 
 QNavigationManager::QNavigationManager(QNavigationManagerEngine *engine, QObject *parent) : QObject(parent),
diff --git a/src/location/maps/qnavigationmanager_p.h b/src/location/maps/qnavigationmanager_p.h
index 1d8c1722..544e15f5 100644
--- a/src/location/maps/qnavigationmanager_p.h
+++ b/src/location/maps/qnavigationmanager_p.h
@@ -51,6 +51,7 @@
 #include <QObject>
 #include <QSize>
 #include <QPair>
+#include <QLocale>
 #include <QtLocation/private/qlocationglobal_p.h>
 #include <QtLocation/private/qgeomapparameter_p.h>
 
@@ -58,11 +59,11 @@ QT_BEGIN_NAMESPACE
 
 class QNavigationManagerEngine;
 class QNavigationManagerPrivate;
-class QDeclarativeNavigatorPrivate;
+class QDeclarativeNavigatorParams;
 class QDeclarativeGeoWaypoint;
 class QGeoRoute;
 class QGeoRouteSegment;
-
+class QAbstractNavigator;
 class Q_LOCATION_PRIVATE_EXPORT QNavigationManager : public QObject
 {
     Q_OBJECT
@@ -75,32 +76,19 @@ public:
     QNavigationManagerEngine *engine();
     bool isInitialized() const;
 
-    void setNavigator(QDeclarativeNavigatorPrivate *navigator);
-    QDeclarativeNavigatorPrivate *declarativeNavigator() const;
-
     void setLocale(const QLocale &locale);
     QLocale locale() const;
+    void setMeasurementSystem(QLocale::MeasurementSystem system);
+    QLocale::MeasurementSystem measurementSystem() const;
 
-    void setParameters(const QList<QGeoMapParameter *> &parameters);
-    QList<QGeoMapParameter *> parameters() const;
-
-    bool ready() const;
-    bool start();
-    bool stop();
-    bool active() const;
+    QAbstractNavigator *createNavigator(const QSharedPointer<QDeclarativeNavigatorParams> &navigator);
 
 Q_SIGNALS:
     void initialized();
 
-    // These must be emitted by the engine
-    void activeChanged(bool active);
-    void waypointReached(const QDeclarativeGeoWaypoint *pos);
-    void destinationReached();
-    void currentRouteChanged(const QGeoRoute &route);
-    void currentSegmentChanged(int segment);
 
 protected:
-    QNavigationManager(QNavigationManagerEngine *engine, QObject *parent = 0);
+    QNavigationManager(QNavigationManagerEngine *engine, QObject *parent = nullptr);
 
 private:
     QNavigationManagerPrivate *d_ptr;
diff --git a/src/location/maps/qnavigationmanagerengine.cpp b/src/location/maps/qnavigationmanagerengine.cpp
index b9191e74..1f82b4a1 100644
--- a/src/location/maps/qnavigationmanagerengine.cpp
+++ b/src/location/maps/qnavigationmanagerengine.cpp
@@ -49,9 +49,47 @@ public:
     bool initialized = false;
 };
 
+class QAbstractNavigatorPrivate
+{
+public:
+    QLocale locale;
+    QLocale::MeasurementSystem measurementSystem;
+    bool initialized = false;
+};
+
+QAbstractNavigator::QAbstractNavigator(QObject *parent)
+    : QObject(parent)
+    , d(new QAbstractNavigatorPrivate)
+{
+}
+
+QAbstractNavigator::~QAbstractNavigator()
+{
+}
+
+void QAbstractNavigator::setLocale(const QLocale &locale)
+{
+    d->locale = locale;
+}
+
+QLocale QAbstractNavigator::locale() const
+{
+    return  d->locale;
+}
+
+void QAbstractNavigator::setMeasurementSystem(QLocale::MeasurementSystem system)
+{
+    d->measurementSystem = system;
+}
+
+QLocale::MeasurementSystem QAbstractNavigator::measurementSystem() const
+{
+    return d->measurementSystem;
+}
+
 QNavigationManagerEngine::QNavigationManagerEngine(const QVariantMap &parameters, QObject *parent)
     : QObject(parent)
-    , d_ptr(new QNavigationManagerEnginePrivate)
+    , d(new QNavigationManagerEnginePrivate)
 {
     Q_UNUSED(parameters)
 }
@@ -62,64 +100,52 @@ QNavigationManagerEngine::~QNavigationManagerEngine()
 
 void QNavigationManagerEngine::setManagerName(const QString &name)
 {
-    d_ptr->managerName = name;
+    d->managerName = name;
 }
 
 QString QNavigationManagerEngine::managerName() const
 {
-    return d_ptr->managerName;
+    return d->managerName;
 }
 
 void QNavigationManagerEngine::setManagerVersion(int version)
 {
-    d_ptr->managerVersion = version;
+    d->managerVersion = version;
 }
 
 int QNavigationManagerEngine::managerVersion() const
 {
-    return d_ptr->managerVersion;
+    return d->managerVersion;
 }
 
 void QNavigationManagerEngine::setLocale(const QLocale &locale)
 {
-    d_ptr->locale = locale;
+    d->locale = locale;
 }
 
 QLocale QNavigationManagerEngine::locale() const
 {
-    return d_ptr->locale;
+    return d->locale;
 }
 
 void QNavigationManagerEngine::setMeasurementSystem(QLocale::MeasurementSystem system)
 {
-    d_ptr->measurementSystem = system;
+    d->measurementSystem = system;
 }
 
 QLocale::MeasurementSystem QNavigationManagerEngine::measurementSystem() const
 {
-    return d_ptr->measurementSystem;
+    return d->measurementSystem;
 }
 
 bool QNavigationManagerEngine::isInitialized() const
 {
-    return d_ptr->initialized;
-}
-
-// Subclasses are supposed to emit activeChanged from here.
-bool QNavigationManagerEngine::start(QDeclarativeNavigatorPrivate & /*navigator*/, const QList<QGeoMapParameter*> & /*navigationParams*/)
-{
-
-    return false;
-}
-
-bool QNavigationManagerEngine::stop(QDeclarativeNavigatorPrivate & /*navigator*/) // navigator needed to find the right navi session to stop.
-{
-    return false;
+    return d->initialized;
 }
 
 void QNavigationManagerEngine::engineInitialized()
 {
-    d_ptr->initialized = true;
+    d->initialized = true;
     emit initialized();
 }
 
diff --git a/src/location/maps/qnavigationmanagerengine_p.h b/src/location/maps/qnavigationmanagerengine_p.h
index 803050c5..62d9eb87 100644
--- a/src/location/maps/qnavigationmanagerengine_p.h
+++ b/src/location/maps/qnavigationmanagerengine_p.h
@@ -55,39 +55,67 @@
 
 QT_BEGIN_NAMESPACE
 
+class QAbstractNavigatorPrivate;
 class QGeoMap;
 class QGeoMapParameter;
 class QMapRouteObject;
+class QGeoRoute;
+class QNavigationManager;
 class QNavigationManagerEnginePrivate;
-class QDeclarativeNavigatorPrivate;
+class QDeclarativeNavigatorParams;
+class QDeclarativeGeoWaypoint;
+
+class Q_LOCATION_PRIVATE_EXPORT QAbstractNavigator: public QObject
+{
+    Q_OBJECT
+public:
+    QAbstractNavigator(QObject *parent = nullptr);
+    ~QAbstractNavigator() override;
+    virtual void setLocale(const QLocale &locale);
+    virtual QLocale locale() const;
+    virtual void setMeasurementSystem(QLocale::MeasurementSystem system);
+    virtual QLocale::MeasurementSystem measurementSystem() const;
+    virtual bool active() const = 0;
+    virtual bool ready() const = 0;
+
+public slots:
+    virtual bool start() = 0;
+    virtual bool stop() = 0;
+
+signals:
+    // These must be emitted by the engine
+    void activeChanged(bool active);
+    void waypointReached(const QDeclarativeGeoWaypoint *pos);
+    void destinationReached();
+    void currentRouteChanged(const QGeoRoute &route);
+    void currentSegmentChanged(int segment);
+
+private:
+    QScopedPointer<QAbstractNavigatorPrivate> d;
+};
 
 class Q_LOCATION_PRIVATE_EXPORT QNavigationManagerEngine : public QObject
 {
     Q_OBJECT
 public:
     explicit QNavigationManagerEngine(const QVariantMap &parameters, QObject *parent = nullptr);
-    virtual ~QNavigationManagerEngine();
+    ~QNavigationManagerEngine() override;
 
     void setManagerName(const QString &name);
     QString managerName() const;
     void setManagerVersion(int version);
     int managerVersion() const;
-
     virtual void setLocale(const QLocale &locale);
     virtual QLocale locale() const;
     virtual void setMeasurementSystem(QLocale::MeasurementSystem system);
     virtual QLocale::MeasurementSystem measurementSystem() const;
+
     virtual bool isInitialized() const;
-    virtual bool ready(const QDeclarativeNavigatorPrivate &navigator, const QList<QGeoMapParameter*> &navigationParams) = 0;
-    virtual bool active(const QDeclarativeNavigatorPrivate &navigator) = 0;
+    virtual QAbstractNavigator *createNavigator(const QSharedPointer<QDeclarativeNavigatorParams> &navigator) = 0;
 
 signals:
     void initialized();
 
-public slots:
-    virtual bool start(QDeclarativeNavigatorPrivate &navigator, const QList<QGeoMapParameter*> &navigationParams);
-    virtual bool stop(QDeclarativeNavigatorPrivate &navigator);
-
 protected:
     /*!
         Marks the engine as initialized. Subclasses of QGeoMappingManagerEngine are to
@@ -96,7 +124,7 @@ protected:
     */
     virtual void engineInitialized();
 
-    QScopedPointer<QNavigationManagerEnginePrivate> d_ptr;
+    QScopedPointer<QNavigationManagerEnginePrivate> d;
 };
 
 QT_END_NAMESPACE