From a532eafc4edac1532c52dbe35dec73b1d5322c35 Mon Sep 17 00:00:00 2001
From: Konstantin Ritt <ritt.ks@gmail.com>
Date: Wed, 16 Aug 2017 13:50:46 +0400
Subject: [PATCH] Simplify (un)subscribe

Make calls more user-friendly by making the 'qos' param optional.
Do not expose packet id as it couldn't be used for/compared to
anything outside ClientPrivate; also makes (un)subscribe symmetrical.
Inline some code.
Complements 3d6354e9f0598
---
 src/mqtt/qmqtt_client.cpp        |  4 ++--
 src/mqtt/qmqtt_client.h          |  4 ++--
 src/mqtt/qmqtt_client_p.cpp      | 27 ++++++---------------------
 src/mqtt/qmqtt_client_p.h        |  4 +---
 tests/gtest/tests/clienttest.cpp |  6 +++---
 5 files changed, 14 insertions(+), 31 deletions(-)

diff --git a/src/mqtt/qmqtt_client.cpp b/src/mqtt/qmqtt_client.cpp
index 6a07d9d..3a57059 100644
--- a/src/mqtt/qmqtt_client.cpp
+++ b/src/mqtt/qmqtt_client.cpp
@@ -293,10 +293,10 @@ quint16 QMQTT::Client::publish(const Message& message)
     return d->publish(message);
 }
 
-quint16 QMQTT::Client::subscribe(const QString& topic, const quint8 qos)
+void QMQTT::Client::subscribe(const QString& topic, const quint8 qos)
 {
     Q_D(Client);
-    return d->subscribe(topic, qos);
+    d->subscribe(topic, qos);
 }
 
 void QMQTT::Client::unsubscribe(const QString& topic)
diff --git a/src/mqtt/qmqtt_client.h b/src/mqtt/qmqtt_client.h
index dec14da..a3c4ae0 100644
--- a/src/mqtt/qmqtt_client.h
+++ b/src/mqtt/qmqtt_client.h
@@ -187,7 +187,7 @@ public slots:
     void connectToHost();
     void disconnectFromHost();
 
-    quint16 subscribe(const QString& topic, const quint8 qos);
+    void subscribe(const QString& topic, const quint8 qos = 0);
     void unsubscribe(const QString& topic);
 
     quint16 publish(const Message& message);
@@ -197,7 +197,7 @@ public slots:
     void disconnected();
     void error(const QMQTT::ClientError error);
 
-    void subscribed(const QString& topic, const quint8 qos);
+    void subscribed(const QString& topic, const quint8 qos = 0);
     void unsubscribed(const QString& topic);
     void published(const quint16 msgid, const quint8 qos);
     void received(const QMQTT::Message& message);
diff --git a/src/mqtt/qmqtt_client_p.cpp b/src/mqtt/qmqtt_client_p.cpp
index 4aa8c96..750da17 100644
--- a/src/mqtt/qmqtt_client_p.cpp
+++ b/src/mqtt/qmqtt_client_p.cpp
@@ -341,11 +341,10 @@ void QMQTT::ClientPrivate::puback(const quint8 type, const quint16 msgid)
     sendPuback(type, msgid);
 }
 
-quint16 QMQTT::ClientPrivate::subscribe(const QString& topic, const quint8 qos)
+void QMQTT::ClientPrivate::subscribe(const QString& topic, const quint8 qos)
 {
     quint16 msgid = sendSubscribe(topic, qos);
     _midToTopic[msgid] = topic;
-    return msgid;
 }
 
 void QMQTT::ClientPrivate::unsubscribe(const QString& topic)
@@ -359,7 +358,7 @@ void QMQTT::ClientPrivate::onNetworkDisconnected()
     Q_Q(Client);
 
     stopKeepAlive();
-    clearMidToTopic();
+    _midToTopic.clear();
     emit q->disconnected();
 }
 
@@ -405,12 +404,14 @@ void QMQTT::ClientPrivate::onNetworkReceived(const QMQTT::Frame& frm)
         break;
     case SUBACK:
         mid = frame.readInt();
+        topic = _midToTopic.take(mid);
         qos = frame.readChar();
-        handleSuback(midToTopic(mid), qos);
+        handleSuback(topic, qos);
         break;
     case UNSUBACK:
         mid = frame.readInt();
-        handleUnsuback(midToTopic(mid));
+        topic = _midToTopic.take(mid);
+        handleUnsuback(topic);
         break;
     case PINGRESP:
         handlePingresp();
@@ -480,22 +481,6 @@ void QMQTT::ClientPrivate::handleUnsuback(const QString &topic) {
     emit q->unsubscribed(topic);
 }
 
-QString QMQTT::ClientPrivate::midToTopic(const quint16 mid)
-{
-    QString result;
-    QHash<quint16, QString>::Iterator it = _midToTopic.find(mid);
-    if (it != _midToTopic.end()) {
-        result = it.value();
-        _midToTopic.erase(it);
-    }
-    return result;
-}
-
-void QMQTT::ClientPrivate::clearMidToTopic()
-{
-    _midToTopic.clear();
-}
-
 bool QMQTT::ClientPrivate::autoReconnect() const
 {
     return _network->autoReconnect();
diff --git a/src/mqtt/qmqtt_client_p.h b/src/mqtt/qmqtt_client_p.h
index fcffdcc..872fcf5 100644
--- a/src/mqtt/qmqtt_client_p.h
+++ b/src/mqtt/qmqtt_client_p.h
@@ -95,7 +95,7 @@ class ClientPrivate
     void onNetworkDisconnected();
     quint16 publish(const Message& message);
     void puback(const quint8 type, const quint16 msgid);
-    quint16 subscribe(const QString& topic, const quint8 qos);
+    void subscribe(const QString& topic, const quint8 qos);
     void unsubscribe(const QString& topic);
     void onNetworkReceived(const QMQTT::Frame& frame);
     void handleConnack(const quint8 ack);
@@ -105,8 +105,6 @@ class ClientPrivate
     void handleUnsuback(const QString& topic);
     void handlePubcomp(const Message& message);
     void handlePingresp();
-    QString midToTopic(const quint16 mid);
-    void clearMidToTopic();
     bool autoReconnect() const;
     void setAutoReconnect(const bool autoReconnect);
     bool autoReconnectInterval() const;
diff --git a/tests/gtest/tests/clienttest.cpp b/tests/gtest/tests/clienttest.cpp
index 2493ec1..ae4b6bf 100644
--- a/tests/gtest/tests/clienttest.cpp
+++ b/tests/gtest/tests/clienttest.cpp
@@ -431,11 +431,11 @@ TEST_F(ClientTest, subscribeEmitsSubscribedSignal_Test)
     EXPECT_CALL(*_networkMock, sendFrame(_));
     QSignalSpy spy(_client.data(), &QMQTT::Client::subscribed);
 
-    quint16 msgid = _client->subscribe("topic", QOS2);
+    _client->subscribe("topic", QOS2);
 
     QByteArray payLoad;
-    payLoad.append((char)(msgid >> 8)); // message ID MSB
-    payLoad.append((char)(msgid && 0xFF)); // message ID LSB
+    payLoad.append((char)0x00); // message ID MSB
+    payLoad.append((char)0x01); // message ID LSB
     payLoad.append((char)QOS2); // QOS
     QMQTT::Frame frame(SUBACK_TYPE, payLoad);
     emit _networkMock->received(frame);