Merge lp:~unity-api-team/indicator-network/fix_unlock_all into lp:indicator-network/14.10

[Antti Kaijanmäki]

Fix UnlockAllModems.

[Charles Kerr (charlesk)]

This patch is kind of tortured, but after talking it through with Antti I agree this is a good approach given the tight schedule and the need to know that the patch should be safe.

It would be preferable if a future revision fixes the workarounds described in the comment in SimUnlockDialog::Private::reset()

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:448
http://jenkins.qa.ubuntu.com/job/indicator-network-ci/200/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-utopic-amd64-ci/74
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-utopic-armhf-ci/74
        deb: http://jenkins.qa.ubuntu.com/job/indicator-network-utopic-armhf-ci/74/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-network-ci/200/rebuild

review: Approve (continuous-integration)

[Michael Terry (mterry)]

I tested this and it worked for me. But obviously not a code review or anything.

=== modified file 'src/indicator/modem-manager.cpp'
--- src/indicator/modem-manager.cpp	2014-10-10 20:33:13 +0000
+++ src/indicator/modem-manager.cpp	2014-10-14 20:06:14 +0000
@@ -67,19 +67,11 @@
         m_dbus.reset(new core::dbus::DBus(m_bus));
 
         m_unlockDialog = std::make_shared<SimUnlockDialog>();
-        m_unlockDialog->state().changed().connect([this](SimUnlockDialog::State state){
-            switch (state) {
-            case SimUnlockDialog::State::unlocking:
-            case SimUnlockDialog::State::changingPin:
-                // don't care
-                break;
-            case SimUnlockDialog::State::ready:
-                if (!m_pendingUnlocks.empty()) {
-                    auto modem = m_pendingUnlocks.front();
-                    m_pendingUnlocks.pop_front();
-                    m_unlockDialog->unlock(modem);
-                }
-                break;
+        m_unlockDialog->ready().connect([this](){
+            if (!m_pendingUnlocks.empty()) {
+                auto modem = m_pendingUnlocks.front();
+                m_pendingUnlocks.pop_front();
+                m_unlockDialog->unlock(modem);
             }
         });
 
@@ -196,7 +188,7 @@
                 || std::count(d->m_pendingUnlocks.begin(), d->m_pendingUnlocks.end(), modem) != 0)
             return;
 
-        if (d->m_unlockDialog->state().get() == SimUnlockDialog::State::ready
+        if (d->m_unlockDialog->state() == SimUnlockDialog::State::ready
                 && d->m_pendingUnlocks.size() == 0)
             d->m_unlockDialog->unlock(modem);
         else

=== modified file 'src/indicator/sim-unlock-dialog.cpp'
--- src/indicator/sim-unlock-dialog.cpp	2014-10-07 17:42:56 +0000
+++ src/indicator/sim-unlock-dialog.cpp	2014-10-14 20:06:14 +0000
@@ -85,6 +85,7 @@
     };
     EnterPinStates m_enterPinState;
 
+    core::Signal<void> m_ready;
     core::Property<State> m_state;
     Modem::Ptr m_modem;
 
@@ -100,6 +101,9 @@
 
     core::Property<bool> m_showSimIdentifiers;
 
+    /// @todo see comment in reset()
+    bool m_doCleanUp;
+
     bool sendEnterPin(std::string pin)
     {
         int retries = -1;
@@ -216,8 +220,13 @@
 };
 
 SimUnlockDialog::Private::Private()
+    : m_doCleanUp{true}
 {
     m_sd = std::make_shared<notify::snapdecision::SimUnlock>();
+    m_sd->cancelled().connect(std::bind(&Private::cancelled, this));
+    m_sd->pinEntered().connect(std::bind(&Private::pinEntered, this, std::placeholders::_1));
+    m_sd->closed().connect(std::bind(&Private::closed, this));
+
     m_showSimIdentifiers.set(false);
     reset();
 }
@@ -370,8 +379,7 @@
 void
 SimUnlockDialog::Private::reset()
 {
-    /** @todo
-     * bug in dbus-cpp :/
+    /** @bug in properties-cpp :/
      * can't disconnect here as the reset() is called from pinEnterer() and
      * we can't disconnect a signal inside one of it's handlers right now.
      * uncomment this code once dbus-cpp is fixed.
@@ -389,6 +397,9 @@
     m_enterPinState = EnterPinStates::initial;
 
     m_state.set(State::ready);
+    m_doCleanUp = false;
+    m_ready();
+    m_doCleanUp = true;
 }
 
 SimUnlockDialog::SimUnlockDialog()
@@ -410,9 +421,11 @@
     d->m_state.set(State::unlocking);
 
     /// @todo delayed disconnections, see comment in reset()
-    for (auto &c : d->m_connections)
-        c.disconnect();
-    d->m_connections.clear();
+    if (d->m_doCleanUp) {
+        for (auto &c : d->m_connections)
+            c.disconnect();
+        d->m_connections.clear();
+    }
 
     auto retries = modem->retries().get();
     auto type = modem->requiredPin().get();
@@ -434,15 +447,6 @@
     c = modem->retries().changed().connect(std::bind(&Private::update, d.get()));
     d->m_connections.push_back(c);
 
-    c = d->m_sd->cancelled().connect(std::bind(&Private::cancelled, d.get()));
-    d->m_connections.push_back(c);
-
-    c = d->m_sd->pinEntered().connect(std::bind(&Private::pinEntered, d.get(), std::placeholders::_1));
-    d->m_connections.push_back(c);
-
-    c = d->m_sd->closed().connect(std::bind(&Private::closed, d.get()));
-    d->m_connections.push_back(c);
-
     int pinRetries = -1;
     int pukRetries = -1;
 
@@ -470,67 +474,6 @@
 }
 
 void
-SimUnlockDialog::changePin(Modem::Ptr modem)
-{
-    /// @todo do we need this?
-    if (d->m_modem)
-        throw std::logic_error("Unlocking already in progress.");
-
-    if (modem->simStatus().get() != Modem::SimStatus::ready)
-        throw std::logic_error("SIM is not ready");
-
-    d->m_modem = modem;
-    d->m_state.set(State::changingPin);
-
-    if (d->m_modem)
-        throw std::logic_error("Unlocking already in progress.");
-
-    auto c = d->m_sd->cancelled().connect(std::bind(&Private::cancelled, d.get()));
-    d->m_connections.push_back(c);
-
-    c = d->m_sd->closed().connect(std::bind(&Private::closed, d.get()));
-    d->m_connections.push_back(c);
-
-    /// @todo add SIM identifier
-    d->m_sd->title().set(_("Enter SIM PIN"));
-    d->m_sd->body().set("");
-    d->m_sd->pinMinMax().set({4,8});
-    d->m_sd->show();
-
-    std::string currentPin;
-    std::string newPin;
-
-    c = d->m_sd->pinEntered().connect([this, &currentPin, &newPin](std::string pin){
-        if (currentPin.empty()) {
-            currentPin = pin;
-            d->m_sd->title().set(_("Enter new PIN code"));
-            /// @todo make sure the dialog can't be cancelled from this point onward
-        } else if (newPin.empty()) {
-            newPin = pin;
-            d->m_sd->title().set(_("Confirm new PIN code"));
-        } else {
-            // we have the current PIN and the newPin and confirmed pin is provided as argument pin
-            if (newPin != pin) {
-                newPin.clear();
-                d->m_sd->showError(_("PIN codes did not match."));
-                d->m_sd->title().set(_("Enter new PIN code"));
-            } else {
-                if (d->m_modem->changePin(Modem::PinType::pin, currentPin, newPin)) {
-                    currentPin.clear();
-                    newPin.clear();
-                    /// @todo make sure the dialog can be cancelled again
-                    d->m_sd->showError(_("Failed to change PIN.")); /// @todo instruct that the current pin was most probably wrong.
-                    d->m_sd->title().set(_("Enter SIM PIN"));
-                } else {
-                    d->m_sd->close();
-                }
-            }
-        }
-    });
-    d->m_connections.push_back(c);
-}
-
-void
 SimUnlockDialog::cancel()
 {
     d->m_sd->close();
@@ -542,10 +485,16 @@
     return d->m_modem;
 }
 
-core::Property<SimUnlockDialog::State> &
-SimUnlockDialog::state()
-{
-    return d->m_state;
+SimUnlockDialog::State
+SimUnlockDialog::state() const
+{
+    return d->m_state.get();
+}
+
+core::Signal<void> &
+SimUnlockDialog::ready()
+{
+    return d->m_ready;
 }
 
 core::Property<bool> &

=== modified file 'src/indicator/sim-unlock-dialog.h'
--- src/indicator/sim-unlock-dialog.h	2014-10-07 08:29:33 +0000
+++ src/indicator/sim-unlock-dialog.h	2014-10-14 20:06:14 +0000
@@ -41,20 +41,13 @@
 
     void unlock(Modem::Ptr modem);
 
-    /**
-     * there must be no other operation active on the dialog
-     * modem has to be on ready state, i.e. it has to be unlocked before
-     * changing the pin.
-     *
-     * @param modem
-     */
-    void changePin(Modem::Ptr modem);
-
     void cancel();
 
     Modem::Ptr modem();
 
-    core::Property<State> &state();
+    State state() const;
+    core::Signal<void> &ready();
+
     core::Property<bool> &showSimIdentifiers();
 };
 

=== modified file 'src/notify-cpp/snapdecision/sim-unlock.cpp'
--- src/notify-cpp/snapdecision/sim-unlock.cpp	2014-10-07 17:24:22 +0000
+++ src/notify-cpp/snapdecision/sim-unlock.cpp	2014-10-14 20:06:14 +0000
@@ -34,6 +34,7 @@
 public:
 
     Notification::Ptr m_notification;
+    Notification::Ptr m_pending;
 
     core::Property<std::string> m_title;
     core::Property<std::string> m_body;
@@ -45,6 +46,8 @@
 
     std::shared_ptr<SessionBus> m_sessionBus;
 
+    std::map<std::string, Variant> m_modelPaths;
+
     Action::Ptr m_notifyAction;
     Action::Ptr m_pinMinMaxAction;
     Action::Ptr m_popupAction;
@@ -59,6 +62,21 @@
     std::function<void()> m_pendingErrorClosed;
     std::function<void()> m_pendingPopupClosed;
 
+    void resetNotification(const std::string &title,
+                           const std::string &body)
+    {
+        m_pending = m_notification;
+        m_notification = std::make_shared<notify::Notification>(title, body, "");
+        m_notification->setHintString("x-canonical-snap-decisions", "true");
+        m_notification->setHint("x-canonical-snap-decisions-timeout", TypedVariant<std::int32_t>(std::numeric_limits<std::int32_t>::max()));
+        m_notification->setHint("x-canonical-private-menu-model", TypedVariant<std::map<std::string, Variant>>(m_modelPaths));
+        m_notification->closed().connect([this]()
+        {
+            resetNotification(m_title.get(), m_body.get());
+            m_closed();
+        });
+    }
+
     Private(const std::string &title,
             const std::string &body,
             std::pair<std::uint8_t, std::uint8_t> pinMinMax)
@@ -78,10 +96,9 @@
         std::map<std::string, Variant> modelActions;
         modelActions["notifications"] = TypedVariant<std::string>(actionPath);
 
-        std::map<std::string, Variant> modelPaths;
-        modelPaths["busName"] = TypedVariant<std::string>(m_sessionBus->address());
-        modelPaths["menuPath"] = TypedVariant<std::string>(menuPath);
-        modelPaths["actions"] = TypedVariant<std::map<std::string, Variant>>(modelActions);
+        m_modelPaths["busName"] = TypedVariant<std::string>(m_sessionBus->address());
+        m_modelPaths["menuPath"] = TypedVariant<std::string>(menuPath);
+        m_modelPaths["actions"] = TypedVariant<std::map<std::string, Variant>>(modelActions);
 
         m_menu = std::make_shared<Menu>();
         m_menuItem = std::make_shared<MenuItem>("", "notifications.simunlock");
@@ -141,11 +158,7 @@
         m_menuExporter = std::make_shared<MenuExporter>(m_sessionBus, menuPath, m_menu);
         m_actionGroupExporter = std::make_shared<ActionGroupExporter>(m_sessionBus, m_actionGroup, actionPath);
 
-        m_notification = std::make_shared<notify::Notification>(title, body, "");
-        m_notification->setHintString("x-canonical-snap-decisions", "true");
-        m_notification->setHint("x-canonical-snap-decisions-timeout", TypedVariant<std::int32_t>(std::numeric_limits<std::int32_t>::max()));
-        m_notification->setHint("x-canonical-private-menu-model", TypedVariant<std::map<std::string, Variant>>(modelPaths));
-        m_notification->closed().connect([this](){ m_closed(); });
+        resetNotification(title, body);
 
         m_title.changed().connect([this](const std::string &value){
             m_notification->summary().set(value);