Merge lp:~pete-woods/indicator-network/connection-manager-improvements into lp:indicator-network

[Pete Woods]

Use high-level C++ ActiveConnectionManager class for activating and deactivating connections

- Add activate and deactivate methods to ActiveConnectionManager.
- Improve ActiveConnectionManager so that its state is up to date as soon as activate/deactivate are called.
- Use ActiveConnectionManager across the whole codebase for activating and deactivating connections.

Use high-level C++ ActiveConnectionManager class for activating and deactivating connections

- Add activate and deactivate methods to ActiveConnectionManager.
- Improve ActiveConnectionManager so that its state is up to date as soon as activate/deactivate are called.
- Use ActiveConnectionManager across the whole codebase for activating and deactivating connections.

[Xavi Garcia (xavi-garcia-mena)]

Looks good to me.
In fact the diff looks bigger than it really is.
I don't see anything weird.

I've looked at the code and the full files modified in the branch, but I'm not an expert in indicator-network.

From a point of view of managing the active connections it looks good.
Maybe it would be good to have another pair of eyes on this...

review: Approve

[Charles Kerr (charlesk)]

LGTM as well. Most of this looks like straightforward consolidation into ACM.

review: Approve

=== modified file 'src/indicator/nmofono/connection/active-connection-manager.cpp'
--- src/indicator/nmofono/connection/active-connection-manager.cpp	2016-12-01 11:40:00 +0000
+++ src/indicator/nmofono/connection/active-connection-manager.cpp	2016-12-13 14:49:16 +0000
@@ -67,6 +67,26 @@
         }
     }
 
+    ActiveConnection::SPtr addConnection(const QDBusObjectPath& path)
+    {
+        ActiveConnection::SPtr connection;
+
+        auto it = m_connections.constFind(path);
+        if (it == m_connections.constEnd())
+        {
+            connection = make_shared<ActiveConnection>(path, m_manager->connection());
+            m_connections.insert(path, connection);
+            Q_EMIT p.connectionsChanged(m_connections.values().toSet());
+            Q_EMIT p.connectionsUpdated();
+        }
+        else
+        {
+            connection = *it;
+        }
+
+        return connection;
+    }
+
 public Q_SLOTS:
     void propertiesChanged(const QVariantMap &properties)
     {
@@ -111,7 +131,17 @@
 
 ActiveConnection::SPtr ActiveConnectionManager::connection(const QDBusObjectPath& path) const
 {
-    return d->m_connections.value(path);
+    auto connection = d->m_connections.value(path);
+    if (path.path() != "/" && !connection)
+    {
+        d->updateConnections(d->m_manager->activeConnections());
+        connection = d->m_connections.value(path);
+        if (!connection)
+        {
+            qWarning() << "Could not find connection at path" << path.path();
+        }
+    }
+    return connection;
 }
 
 bool ActiveConnectionManager::deactivate(ActiveConnection::SPtr activeConnection)
@@ -126,6 +156,31 @@
     return true;
 }
 
+ActiveConnection::SPtr ActiveConnectionManager::activate(const QDBusObjectPath& connection, const QDBusObjectPath& device, const QDBusObjectPath& specificObject)
+{
+    auto reply = d->m_manager->ActivateConnection(connection, device, specificObject);
+    reply.waitForFinished();
+    if (reply.isError())
+    {
+        qWarning() << reply.error().message();
+        return ActiveConnection::SPtr();
+    }
+    return d->addConnection(reply);
+}
+
+
+ActiveConnection::SPtr ActiveConnectionManager::addAndActivate(const QVariantDictMap &connection, const QDBusObjectPath &device, const QDBusObjectPath &specificObject)
+{
+    auto reply = d->m_manager->AddAndActivateConnection(connection, device, specificObject);
+    reply.waitForFinished();
+    if (reply.isError())
+    {
+        qWarning() << reply.error().message();
+        return ActiveConnection::SPtr();
+    }
+    return d->addConnection(reply.argumentAt<1>());
+}
+
 }
 }
 

=== modified file 'src/indicator/nmofono/connection/active-connection-manager.h'
--- src/indicator/nmofono/connection/active-connection-manager.h	2016-12-01 11:40:00 +0000
+++ src/indicator/nmofono/connection/active-connection-manager.h	2016-12-13 14:49:16 +0000
@@ -23,6 +23,7 @@
 #include <QSet>
 
 #include <nmofono/connection/active-connection.h>
+#include <dbus-types.h>
 
 namespace nmofono
 {
@@ -46,6 +47,10 @@
 
     bool deactivate(ActiveConnection::SPtr activeConnection);
 
+    ActiveConnection::SPtr activate(const QDBusObjectPath& connection, const QDBusObjectPath& device = QDBusObjectPath("/"), const QDBusObjectPath& specificObject = QDBusObjectPath("/"));
+
+    ActiveConnection::SPtr addAndActivate(const QVariantDictMap &connection, const QDBusObjectPath &device, const QDBusObjectPath &specificObject);
+
 Q_SIGNALS:
     void connectionsChanged(const QSet<ActiveConnection::SPtr>& connections);
 

=== modified file 'src/indicator/nmofono/ethernet/ethernet-link.cpp'
--- src/indicator/nmofono/ethernet/ethernet-link.cpp	2016-12-05 11:07:31 +0000
+++ src/indicator/nmofono/ethernet/ethernet-link.cpp	2016-12-13 14:49:16 +0000
@@ -211,8 +211,6 @@
 
     shared_ptr<OrgFreedesktopNetworkManagerDeviceInterface> m_dev;
 
-    shared_ptr<OrgFreedesktopNetworkManagerInterface> m_nm;
-
     shared_ptr<OrgFreedesktopNetworkManagerSettingsInterface> m_settings;
 
     shared_ptr<OrgFreedesktopNetworkManagerDeviceWiredInterface> m_wired;
@@ -234,11 +232,10 @@
     bool m_autoConnect = false;
 };
 
-EthernetLink::EthernetLink(shared_ptr<OrgFreedesktopNetworkManagerDeviceInterface> dev, shared_ptr<OrgFreedesktopNetworkManagerInterface> nm, shared_ptr<OrgFreedesktopNetworkManagerSettingsInterface> settings, ActiveConnectionManager::SPtr connectionManager) :
+EthernetLink::EthernetLink(shared_ptr<OrgFreedesktopNetworkManagerDeviceInterface> dev, shared_ptr<OrgFreedesktopNetworkManagerSettingsInterface> settings, ActiveConnectionManager::SPtr connectionManager) :
         d(new Private(*this))
 {
     d->m_dev = dev;
-    d->m_nm = nm;
     d->m_settings = settings;
     d->m_connectionManager = connectionManager;
     d->m_devicePropertyCache = make_shared<util::DBusPropertyCache>(NM_DBUS_SERVICE, NM_DBUS_INTERFACE_DEVICE, dev->path(), dev->connection());
@@ -321,12 +318,7 @@
     {
         if (d->m_preferredConnection)
         {
-            auto reply = d->m_nm->ActivateConnection(d->m_preferredConnection->path(), QDBusObjectPath(d->m_dev->path()), QDBusObjectPath("/"));
-            reply.waitForFinished();
-            if (reply.isError())
-            {
-                qWarning() << reply.error().message();
-            }
+            d->m_connectionManager->activate(d->m_preferredConnection->path(), QDBusObjectPath(d->m_dev->path()));
         }
         d->m_dev->setAutoconnect(true);
     }
@@ -356,12 +348,7 @@
 
     if (d->m_autoConnect && preferredConnection)
     {
-        auto reply = d->m_nm->ActivateConnection(d->m_preferredConnection->path(), QDBusObjectPath(d->m_dev->path()), QDBusObjectPath("/"));
-        reply.waitForFinished();
-        if (reply.isError())
-        {
-            qWarning() << reply.error().message();
-        }
+        d->m_connectionManager->activate(d->m_preferredConnection->path(), QDBusObjectPath(d->m_dev->path()));
     }
 
     Q_EMIT preferredConnectionChanged(preferredConnection);

=== modified file 'src/indicator/nmofono/ethernet/ethernet-link.h'
--- src/indicator/nmofono/ethernet/ethernet-link.h	2016-12-01 11:40:00 +0000
+++ src/indicator/nmofono/ethernet/ethernet-link.h	2016-12-13 14:49:16 +0000
@@ -46,7 +46,6 @@
     UNITY_DEFINES_PTRS(EthernetLink);
 
     EthernetLink(std::shared_ptr<OrgFreedesktopNetworkManagerDeviceInterface> dev,
-                 std::shared_ptr<OrgFreedesktopNetworkManagerInterface> nm,
                  std::shared_ptr<OrgFreedesktopNetworkManagerSettingsInterface> settings,
                  nmofono::connection::ActiveConnectionManager::SPtr connectionManager);
 

=== modified file 'src/indicator/nmofono/hotspot-manager.cpp'
--- src/indicator/nmofono/hotspot-manager.cpp	2016-05-26 13:51:39 +0000
+++ src/indicator/nmofono/hotspot-manager.cpp	2016-12-13 14:49:16 +0000
@@ -108,21 +108,15 @@
 
     bool activateConnection(const QDBusObjectPath& device)
     {
-        auto reply = m_manager->ActivateConnection(
-                        QDBusObjectPath(m_hotspot->path()), device,
-                        QDBusObjectPath("/"));
-        reply.waitForFinished();
-        if (reply.isError())
+        auto activeConnectionObject = m_activeConnectionManager->activate(QDBusObjectPath(m_hotspot->path()), device);
+        if (!activeConnectionObject)
         {
-            qCritical() << "Could not activate hotspot connection"
-                    << reply.error().message();
+            qCritical() << "Could not activate hotspot connection";
             return false;
         }
 
-        QDBusObjectPath activeConnectionPath(reply);
-
         OrgFreedesktopNetworkManagerConnectionActiveInterface activeConnection (
-                    NM_DBUS_SERVICE, activeConnectionPath.path (),
+                    NM_DBUS_SERVICE, activeConnectionObject->path().path (),
                     m_manager->connection());
 
         int count = 0;

=== modified file 'src/indicator/nmofono/manager-impl.cpp'
--- src/indicator/nmofono/manager-impl.cpp	2016-12-01 11:40:00 +0000
+++ src/indicator/nmofono/manager-impl.cpp	2016-12-13 14:49:16 +0000
@@ -784,7 +784,7 @@
             case NM_DEVICE_TYPE_ETHERNET:
             {
                 link = make_shared<ethernet::EthernetLink>(
-                        dev, d->nm, d->m_settingsInterface, d->m_activeConnectionManager);
+                        dev, d->m_settingsInterface, d->m_activeConnectionManager);
                 break;
             }
             case NM_DEVICE_TYPE_MODEM:

=== modified file 'src/indicator/nmofono/vpn/vpn-manager.cpp'
--- src/indicator/nmofono/vpn/vpn-manager.cpp	2016-05-26 13:51:39 +0000
+++ src/indicator/nmofono/vpn/vpn-manager.cpp	2016-12-13 14:49:16 +0000
@@ -112,19 +112,7 @@
 
     void activateConnection(const QDBusObjectPath& connection)
     {
-        auto reply = m_nmInterface->ActivateConnection(connection, QDBusObjectPath("/"), QDBusObjectPath("/"));
-        auto watcher(new QDBusPendingCallWatcher(reply, this));
-        connect(watcher, &QDBusPendingCallWatcher::finished, this, &Priv::activateConnectionFinished);
-    }
-
-    void activateConnectionFinished(QDBusPendingCallWatcher *call)
-    {
-        QDBusPendingReply<QDBusObjectPath> reply = *call;
-        if (reply.isError())
-        {
-            qWarning() << reply.error().message();
-        }
-        call->deleteLater();
+        m_activeConnectionManager->activate(connection);
     }
 
     void updateActiveAndBusy()

=== modified file 'src/indicator/nmofono/wifi/wifi-link-impl.cpp'
--- src/indicator/nmofono/wifi/wifi-link-impl.cpp	2016-12-01 11:40:00 +0000
+++ src/indicator/nmofono/wifi/wifi-link-impl.cpp	2016-12-13 14:49:16 +0000
@@ -108,7 +108,7 @@
         case NM_DEVICE_STATE_FAILED:
         {
             // make sure to set activeConnection before changing the status
-            updateActiveConnection(QDBusObjectPath("/"));
+            updateActiveConnection(connection::ActiveConnection::SPtr());
 
             switch(m_killSwitch->state()) {
             case KillSwitch::State::hard_blocked:
@@ -134,7 +134,7 @@
             // we have one.
             if (path != QDBusObjectPath("/"))
             {
-                updateActiveConnection(path);
+                updateActiveConnection(m_activeConnectionManager->connection(path));
             }
             setStatus(Status::connecting);
             break;
@@ -142,21 +142,20 @@
         case NM_DEVICE_STATE_SECONDARIES:
         {
             // make sure to set activeConnection before changing the status
-            updateActiveConnection(m_dev->activeConnection());
+            updateActiveConnection(m_activeConnectionManager->connection(m_dev->activeConnection()));
             setStatus(Status::connected);
             break;
         }
         case NM_DEVICE_STATE_ACTIVATED:
         {
             // make sure to set activeConnection before changing the status
-            updateActiveConnection(m_dev->activeConnection());
+            updateActiveConnection(m_activeConnectionManager->connection(m_dev->activeConnection()));
             setStatus(Status::online);
             break;
         }}
 
     }
 
-
     void disconnectSignalStengthConnection()
     {
         if (m_signalStrengthConnection)
@@ -166,11 +165,10 @@
         }
     }
 
-    /// '/' path means invalid.
-    void updateActiveConnection(const QDBusObjectPath &path)
+    void updateActiveConnection(connection::ActiveConnection::SPtr activeConnection)
     {
         // clear the one we have.
-        if (path == QDBusObjectPath("/")) {
+        if (!activeConnection) {
             m_activeAccessPoint.reset();
             Q_EMIT p.activeAccessPointUpdated(m_activeAccessPoint);
             m_activeConnection.reset();
@@ -180,45 +178,38 @@
         }
 
         // already up-to-date
-        if (m_activeConnection && m_activeConnection->path() == path)
+        if (m_activeConnection && m_activeConnection->path() == activeConnection->path())
         {
             return;
         }
 
-        m_activeConnection = m_activeConnectionManager->connection(path);
-        if (m_activeConnection)
-        {
-            auto state = m_activeConnection->state();
-            switch (state) {
-            case connection::ActiveConnection::State::unknown:
-            case connection::ActiveConnection::State::activating:
-            case connection::ActiveConnection::State::activated:
-            case connection::ActiveConnection::State::deactivating:
-            case connection::ActiveConnection::State::deactivated:
-                // for Wi-Fi devices specific_object is the AccessPoint object.
-                QDBusObjectPath ap_path = m_activeConnection->specificObject();
-                for (auto &ap : m_groupedAccessPoints) {
-                    auto shap =  dynamic_pointer_cast<GroupedAccessPoint>(ap);
-                    if (shap->has_object(ap_path)) {
-                        m_activeAccessPoint = ap;
-                        disconnectSignalStengthConnection();
-                        m_signalStrengthConnection = make_unique<
-                                QMetaObject::Connection>(
-                                connect(m_activeAccessPoint.get(),
-                                        &AccessPoint::strengthUpdated, this,
-                                        &Private::strengthUpdated));
-                        Q_EMIT p.activeAccessPointUpdated(m_activeAccessPoint);
-                        strengthUpdated();
-                        break;
-                    }
+        m_activeConnection = activeConnection;
+        auto state = m_activeConnection->state();
+        switch (state) {
+        case connection::ActiveConnection::State::unknown:
+        case connection::ActiveConnection::State::activating:
+        case connection::ActiveConnection::State::activated:
+        case connection::ActiveConnection::State::deactivating:
+        case connection::ActiveConnection::State::deactivated:
+            // for Wi-Fi devices specific_object is the AccessPoint object.
+            QDBusObjectPath ap_path = m_activeConnection->specificObject();
+            for (auto &ap : m_groupedAccessPoints) {
+                auto shap =  dynamic_pointer_cast<GroupedAccessPoint>(ap);
+                if (shap->has_object(ap_path)) {
+                    m_activeAccessPoint = ap;
+                    disconnectSignalStengthConnection();
+                    m_signalStrengthConnection = make_unique<
+                            QMetaObject::Connection>(
+                            connect(m_activeAccessPoint.get(),
+                                    &AccessPoint::strengthUpdated, this,
+                                    &Private::strengthUpdated));
+                    Q_EMIT p.activeAccessPointUpdated(m_activeAccessPoint);
+                    strengthUpdated();
+                    break;
                 }
             }
         }
-        else
-        {
-            qWarning() << "Failed to get active connection:" << path.path();
-        }
-    }
+}
 
 
 public Q_SLOTS:
@@ -450,81 +441,77 @@
 {
     qDebug() << "Connecting to:" << accessPoint->ssid();
 
-    try {
-        d->m_connecting = true;
-        QByteArray ssid = accessPoint->raw_ssid();
+    d->m_connecting = true;
+    QByteArray ssid = accessPoint->raw_ssid();
 
-        shared_ptr<OrgFreedesktopNetworkManagerSettingsConnectionInterface> found;
-        QList<QDBusObjectPath> connections = d->m_dev->availableConnections();
-        for (auto &path : connections) {
-            auto con = make_shared<OrgFreedesktopNetworkManagerSettingsConnectionInterface>(
-                    NM_DBUS_SERVICE, path.path(), d->m_dev->connection());
-            QVariantDictMap settings = con->GetSettings();
-            auto wirelessIt = settings.find("802-11-wireless");
-            if (wirelessIt != settings.cend())
+    shared_ptr<OrgFreedesktopNetworkManagerSettingsConnectionInterface> found;
+    QList<QDBusObjectPath> connections = d->m_dev->availableConnections();
+    for (auto &path : connections) {
+        auto con = make_shared<OrgFreedesktopNetworkManagerSettingsConnectionInterface>(
+                NM_DBUS_SERVICE, path.path(), d->m_dev->connection());
+        QVariantDictMap settings = con->GetSettings();
+        auto wirelessIt = settings.find("802-11-wireless");
+        if (wirelessIt != settings.cend())
+        {
+            auto ssidIt = wirelessIt->find("ssid");
+            if (ssidIt != wirelessIt->cend())
             {
-                auto ssidIt = wirelessIt->find("ssid");
-                if (ssidIt != wirelessIt->cend())
+                if (ssidIt->toByteArray() == ssid)
                 {
-                    if (ssidIt->toByteArray() == ssid)
-                    {
-                        found = con;
-                        break;
-                    }
+                    found = con;
+                    break;
                 }
             }
         }
-
-        /// @todo check the timestamps as there might be multiple ones that are suitable.
-        /// @todo oh, and check more parameters than just the ssid
-
-        QDBusObjectPath ac("/");
-        if (found) {
-            qDebug() << "Connecting to known access point";
-            ac = d->m_nm->ActivateConnection(QDBusObjectPath(found->path()),
-                                           QDBusObjectPath(d->m_dev->path()),
-                                           accessPoint->object_path());
+    }
+
+    /// @todo check the timestamps as there might be multiple ones that are suitable.
+    /// @todo oh, and check more parameters than just the ssid
+
+    connection::ActiveConnection::SPtr ac;
+    if (found) {
+        qDebug() << "Connecting to known access point";
+        ac = d->m_activeConnectionManager->activate(QDBusObjectPath(found->path()),
+                                       QDBusObjectPath(d->m_dev->path()),
+                                       accessPoint->object_path());
+    } else {
+        if (accessPoint->enterprise()) {
+            qDebug() << "New connection to enterprise access point";
+            // activate system settings URI
+            QUrlQuery q;
+            q.addQueryItem("ssid", accessPoint->raw_ssid());
+            q.addQueryItem("bssid", accessPoint->bssid());
+            QString url = "settings:///system/wifi?" + q.query(QUrl::FullyEncoded);
+
+            UrlDispatcher::send(url.toStdString(), [](string url, bool success) {
+                if (!success) {
+                    cerr << "URL Dispatcher failed on " << url << endl;
+                }
+            });
         } else {
-            if (accessPoint->enterprise()) {
-                qDebug() << "New connection to enterprise access point";
-                // activate system settings URI
-                QUrlQuery q;
-                q.addQueryItem("ssid", accessPoint->raw_ssid());
-                q.addQueryItem("bssid", accessPoint->bssid());
-                QString url = "settings:///system/wifi?" + q.query(QUrl::FullyEncoded);
-
-                UrlDispatcher::send(url.toStdString(), [](string url, bool success) {
-                    if (!success) {
-                        cerr << "URL Dispatcher failed on " << url << endl;
-                    }
-                });
-            } else {
-                qDebug() << "New connection to regular access point";
-                QVariantDictMap conf;
-
-                /// @todo getting the ssid multiple times over dbus is stupid.
-
-                QVariantMap wireless_conf;
-                wireless_conf["ssid"] = ssid;
-
-                conf["802-11-wireless"] = wireless_conf;
-                auto ret = d->m_nm->AddAndActivateConnection(
-                        conf, QDBusObjectPath(d->m_dev->path()), accessPoint->object_path());
-                ret.waitForFinished();
-                ac = ret.argumentAt<1>();
-            }
-        }
-        // For enterprise access points, the system settings app will perform the connection
-        if (!accessPoint->enterprise()) {
-            d->updateActiveConnection(ac);
-        }
-        d->m_connecting = false;
-    } catch(const exception &e) {
-        // @bug default timeout expired: LP(#1361642)
-        // If this happens, indicator-network is in an unknown state with no clear way of
-        // recovering. The only reasonable way out is a graceful exit.
-        qWarning() << " Failed to activate connection: " << e.what();
-    }
+            qDebug() << "New connection to regular access point";
+            QVariantDictMap conf;
+
+            /// @todo getting the ssid multiple times over dbus is stupid.
+
+            QVariantMap wireless_conf;
+            wireless_conf["ssid"] = ssid;
+
+            conf["802-11-wireless"] = wireless_conf;
+            ac = d->m_activeConnectionManager->addAndActivate(conf, QDBusObjectPath(d->m_dev->path()), accessPoint->object_path());
+        }
+    }
+    if (!ac)
+    {
+        qWarning() << " Failed to activate connection";
+        return;
+    }
+
+    // For enterprise access points, the system settings app will perform the connection
+    if (!accessPoint->enterprise()) {
+        d->updateActiveConnection(ac);
+    }
+    d->m_connecting = false;
 }
 
 AccessPoint::Ptr