Speed up the device removal - VPN-1094 (#1976)

src/models/devicemodel.cpp

@@ -71,8 +71,19 @@ bool sortCallback(const Device& a, const Device& b, const Keys* keys) {
 bool DeviceModel::fromJsonInternal(const Keys* keys, const QByteArray& json) {
   beginResetModel();
 
+  // Maybe we have to refresh the device list during a removal operation. If
+  // this happens, maybe we have to store some of the "incoming" devices in the
+  // list of the removed ones.
+  // This is done comparing the list of the publicKeys of the removed devices
+  // with the new ones.
+  QStringList removedPublicKeys;
+  for (const Device& removedDevice : m_removedDevices) {
+    removedPublicKeys.append(removedDevice.publicKey());
+  }
+
   m_rawJson = "";
   m_devices.clear();
+  m_removedDevices.clear();
 
   QJsonDocument doc = QJsonDocument::fromJson(json);
   if (!doc.isObject()) {
@@ -96,7 +107,12 @@ bool DeviceModel::fromJsonInternal(const Keys* keys, const QByteArray& json) {
     if (!device.fromJson(deviceValue)) {
       return false;
     }
-    m_devices.append(device);
+
+    if (removedPublicKeys.contains(device.publicKey())) {
+      m_removedDevices.append(device);
+    } else {
+      m_devices.append(device);
+    }
   }
 
   std::sort(m_devices.begin(), m_devices.end(),
@@ -150,7 +166,51 @@ QVariant DeviceModel::data(const QModelIndex& index, int role) const {
   }
 }
 
+void DeviceModel::startDeviceRemovalFromPublicKey(const QString& publicKey) {
+  for (int i = 0; i < m_devices.length(); ++i) {
+    if (m_devices.at(i).publicKey() == publicKey) {
+      // Let's remove this device in a separate list to restore it in case the
+      // removal fails or in case we have to refresh the whole model.
+      m_removedDevices.append(m_devices.at(i));
+      removeRow(i);
+      emit changed();
+      return;
+    }
+  }
+}
+
+void DeviceModel::stopDeviceRemovalFromPublicKey(const QString& publicKey,
+                                                 const Keys* keys) {
+  for (auto i = m_removedDevices.begin(); i != m_removedDevices.end(); ++i) {
+    if (i->publicKey() == publicKey) {
+      // We were not supposed to find the device in this list. If this happens
+      // is because something went wrong during the removal operation. Let's
+      // bring the device back.
+      beginResetModel();
+
+      m_devices.append(*i);
+
+      std::sort(m_devices.begin(), m_devices.end(),
+                std::bind(sortCallback, std::placeholders::_1,
+                          std::placeholders::_2, keys));
+
+      m_removedDevices.erase(i);
+
+      endResetModel();
+      emit changed();
+      break;
+    }
+  }
+}
+
 void DeviceModel::removeDeviceFromPublicKey(const QString& publicKey) {
+  for (auto i = m_removedDevices.begin(); i != m_removedDevices.end(); ++i) {
+    if (i->publicKey() == publicKey) {
+      m_removedDevices.erase(i);
+      break;
+    }
+  }
+
   for (int i = 0; i < m_devices.length(); ++i) {
     if (m_devices.at(i).publicKey() == publicKey) {
       removeRow(i);

src/models/devicemodel.h

@@ -37,6 +37,10 @@ class DeviceModel final : public QAbstractListModel {
 
   void writeSettings();
 
+  void startDeviceRemovalFromPublicKey(const QString& publicKey);
+  void stopDeviceRemovalFromPublicKey(const QString& publicKey,
+                                      const Keys* keys);
+
   void removeDeviceFromPublicKey(const QString& publicKey);
 
   int activeDevices() const { return m_devices.count(); }
@@ -72,6 +76,9 @@ class DeviceModel final : public QAbstractListModel {
   QByteArray m_rawJson;
 
   QList<Device> m_devices;
+
+  // This list contains the list of devices that are about to be removed.
+  QList<Device> m_removedDevices;
 };
 
 #endif  // DEVICEMODEL_H

src/mozillavpn.cpp

@@ -86,6 +86,9 @@ MozillaVPN* MozillaVPN::instance() {
   return s_instance;
 }
 
+// static
+MozillaVPN* MozillaVPN::maybeInstance() { return s_instance; }
+
 MozillaVPN::MozillaVPN() : m_private(new Private()) {
   MVPN_COUNT_CTOR(MozillaVPN);
 
@@ -703,6 +706,11 @@ void MozillaVPN::serversFetched(const QByteArray& serverData,
   }
 }
 
+void MozillaVPN::deviceRemovalCompleted(const QString& publicKey) {
+  logger.debug() << "Device removal task completed";
+  m_private->m_deviceModel.stopDeviceRemovalFromPublicKey(publicKey, keys());
+}
+
 void MozillaVPN::removeDeviceFromPublicKey(const QString& publicKey) {
   logger.debug() << "Remove device";
 
@@ -712,6 +720,12 @@ void MozillaVPN::removeDeviceFromPublicKey(const QString& publicKey) {
     // Let's inform the UI about what is going to happen.
     emit deviceRemoving(publicKey);
     scheduleTask(new TaskRemoveDevice(publicKey));
+
+    if (m_state != StateDeviceLimit) {
+      // To have a faster UI, we inform the device-model that this public key
+      // is going to be removed.
+      m_private->m_deviceModel.startDeviceRemovalFromPublicKey(publicKey);
+    }
   }
 
   if (m_state != StateDeviceLimit) {

src/mozillavpn.h

@@ -112,6 +112,10 @@ class MozillaVPN final : public QObject {
 
   static MozillaVPN* instance();
 
+  // This is exactly like the ::instance() method, but it doesn't crash if the
+  // MozillaVPN is null. It should be used rarely.
+  static MozillaVPN* maybeInstance();
+
   void initialize();
 
   State state() const;
@@ -204,6 +208,7 @@ class MozillaVPN final : public QObject {
                    const QString& privateKey);
 
   void deviceRemoved(const QString& publicKey);
+  void deviceRemovalCompleted(const QString& publicKey);
 
   void serversFetched(const QByteArray& serverData,
                       const QByteArray& serverExtraData);

src/tasks/removedevice/taskremovedevice.cpp

@@ -19,7 +19,17 @@ TaskRemoveDevice::TaskRemoveDevice(const QString& publicKey)
   MVPN_COUNT_CTOR(TaskRemoveDevice);
 }
 
-TaskRemoveDevice::~TaskRemoveDevice() { MVPN_COUNT_DTOR(TaskRemoveDevice); }
+TaskRemoveDevice::~TaskRemoveDevice() {
+  MVPN_COUNT_DTOR(TaskRemoveDevice);
+
+  // Nothing guarantees that when this task is deleted, the VPN object is still
+  // alive. We cannot use the QObject-parenting solution because it deletes the
+  // parent before the children.
+  MozillaVPN* vpn = MozillaVPN::maybeInstance();
+  if (vpn) {
+    vpn->deviceRemovalCompleted(m_publicKey);
+  }
+}
 
 void TaskRemoveDevice::run(MozillaVPN* vpn) {
   logger.debug() << "Removing the device with public key" << m_publicKey;

tests/auth/mocmozillavpn.cpp

@@ -9,7 +9,10 @@
 static MozillaVPN* s_instance = nullptr;
 
 // static
-MozillaVPN* MozillaVPN::instance() {
+MozillaVPN* MozillaVPN::instance() { return maybeInstance(); }
+
+// static
+MozillaVPN* MozillaVPN::maybeInstance() {
   if (!s_instance) {
     s_instance = new MozillaVPN();
   }
@@ -52,6 +55,8 @@ void MozillaVPN::deviceAdded(const QString&, const QString&, const QString&) {}
 
 void MozillaVPN::deviceRemoved(const QString&) {}
 
+void MozillaVPN::deviceRemovalCompleted(const QString&) {}
+
 bool MozillaVPN::setServerList(const QByteArray&, const QByteArray&) {
   return true;
 }

tests/unit/mocmozillavpn.cpp

@@ -10,7 +10,10 @@
 static MozillaVPN* s_instance = nullptr;
 
 // static
-MozillaVPN* MozillaVPN::instance() {
+MozillaVPN* MozillaVPN::instance() { return maybeInstance(); }
+
+// static
+MozillaVPN* MozillaVPN::maybeInstance() {
   if (!s_instance) {
     s_instance = new MozillaVPN();
   }
@@ -54,6 +57,8 @@ void MozillaVPN::deviceAdded(const QString&, const QString&, const QString&) {}
 
 void MozillaVPN::deviceRemoved(const QString&) {}
 
+void MozillaVPN::deviceRemovalCompleted(const QString&) {}
+
 bool MozillaVPN::setServerList(const QByteArray&, const QByteArray&) {
   return true;
 }

tests/unit/testmodels.cpp

@@ -401,6 +401,64 @@ void TestModels::deviceModelFromJson() {
   }
 }
 
+void TestModels::deviceModelRemoval() {
+  QJsonObject d1;
+  d1.insert("name", "deviceName");
+  d1.insert("unique_id", "d1");
+  d1.insert("pubkey", "devicePubkey1");
+  d1.insert("created_at", "2017-07-24T15:46:29");
+  d1.insert("ipv4_address", "deviceIpv4");
+  d1.insert("ipv6_address", "deviceIpv6");
+
+  QJsonObject d2;
+  d2.insert("name", "deviceName");
+  d2.insert("unique_id", "d2");
+  d2.insert("pubkey", "devicePubkey2");
+  d2.insert("created_at", "2017-07-24T15:46:29");
+  d2.insert("ipv4_address", "deviceIpv4");
+  d2.insert("ipv6_address", "deviceIpv6");
+
+  QJsonArray devices;
+  devices.append(d1);
+  devices.append(d2);
+
+  QJsonObject obj;
+  obj.insert("devices", devices);
+
+  Keys keys;
+  keys.storeKeys("private", "currentDevicePubkey");
+
+  DeviceModel dm;
+  QCOMPARE(dm.fromJson(&keys, QJsonDocument(obj).toJson()), true);
+
+  QCOMPARE(dm.rowCount(QModelIndex()), 2);
+
+  // Let's start the removal.
+  dm.startDeviceRemovalFromPublicKey("devicePubkey1");
+  QCOMPARE(dm.rowCount(QModelIndex()), 1);
+
+  // Refresh the model. The removed device is still gone.
+  QCOMPARE(dm.fromJson(&keys, QJsonDocument(obj).toJson()), true);
+  QCOMPARE(dm.rowCount(QModelIndex()), 1);
+
+  // Complete the removal without removing the device for real (simulate a
+  // failure).
+  dm.stopDeviceRemovalFromPublicKey("devicePubkey1", &keys);
+  QCOMPARE(dm.rowCount(QModelIndex()), 2);
+
+  // Let's start the removal again.
+  dm.startDeviceRemovalFromPublicKey("devicePubkey1");
+  QCOMPARE(dm.rowCount(QModelIndex()), 1);
+
+  // Remove the device for real.
+  dm.removeDeviceFromPublicKey("devicePubkey1");
+  QCOMPARE(dm.rowCount(QModelIndex()), 1);
+
+  // We have only 1 device left.
+  dm.stopDeviceRemovalFromPublicKey("devicePubkey1", &keys);
+  QCOMPARE(dm.rowCount(QModelIndex()), 1);
+}
+
 // Feedback Category
 // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

tests/unit/testmodels.h

@@ -16,6 +16,7 @@ class TestModels final : public TestHelper {
   void deviceModelBasic();
   void deviceModelFromJson_data();
   void deviceModelFromJson();
+  void deviceModelRemoval();
 
   void feedbackCategoryBasic();