Improvements to geoclue2 plugin

Relies on automatic service activation:
https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

This means that the service isn't explicitly started but gets started
automatically when a request to the service is made. It's also possible
that the geoclue2 service gets shutdown during runtime (for example when
no client is requesting updates for some time). Automatic service
activation ensures, that the service gets (re)started whenever needed.

Create the manager interface right in the constructor as there's no
benefit in creating it lazily. Also, since automatic service activation
is used, m_manager->isValid() can't be used, because it always returns
false until the service has been (automatically) started.

Always return correct values from supportedPositioningMethods(). The use
of the m_manager->availableAccuracyLevel() shorthand helper doesn't tell
the difference between an invalid property (for example if geoclue2
isn't available at all) and GCLUE_ACCURACY_LEVEL_NONE since both return
0. Getting the QVariant of the property is needed to distinguish those
cases. An invalid QVariant means that we couldn't communicate to
geoclue2. A QVariant that can be turned into 0 means
GCLUE_ACCURACY_LEVEL_NONE. In this commit GCLUE_ACCURACY_LEVEL_NONE is
also correctly turned into NoPositioningMethods.

If client doesn't exist when startClient() is called, create a client
first. It's also possible that a client is crerated more than once
during runtime. For example, when the above mentioned automatic geoclue2
service shutdown takes place, the old client is no longer usable. Now
in this case the old client gets discarded and a new client is created.

Call startClient() in startUpdates() – because otherwise startUpdates()
wouldn't really work.

Remove the warning message from the beginning of configureClient(). It's
quite OK that configureClient() gets called before m_client exists (it
just doesn't do anything then). This was probably in place with the
assumption that setUpdateInterval(), setPreferredPositioningMethods()
and startUpdates() would get called in some specific order. But no such
assumptions can be made.

Delete client interface object once it's not usable anymore (either
gives an error or is explicitly stopped). This way, a new client gets
created once a client is needed again.

Other minor changes:
- In startClient(), check whether starting the client is needed before
  doing it. This removes the need to do any checks before calling this
  function.
- In stopClient(), check whether stopping the client is OK before doing
  it. This removes the need to do any checks before calling this
  function.
- Use QPointer for the D-Bus interface objects -> no need to set the
  pointers to nullptr after deletion.
- Add explicit casts to few places that gave implicit signedness change
  warnings.

Change-Id: If9db25775710fe64f272db222030e481533d9732
Reviewed-by: Volker Krause <volker.krause@kdab.com>
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

2 files changed, 48 insertions, 80 deletions

diff --git a/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2.cpp b/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2.cpp
index 1e457437..cd514d30 100644
--- a/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2.cpp
+++ b/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2.cpp
@@ -48,7 +48,6 @@
 // Auto-generated D-Bus files.
 #include <client_interface.h>
 #include <location_interface.h>
-#include <manager_interface.h>
 
 Q_DECLARE_LOGGING_CATEGORY(lcPositioningGeoclue2)
 
@@ -81,6 +80,10 @@ static QString lastPositionFilePath()
 QGeoPositionInfoSourceGeoclue2::QGeoPositionInfoSourceGeoclue2(QObject *parent)
     : QGeoPositionInfoSource(parent)
     , m_requestTimer(new QTimer(this))
+    , m_manager(QLatin1String(GEOCLUE2_SERVICE_NAME),
+                QStringLiteral("/org/freedesktop/GeoClue2/Manager"),
+                QDBusConnection::systemBus(),
+                this)
 {
     qDBusRegisterMetaType<Timestamp>();
 
@@ -89,25 +92,6 @@ QGeoPositionInfoSourceGeoclue2::QGeoPositionInfoSourceGeoclue2(QObject *parent)
     m_requestTimer->setSingleShot(true);
     connect(m_requestTimer, &QTimer::timeout,
             this, &QGeoPositionInfoSourceGeoclue2::requestUpdateTimeout);
-
-    const auto flags = QDBusServiceWatcher::WatchForRegistration;
-    const auto serviceWatcher = new QDBusServiceWatcher(
-                QLatin1String(GEOCLUE2_SERVICE_NAME),
-                QDBusConnection::systemBus(),
-                flags,
-                this);
-    connect(serviceWatcher, &QDBusServiceWatcher::serviceRegistered,
-            this, &QGeoPositionInfoSourceGeoclue2::createClient);
-
-    if (const auto iface = QDBusConnection::systemBus().interface()) {
-        if (iface->isServiceRegistered(QLatin1String(GEOCLUE2_SERVICE_NAME)))
-            createClient(QLatin1String(GEOCLUE2_SERVICE_NAME));
-        else
-            iface->startService(QLatin1String(GEOCLUE2_SERVICE_NAME));
-    } else {
-        qCCritical(lcPositioningGeoclue2) << "D-Bus connection interface is not exists";
-        setError(AccessError);
-    }
 }
 
 QGeoPositionInfoSourceGeoclue2::~QGeoPositionInfoSourceGeoclue2()
@@ -130,11 +114,25 @@ QGeoPositionInfo QGeoPositionInfoSourceGeoclue2::lastKnownPosition(bool fromSate
 
 QGeoPositionInfoSourceGeoclue2::PositioningMethods QGeoPositionInfoSourceGeoclue2::supportedPositioningMethods() const
 {
-    if (m_manager) {
-        return m_manager->availableAccuracyLevel() == GCLUE_ACCURACY_LEVEL_EXACT
-            ? AllPositioningMethods : NonSatellitePositioningMethods;
+    bool ok;
+    const auto accuracy = m_manager.property("AvailableAccuracyLevel").toUInt(&ok);
+    if (!ok) {
+        const_cast<QGeoPositionInfoSourceGeoclue2 *>(this)->setError(AccessError);
+        return NoPositioningMethods;
+    }
+
+    switch (accuracy) {
+    case GCLUE_ACCURACY_LEVEL_COUNTRY:
+    case GCLUE_ACCURACY_LEVEL_CITY:
+    case GCLUE_ACCURACY_LEVEL_NEIGHBORHOOD:
+    case GCLUE_ACCURACY_LEVEL_STREET:
+        return NonSatellitePositioningMethods;
+    case GCLUE_ACCURACY_LEVEL_EXACT:
+        return AllPositioningMethods;
+    case GCLUE_ACCURACY_LEVEL_NONE:
+    default:
+        return NoPositioningMethods;
     }
-    return NoPositioningMethods;
 }
 
 void QGeoPositionInfoSourceGeoclue2::setPreferredPositioningMethods(PositioningMethods methods)
@@ -163,6 +161,8 @@ void QGeoPositionInfoSourceGeoclue2::startUpdates()
     qCDebug(lcPositioningGeoclue2) << "Starting updates";
     m_running = true;
 
+    startClient();
+
     if (m_lastPosition.isValid()) {
         QMetaObject::invokeMethod(this, "positionUpdated", Qt::QueuedConnection,
                                   Q_ARG(QGeoPositionInfo, m_lastPosition));
@@ -179,9 +179,7 @@ void QGeoPositionInfoSourceGeoclue2::stopUpdates()
     qCDebug(lcPositioningGeoclue2) << "Stopping updates";
     m_running = false;
 
-    // Only stop positioning if single update not requested.
-    if (!m_requestTimer->isActive() && m_client)
-        stopClient();
+    stopClient();
 }
 
 void QGeoPositionInfoSourceGeoclue2::requestUpdate(int timeout)
@@ -197,8 +195,7 @@ void QGeoPositionInfoSourceGeoclue2::requestUpdate(int timeout)
     }
 
     m_requestTimer->start(timeout ? timeout : UPDATE_TIMEOUT_COLD_START);
-    if (!m_running)
-        startClient();
+    startClient();
 }
 
 void QGeoPositionInfoSourceGeoclue2::setError(QGeoPositionInfoSource::Error error)
@@ -236,35 +233,9 @@ void QGeoPositionInfoSourceGeoclue2::saveLastPosition()
 #endif
 }
 
-void QGeoPositionInfoSourceGeoclue2::createClient(const QString &service)
+void QGeoPositionInfoSourceGeoclue2::createClient()
 {
-    if (service != QLatin1String(GEOCLUE2_SERVICE_NAME)) {
-        qCCritical(lcPositioningGeoclue2) << "Registered unexpected service:"
-                                          << service << ", expected:"
-                                          << GEOCLUE2_SERVICE_NAME;
-        setError(UnknownSourceError);
-        return;
-    }
-
-    if (!m_manager) {
-        m_manager = new OrgFreedesktopGeoClue2ManagerInterface(
-                    QLatin1String(GEOCLUE2_SERVICE_NAME),
-                    QStringLiteral("/org/freedesktop/GeoClue2/Manager"),
-                    QDBusConnection::systemBus(),
-                    this);
-    }
-
-    if (!m_manager->isValid()) {
-        const auto error = m_manager->lastError();
-        qCCritical(lcPositioningGeoclue2) << "Unable to create the manager object:"
-                                          << error.name() << error.message();
-        setError(AccessError);
-        delete m_manager;
-        m_manager = nullptr;
-        return;
-    }
-
-    const QDBusPendingReply<QDBusObjectPath> reply = m_manager->GetClient();
+    const QDBusPendingReply<QDBusObjectPath> reply = m_manager.GetClient();
     const auto watcher = new QDBusPendingCallWatcher(reply, this);
     connect(watcher, &QDBusPendingCallWatcher::finished,
             [this](QDBusPendingCallWatcher *watcher) {
@@ -292,13 +263,11 @@ void QGeoPositionInfoSourceGeoclue2::createClient(const QString &service)
                                                   << error.name() << error.message();
                 setError(AccessError);
                 delete m_client;
-                m_client = nullptr;
             } else {
                 connect(m_client, &OrgFreedesktopGeoClue2ClientInterface::LocationUpdated,
                         this, &QGeoPositionInfoSourceGeoclue2::handleNewLocation);
 
-                // only start the client if someone asked for it already
-                if (configureClient() && (m_running || m_requestTimer->isActive()))
+                if (configureClient())
                     startClient();
             }
         }
@@ -307,9 +276,12 @@ void QGeoPositionInfoSourceGeoclue2::createClient(const QString &service)
 
 void QGeoPositionInfoSourceGeoclue2::startClient()
 {
+    // only start the client if someone asked for it already
+    if (!m_running && !m_requestTimer->isActive())
+        return;
+
     if (!m_client) {
-        qCWarning(lcPositioningGeoclue2) << "Unable to start the client "
-                                            "due to it is not created yet";
+        createClient();
         return;
     }
 
@@ -325,6 +297,7 @@ void QGeoPositionInfoSourceGeoclue2::startClient()
             qCCritical(lcPositioningGeoclue2) << "Unable to start the client:"
                                               << error.name() << error.message();
             setError(AccessError);
+            delete m_client;
         } else {
             qCDebug(lcPositioningGeoclue2) << "Client successfully started";
 
@@ -340,11 +313,9 @@ void QGeoPositionInfoSourceGeoclue2::startClient()
 
 void QGeoPositionInfoSourceGeoclue2::stopClient()
 {
-    if (!m_client) {
-        qCWarning(lcPositioningGeoclue2) << "Unable to stop the client "
-                                            "due to it is not created yet";
+    // Only stop client if updates are no longer wanted.
+    if (m_requestTimer->isActive() || m_running || !m_client)
         return;
-    }
 
     const QDBusPendingReply<> reply = m_client->Stop();
     const auto watcher = new QDBusPendingCallWatcher(reply, this);
@@ -361,16 +332,14 @@ void QGeoPositionInfoSourceGeoclue2::stopClient()
         } else {
             qCDebug(lcPositioningGeoclue2) << "Client successfully stopped";
         }
+        delete m_client;
     });
 }
 
 bool QGeoPositionInfoSourceGeoclue2::configureClient()
 {
-    if (!m_client) {
-        qCWarning(lcPositioningGeoclue2) << "Unable to configure the client "
-                                            "due to it is not created yet";
+    if (!m_client)
         return false;
-    }
 
     auto desktopId = QString::fromUtf8(qgetenv("QT_GEOCLUE_APP_DESKTOP_ID"));
     if (desktopId.isEmpty())
@@ -387,7 +356,7 @@ bool QGeoPositionInfoSourceGeoclue2::configureClient()
     m_client->setDesktopId(desktopId);
 
     const auto msecs = updateInterval();
-    const auto secs = msecs / 1000;
+    const uint secs = qMax(uint(msecs), 0u) / 1000u;
     m_client->setTimeThreshold(secs);
 
     const auto methods = preferredPositioningMethods();
@@ -415,8 +384,7 @@ void QGeoPositionInfoSourceGeoclue2::requestUpdateTimeout()
 
     emit updateTimeout();
 
-    if (!m_running)
-        stopClient();
+    stopClient();
 }
 
 void QGeoPositionInfoSourceGeoclue2::handleNewLocation(const QDBusObjectPath &oldLocation,
@@ -450,7 +418,7 @@ void QGeoPositionInfoSourceGeoclue2::handleNewLocation(const QDBusObjectPath &ol
             const auto dt = QDateTime::currentDateTime();
             m_lastPosition = QGeoPositionInfo(coordinate, dt);
         } else {
-            auto dt = QDateTime::fromSecsSinceEpoch(ts.m_seconds);
+            auto dt = QDateTime::fromSecsSinceEpoch(qint64(ts.m_seconds));
             dt = dt.addMSecs(ts.m_microseconds / 1000);
             m_lastPosition = QGeoPositionInfo(coordinate, dt);
         }
@@ -469,8 +437,7 @@ void QGeoPositionInfoSourceGeoclue2::handleNewLocation(const QDBusObjectPath &ol
         qCDebug(lcPositioningGeoclue2) << "New position:" << m_lastPosition;
     }
 
-    if (!m_running)
-        stopClient();
+    stopClient();
 }
 
 QT_END_NAMESPACE
diff --git a/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2_p.h b/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2_p.h
index 1ccd7290..16f5b9a1 100644
--- a/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2_p.h
+++ b/src/plugins/position/geoclue2/qgeopositioninfosource_geoclue2_p.h
@@ -41,9 +41,10 @@
 #define QGEOPOSITIONINFOSOURCE_GEOCLUE2_P_H
 
 #include <QtPositioning/QGeoPositionInfoSource>
+#include <QtCore/QPointer>
+#include <manager_interface.h>
 
 class OrgFreedesktopGeoClue2ClientInterface;
-class OrgFreedesktopGeoClue2ManagerInterface;
 
 QT_BEGIN_NAMESPACE
 class QDBusObjectPath;
@@ -74,7 +75,7 @@ private:
     void setError(QGeoPositionInfoSource::Error error);
     void restoreLastPosition();
     void saveLastPosition();
-    void createClient(const QString &service);
+    void createClient();
     bool configureClient();
     void startClient();
     void stopClient();
@@ -83,8 +84,8 @@ private:
                            const QDBusObjectPath &newLocation);
 
     QTimer *m_requestTimer = nullptr;
-    OrgFreedesktopGeoClue2ManagerInterface *m_manager = nullptr;
-    OrgFreedesktopGeoClue2ClientInterface *m_client = nullptr;
+    OrgFreedesktopGeoClue2ManagerInterface m_manager;
+    QPointer<OrgFreedesktopGeoClue2ClientInterface> m_client;
     bool m_running = false;
     bool m_lastPositionFromSatellite = false;
     QGeoPositionInfoSource::Error m_error = NoError;