From e7f523bc9ae40eb6779300bd7d5cc32972d2c65a Mon Sep 17 00:00:00 2001 From: Anthony Bilinski Date: Sun, 27 Jan 2019 03:52:29 -0800 Subject: [PATCH] refactor(messages): don't use receipt 0 as unsent, simplify class state --- src/persistence/offlinemsgengine.cpp | 174 ++++++++++++++--------- src/persistence/offlinemsgengine.h | 42 +++--- src/widget/form/chatform.cpp | 36 +++-- src/widget/form/settings/privacyform.cpp | 2 +- src/widget/widget.cpp | 4 +- 5 files changed, 154 insertions(+), 104 deletions(-) diff --git a/src/persistence/offlinemsgengine.cpp b/src/persistence/offlinemsgengine.cpp index 32584ff02..1d925b6da 100644 --- a/src/persistence/offlinemsgengine.cpp +++ b/src/persistence/offlinemsgengine.cpp @@ -26,6 +26,7 @@ #include #include #include +#include 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); - - auto it = receipts.find(receipt); - if (it == receipts.end()) { - it = receipts.insert(receipt, Receipt()); - } else if (it->bRecepitReceived) { - qWarning() << "Received duplicate receipt"; + if (receivedReceipts.contains(receipt)) { + qWarning() << "Receievd duplicate receipt" << receipt.get() << "from friend" << f->getId(); + return; } - it->bRecepitReceived = true; - processReceipt(receipt); + receivedReceipts.append(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); - - auto it = receipts.find(receipt); - if (it == receipts.end()) { - it = receipts.insert(receipt, Receipt()); - } else if (it->bRowValid && receipt.get() != 0 /* offline receipt */) { - qWarning() << "Received duplicate registration of receipt"; - } - it->rowId = messageID; - it->bRowValid = true; - undeliveredMsgs[messageID] = {msg, receipt}; - processReceipt(receipt); + assert([&](){ + for (const auto& message : unsentSavedMessages) { + if (message.rowId == messageID) { + return false; + } + } + return true; + }()); + 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 msgs = undeliveredMsgs; - removeAllReceipts(); - undeliveredMsgs.clear(); + QVector messages = sentSavedMessages.values().toVector() + unsentSavedMessages; + // order messages by authorship time to resend in same order as they were written + 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) { - auto val = iter.value(); - auto key = iter.key(); - QString messageText = val.msg->toString(); - ReceiptNum rec; - if (val.msg->isAction()) { - Core::getInstance()->sendAction(f->getId(), messageText, rec); + for (const auto& message : messages) { + QString messageText = message.chatMessage->toString(); + ReceiptNum receipt; + bool messageSent{false}; + if (message.chatMessage->isAction()) { + messageSent = Core::getInstance()->sendAction(f->getId(), messageText, receipt); } 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); - - receipts.clear(); + receivedReceipts.clear(); + sentSavedMessages.clear(); + unsentSavedMessages.clear(); } -void OfflineMsgEngine::updateTimestamp(ReceiptNum receiptId) +void OfflineMsgEngine::completeMessage(QMap::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()); } diff --git a/src/persistence/offlinemsgengine.h b/src/persistence/offlinemsgengine.h index a6671e28f..2e044eade 100644 --- a/src/persistence/offlinemsgengine.h +++ b/src/persistence/offlinemsgengine.h @@ -28,6 +28,7 @@ #include #include #include +#include class Friend; @@ -36,36 +37,35 @@ class OfflineMsgEngine : public QObject Q_OBJECT public: explicit OfflineMsgEngine(Friend*); - virtual ~OfflineMsgEngine() = default; - - void dischargeReceipt(ReceiptNum receipt); - void registerReceipt(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr msg); + void addSavedMessage(RowId messageID, ChatMessage::Ptr msg); + void addSentSavedMessage(ReceiptNum receipt, RowId messageID, ChatMessage::Ptr msg); void deliverOfflineMsgs(); public slots: - void removeAllReceipts(); - void updateTimestamp(ReceiptNum receiptId); + void removeAllMessages(); + void onReceiptReceived(ReceiptNum receipt); private: - void processReceipt(ReceiptNum receiptId); - struct Receipt + struct Message { - bool bRowValid{false}; - RowId rowId{0}; - bool bRecepitReceived{false}; + bool operator==(const Message& rhs) const { return rhs.rowId == rowId; } + ChatMessage::Ptr chatMessage; + RowId rowId; + std::chrono::time_point authorshipTime; }; - struct MsgPtr - { - ChatMessage::Ptr msg; - ReceiptNum receipt; - }; +private slots: + void completeMessage(QMap::iterator msgIt); + +private: + void updateTimestamp(ChatMessage::Ptr msg); + void checkForCompleteMessages(ReceiptNum receipt); + QMutex mutex; - Friend* f; - QHash receipts; - QMap undeliveredMsgs; - - static const int offlineTimeout; + const Friend* f; + QVector receivedReceipts; + QMap sentSavedMessages; + QVector unsentSavedMessages; }; #endif // OFFLINEMSGENGINE_H diff --git a/src/widget/form/chatform.cpp b/src/widget/form/chatform.cpp index 43da3ded8..08c9a9777 100644 --- a/src/widget/form/chatform.cpp +++ b/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) - : core->sendMessage(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) { - offMsgEngine->registerReceipt(receipt, id, ma); + [messageSent, offMsgEngine, receipt, ma](RowId id) { + if (messageSent) { + offMsgEngine->addSentSavedMessage(receipt, id, ma); + } else { + offMsgEngine->addSavedMessage(id, ma); + } }); } else { // TODO: Make faux-offline messaging work partially with the history disabled diff --git a/src/widget/form/settings/privacyform.cpp b/src/widget/form/settings/privacyform.cpp index 5ee574d47..211d2cbc3 100644 --- a/src/widget/form/settings/privacyform.cpp +++ b/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"), diff --git a/src/widget/widget.cpp b/src/widget/widget.cpp index 89c7ee923..d1caba302 100644 --- a/src/widget/widget.cpp +++ b/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 frnds = FriendList::getAllFriends(); for (Friend* f : frnds) { - chatForms[f->getId()]->getOfflineMsgEngine()->removeAllReceipts(); + chatForms[f->getId()]->getOfflineMsgEngine()->removeAllMessages(); } }