refactor(messages): don't use receipt 0 as unsent, simplify class state

src/persistence/offlinemsgengine.cpp

@@ -26,6 +26,7 @@
 #include <QMutexLocker>
 #include <QTimer>
 #include <QCoreApplication>
+#include <chrono>
 
 OfflineMsgEngine::OfflineMsgEngine(Friend* frnd)
     : mutex(QMutex::Recursive)
@@ -33,111 +34,144 @@ OfflineMsgEngine::OfflineMsgEngine(Friend* frnd)
 {
 }
 
-void OfflineMsgEngine::dischargeReceipt(ReceiptNum receipt)
+/**
+* @brief Notification that the message is now delivered.
+*
+* @param[in] receipt   Toxcore message ID which the receipt is for.
+*/
+void OfflineMsgEngine::onReceiptReceived(ReceiptNum receipt)
 {
     QMutexLocker ml(&mutex);
-
+    if (receivedReceipts.contains(receipt)) {
-    auto it = receipts.find(receipt);
+        qWarning() << "Receievd duplicate receipt" << receipt.get() << "from friend" << f->getId();
-    if (it == receipts.end()) {
+        return;
-        it = receipts.insert(receipt, Receipt());
-    } else if (it->bRecepitReceived) {
-        qWarning() << "Received duplicate receipt";
     }
-    it->bRecepitReceived = true;
+    receivedReceipts.append(receipt);
-    processReceipt(receipt);
+    checkForCompleteMessages(receipt);
 }
 
-void OfflineMsgEngine::registerReceipt(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr msg)
+/**
+* @brief Add a message which has been saved to history, but not sent yet to the peer.
+*
+* OfflineMsgEngine will send this message once the friend becomes online again, then track its
+* receipt, updating history and chatlog once received.
+*
+* @param[in] messageID   database RowId of the message, used to eventually mark messages as received in history
+* @param[in] msg         chat message line in the chatlog, used to eventually set the message's receieved timestamp
+*/
+void OfflineMsgEngine::addSavedMessage(RowId messageID, ChatMessage::Ptr chatMessage)
 {
     QMutexLocker ml(&mutex);
-
+    assert([&](){
-    auto it = receipts.find(receipt);
+        for (const auto& message : unsentSavedMessages) {
-    if (it == receipts.end()) {
+            if (message.rowId == messageID) {
-        it = receipts.insert(receipt, Receipt());
+                return false;
-    } else if (it->bRowValid && receipt.get() != 0 /* offline receipt */) {
-        qWarning() << "Received duplicate registration of receipt";
             }
-    it->rowId = messageID;
+        }
-    it->bRowValid = true;
+        return true;
-    undeliveredMsgs[messageID] = {msg, receipt};
+    }());
-    processReceipt(receipt);
+    unsentSavedMessages.append(Message{chatMessage, messageID, std::chrono::steady_clock::now()});
 }
 
+/**
+* @brief Add a message which has been saved to history, and which has been sent to the peer.
+*
+* OfflineMsgEngine will track this message's receipt. If the friend goes offline then comes back before the receipt
+* is received, OfflineMsgEngine will also resend the message, updating history and chatlog once received.
+*
+* @param[in] receipt     the toxcore message ID, corresponding to expected receipt ID
+* @param[in] messageID   database RowId of the message, used to eventually mark messages as received in history
+* @param[in] msg         chat message line in the chatlog, used to eventually set the message's receieved timestamp
+*/
+void OfflineMsgEngine::addSentSavedMessage(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr chatMessage)
+{
+    QMutexLocker ml(&mutex);
+    assert(!sentSavedMessages.contains(receipt));
+    sentSavedMessages.insert(receipt, {chatMessage, messageID, std::chrono::steady_clock::now()});
+    checkForCompleteMessages(receipt);
+}
+
+/**
+* @brief Deliver all messages, used when a friend comes online.
+*/
 void OfflineMsgEngine::deliverOfflineMsgs()
 {
     QMutexLocker ml(&mutex);
 
-    if (!Settings::getInstance().getFauxOfflineMessaging())
+    if (!Settings::getInstance().getFauxOfflineMessaging()) {
         return;
+    }
 
-    if (f->getStatus() == Status::Offline)
+    if (f->getStatus() == Status::Offline) {
         return;
+    }
 
-    if (undeliveredMsgs.size() == 0)
+    if (sentSavedMessages.empty() && unsentSavedMessages.empty()) {
         return;
+    }
 
-    QMap<RowId, MsgPtr> msgs = undeliveredMsgs;
+    QVector<Message> messages = sentSavedMessages.values().toVector() + unsentSavedMessages;
-    removeAllReceipts();
+    // order messages by authorship time to resend in same order as they were written
-    undeliveredMsgs.clear();
+    qSort(messages.begin(), messages.end(), [](const Message& lhs, const Message& rhs){ return lhs.authorshipTime < rhs.authorshipTime; });
+    removeAllMessages();
 
-    for (auto iter = msgs.begin(); iter != msgs.end(); ++iter) {
+    for (const auto& message : messages) {
-        auto val = iter.value();
+        QString messageText = message.chatMessage->toString();
-        auto key = iter.key();
+        ReceiptNum receipt;
-        QString messageText = val.msg->toString();
+        bool messageSent{false};
-        ReceiptNum rec;
+        if (message.chatMessage->isAction()) {
-        if (val.msg->isAction()) {
+            messageSent = Core::getInstance()->sendAction(f->getId(), messageText, receipt);
-            Core::getInstance()->sendAction(f->getId(), messageText, rec);
         } else {
-            Core::getInstance()->sendMessage(f->getId(), messageText, rec);
+            messageSent = Core::getInstance()->sendMessage(f->getId(), messageText, receipt);
+        }
+        if (messageSent) {
+            addSentSavedMessage(receipt, message.rowId, message.chatMessage);
+        } else {
+            qCritical() << "deliverOfflineMsgs failed to send message";
+            addSavedMessage(message.rowId, message.chatMessage);
         }
-
-        registerReceipt(rec, key, val.msg);
     }
 }
 
-void OfflineMsgEngine::removeAllReceipts()
+/**
+* @brief Removes all messages which are being tracked.
+*/
+void OfflineMsgEngine::removeAllMessages()
 {
     QMutexLocker ml(&mutex);
-
+    receivedReceipts.clear();
-    receipts.clear();
+    sentSavedMessages.clear();
+    unsentSavedMessages.clear();
 }
 
-void OfflineMsgEngine::updateTimestamp(ReceiptNum receiptId)
+void OfflineMsgEngine::completeMessage(QMap<ReceiptNum, Message>::iterator msgIt)
 {
-    QMutexLocker ml(&mutex);
-
-    auto receipt = receipts.find(receiptId);
-    const auto msg = undeliveredMsgs.constFind(receipt->rowId);
-    if (msg == undeliveredMsgs.end()) {
-        // this should never occur as registerReceipt adds the msg before processReceipt calls updateTimestamp
-        qCritical() << "Message was not in undeliveredMsgs map when attempting to update its timestamp!";
-        return;
-    }
-    msg->msg->markAsSent(QDateTime::currentDateTime());
-    undeliveredMsgs.remove(receipt->rowId);
-    receipts.erase(receipt);
-}
-
-void OfflineMsgEngine::processReceipt(ReceiptNum receiptId)
-{
-    const auto receipt = receipts.constFind(receiptId);
-    if (receipt == receipts.end()) {
-        // this should never occur as callers ensure receipts contains receiptId
-        qCritical() << "Receipt was not added to map prior to attempting to process it!";
-        return;
-    }
-
-    if (!receipt->bRecepitReceived || !receipt->bRowValid)
-        return;
-
     Profile* const profile = Nexus::getProfile();
     if (profile->isHistoryEnabled()) {
-        profile->getHistory()->markAsSent(receipt->rowId);
+        profile->getHistory()->markAsSent(msgIt->rowId);
     }
 
     if (QThread::currentThread() == QCoreApplication::instance()->thread()) {
-        updateTimestamp(receiptId);
+        updateTimestamp(msgIt->chatMessage);
     } else {
-        QMetaObject::invokeMethod(this, "updateTimestamp", Qt::QueuedConnection, Q_ARG(ReceiptNum, receiptId));
+        QMetaObject::invokeMethod(this, "updateTimestamp", Qt::QueuedConnection, Q_ARG(ChatMessage::Ptr, msgIt->chatMessage));
     }
+    sentSavedMessages.erase(msgIt);
+    receivedReceipts.removeOne(msgIt.key());
+}
+
+void OfflineMsgEngine::checkForCompleteMessages(ReceiptNum receipt)
+{
+    auto msgIt = sentSavedMessages.find(receipt);
+    const bool receiptReceived = receivedReceipts.contains(receipt);
+    if (!receiptReceived || msgIt == sentSavedMessages.end()) {
+        return;
+    }
+    assert(!unsentSavedMessages.contains(*msgIt));
+    completeMessage(msgIt);
+}
+
+void OfflineMsgEngine::updateTimestamp(ChatMessage::Ptr msg)
+{
+    msg->markAsSent(QDateTime::currentDateTime());
 }

src/persistence/offlinemsgengine.h

@@ -28,6 +28,7 @@
 #include <QMutex>
 #include <QObject>
 #include <QSet>
+#include <chrono>
 
 class Friend;
 
@@ -36,36 +37,35 @@ class OfflineMsgEngine : public QObject
     Q_OBJECT
 public:
     explicit OfflineMsgEngine(Friend*);
-    virtual ~OfflineMsgEngine() = default;
+    void addSavedMessage(RowId messageID, ChatMessage::Ptr msg);
-
+    void addSentSavedMessage(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr msg);
-    void dischargeReceipt(ReceiptNum receipt);
-    void registerReceipt(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr msg);
     void deliverOfflineMsgs();
 
 public slots:
-    void removeAllReceipts();
+    void removeAllMessages();
-    void updateTimestamp(ReceiptNum receiptId);
+    void onReceiptReceived(ReceiptNum receipt);
 
 private:
-    void processReceipt(ReceiptNum receiptId);
+    struct Message
-    struct Receipt
     {
-        bool bRowValid{false};
+        bool operator==(const Message& rhs) const { return rhs.rowId == rowId; }
-        RowId rowId{0};
+        ChatMessage::Ptr chatMessage;
-        bool bRecepitReceived{false};
+        RowId rowId;
+        std::chrono::time_point<std::chrono::steady_clock> authorshipTime;
     };
 
-    struct MsgPtr
+private slots:
-    {
+    void completeMessage(QMap<ReceiptNum, Message>::iterator msgIt);
-        ChatMessage::Ptr msg;
+
-        ReceiptNum receipt;
+private:
-    };
+    void updateTimestamp(ChatMessage::Ptr msg);
+    void checkForCompleteMessages(ReceiptNum receipt);
+
     QMutex mutex;
-    Friend* f;
+    const Friend* f;
-    QHash<ReceiptNum, Receipt> receipts;
+    QVector<ReceiptNum> receivedReceipts;
-    QMap<RowId, MsgPtr> undeliveredMsgs;
+    QMap<ReceiptNum, Message> sentSavedMessages;
-
+    QVector<Message> unsentSavedMessages;
-    static const int offlineTimeout;
 };
 
 #endif // OFFLINEMSGENGINE_H

src/widget/form/chatform.cpp

@@ -689,7 +689,7 @@ void ChatForm::onStatusMessage(const QString& message)
 void ChatForm::onReceiptReceived(quint32 friendId, ReceiptNum receipt)
 {
     if (friendId == f->getId()) {
-        offlineEngine->dischargeReceipt(receipt);
+        offlineEngine->onReceiptReceived(receipt);
     }
 }
 
@@ -770,7 +770,7 @@ void ChatForm::dropEvent(QDropEvent* ev)
 void ChatForm::clearChatArea()
 {
     GenericChatForm::clearChatArea(/* confirm = */ false, /* inform = */ true);
-    offlineEngine->removeAllReceipts();
+    offlineEngine->removeAllMessages();
 }
 
 QString getMsgAuthorDispName(const ToxPk& authorPk, const QString& dispName)
@@ -931,15 +931,23 @@ void ChatForm::sendLoadedMessage(ChatMessage::Ptr chatMsg, MessageMetadata const
         return;
     }
 
-    ReceiptNum receipt{0};
+    ReceiptNum receipt;
+    bool  messageSent{false};
     if (f->getStatus() != Status::Offline) {
         Core* core = Core::getInstance();
         uint32_t friendId = f->getId();
         QString stringMsg = chatMsg->toString();
-        metadata.isAction ? core->sendAction(friendId, stringMsg, receipt)
+        messageSent = metadata.isAction ? core->sendAction(friendId, stringMsg, receipt)
                                         : core->sendMessage(friendId, stringMsg, receipt);
+        if (!messageSent) {
+            qWarning() << "Failed to send loaded message, adding to offline messaging";
+        }
+    }
+    if (messageSent) {
+        getOfflineMsgEngine()->addSentSavedMessage(receipt, metadata.id, chatMsg);
+    } else {
+        getOfflineMsgEngine()->addSavedMessage(metadata.id, chatMsg);
     }
-    getOfflineMsgEngine()->registerReceipt(receipt, metadata.id, chatMsg);
 }
 
 void ChatForm::onScreenshotClicked()
@@ -1128,11 +1136,15 @@ void ChatForm::SendMessageStr(QString msg)
             historyPart = ACTION_PREFIX + part;
         }
 
-        ReceiptNum receipt{0};
+        ReceiptNum receipt;
+        bool messageSent{false};
         if (f->getStatus() != Status::Offline) {
             Core* core = Core::getInstance();
             uint32_t friendId = f->getId();
-            isAction ? core->sendAction(friendId, part, receipt) : core->sendMessage(friendId, part, receipt);
+            messageSent = isAction ? core->sendAction(friendId, part, receipt) : core->sendMessage(friendId, part, receipt);
+            if (!messageSent) {
+                qCritical() << "Failed to send message, adding to offline messaging";
+            }
         }
 
         ChatMessage::Ptr ma = createSelfMessage(part, timestamp, isAction, false);
@@ -1144,8 +1156,12 @@ void ChatForm::SendMessageStr(QString msg)
             QString name = Core::getInstance()->getUsername();
             bool isSent = !Settings::getInstance().getFauxOfflineMessaging();
             history->addNewMessage(pk, historyPart, selfPk, timestamp, isSent, name,
-                                   [offMsgEngine, receipt, ma](RowId id) {
+                                   [messageSent, offMsgEngine, receipt, ma](RowId id) {
-                                       offMsgEngine->registerReceipt(receipt, id, ma);
+                                        if (messageSent) {
+                                            offMsgEngine->addSentSavedMessage(receipt, id, ma);
+                                        } else {
+                                            offMsgEngine->addSavedMessage(id, ma);
+                                        }
                                    });
         } else {
             // TODO: Make faux-offline messaging work partially with the history disabled

src/widget/form/settings/privacyform.cpp

@@ -58,8 +58,8 @@ PrivacyForm::~PrivacyForm()
 void PrivacyForm::on_cbKeepHistory_stateChanged()
 {
     Settings::getInstance().setEnableLogging(bodyUI->cbKeepHistory->isChecked());
-    Widget::getInstance()->clearAllReceipts();
     if (!bodyUI->cbKeepHistory->isChecked()) {
+        Widget::getInstance()->clearAllReceipts();
         QMessageBox::StandardButton dialogDelHistory;
         dialogDelHistory =
             QMessageBox::question(nullptr, tr("Confirmation"),

src/widget/widget.cpp

@@ -1218,7 +1218,7 @@ void Widget::onReceiptRecieved(int friendId, ReceiptNum receipt)
         return;
     }
 
-    chatForms[friendId]->getOfflineMsgEngine()->dischargeReceipt(receipt);
+    chatForms[friendId]->getOfflineMsgEngine()->onReceiptReceived(receipt);
 }
 
 void Widget::addFriendDialog(const Friend* frnd, ContentDialog* dialog)
@@ -2177,7 +2177,7 @@ void Widget::clearAllReceipts()
 {
     QList<Friend*> frnds = FriendList::getAllFriends();
     for (Friend* f : frnds) {
-        chatForms[f->getId()]->getOfflineMsgEngine()->removeAllReceipts();
+        chatForms[f->getId()]->getOfflineMsgEngine()->removeAllMessages();
     }
 }