From 57a611f942f2387e497334888a4bc4f68b9275c9 Mon Sep 17 00:00:00 2001 From: Tobias Markmann <tm@ayena.de> Date: Wed, 17 Aug 2016 11:02:24 +0200 Subject: Support Last Message Correction in multi client scenarios Previously Last Message Correction edits are only applied if they came from the same resource. This makes sense in MUC scenarios but does not in 1-to-1 chats. This changes the Last Message Correction behaviour for MUC and 1-to-1 chats so that different clients from the same bare JID can edit each others messages. Test-Information: Added unit test to verify Last Message Corrections work as expected when coming from the same client and from different clients. Manually verified that the receiving client correctly shows a corrected message if the sending client reconnected between first message and edit. All unit tests pass on OS X 10.11.6 with Qt 5.5.1. Change-Id: If533ecc7032e59e324979c577726f2da739012e6 diff --git a/Swift/ChangeLog.md b/Swift/ChangeLog.md index a174436..68d1554 100644 --- a/Swift/ChangeLog.md +++ b/Swift/ChangeLog.md @@ -3,6 +3,7 @@ - Fix UI layout issue for translations that require right-to-left (RTL) layout - macOS releases are now code-signed with a key from Apple, so they can be run without Gatekeeper trust warnings - Handle sessions being closed by the server +- Fix Last Message Correction in multi client scenarios - Fix display of default avatar on Windows 4.0-beta2 ( 2016-07-20 ) diff --git a/Swift/Controllers/Chat/ChatController.cpp b/Swift/Controllers/Chat/ChatController.cpp index 3bc9d20..3e58e40 100644 --- a/Swift/Controllers/Chat/ChatController.cpp +++ b/Swift/Controllers/Chat/ChatController.cpp @@ -565,6 +565,10 @@ bool ChatController::shouldIgnoreMessage(std::shared_ptr<Message> message) { } return false; } + +JID ChatController::messageCorrectionJID(const JID& fromJID) { + return fromJID.toBare(); +} ChatWindow* ChatController::detachChatWindow() { chatWindow_->onUserTyping.disconnect(boost::bind(&ChatStateNotifier::setUserIsTyping, chatStateNotifier_)); diff --git a/Swift/Controllers/Chat/ChatController.h b/Swift/Controllers/Chat/ChatController.h index ffc6be9..1deb0bb 100644 --- a/Swift/Controllers/Chat/ChatController.h +++ b/Swift/Controllers/Chat/ChatController.h @@ -48,6 +48,7 @@ namespace Swift { virtual JID getBaseJID() SWIFTEN_OVERRIDE; virtual void logMessage(const std::string& message, const JID& fromJID, const JID& toJID, const boost::posix_time::ptime& timeStamp, bool isIncoming) SWIFTEN_OVERRIDE; virtual bool shouldIgnoreMessage(std::shared_ptr<Message> message) SWIFTEN_OVERRIDE; + virtual JID messageCorrectionJID(const JID& fromJID) SWIFTEN_OVERRIDE; private: void handlePresenceChange(std::shared_ptr<Presence> newPresence); diff --git a/Swift/Controllers/Chat/ChatControllerBase.cpp b/Swift/Controllers/Chat/ChatControllerBase.cpp index ecaf186..7ae7dbd 100644 --- a/Swift/Controllers/Chat/ChatControllerBase.cpp +++ b/Swift/Controllers/Chat/ChatControllerBase.cpp @@ -279,10 +279,10 @@ void ChatControllerBase::handleIncomingMessage(std::shared_ptr<MessageEvent> mes if (replace) { // Should check if the user has a previous message std::map<JID, std::string>::iterator lastMessage; - lastMessage = lastMessagesUIID_.find(from); + lastMessage = lastMessagesUIID_.find(messageCorrectionJID(from)); if (lastMessage != lastMessagesUIID_.end()) { chatMessage = buildChatWindowChatMessage(body, senderHighlightNameFromMessage(from), senderIsSelf); - replaceMessage(chatMessage, lastMessagesUIID_[from], timeStamp); + replaceMessage(chatMessage, lastMessagesUIID_[messageCorrectionJID(from)], timeStamp); } } else { @@ -298,7 +298,7 @@ void ChatControllerBase::handleIncomingMessage(std::shared_ptr<MessageEvent> mes } void ChatControllerBase::addMessageHandleIncomingMessage(const JID& from, const ChatWindow::ChatMessage& message, bool senderIsSelf, std::shared_ptr<SecurityLabel> label, const boost::posix_time::ptime& timeStamp) { - lastMessagesUIID_[from] = addMessage(message, senderDisplayNameFromMessage(from), senderIsSelf, label, avatarManager_->getAvatarPath(from), timeStamp); + lastMessagesUIID_[messageCorrectionJID(from)] = addMessage(message, senderDisplayNameFromMessage(from), senderIsSelf, label, avatarManager_->getAvatarPath(from), timeStamp); } std::string ChatControllerBase::getErrorMessage(std::shared_ptr<ErrorPayload> error) { diff --git a/Swift/Controllers/Chat/ChatControllerBase.h b/Swift/Controllers/Chat/ChatControllerBase.h index c84c4d4..d10df8f 100644 --- a/Swift/Controllers/Chat/ChatControllerBase.h +++ b/Swift/Controllers/Chat/ChatControllerBase.h @@ -101,6 +101,11 @@ namespace Swift { return false; } + /** + * What JID should be used for last message correction (XEP-0308) tracking. + */ + virtual JID messageCorrectionJID(const JID& fromJID) = 0; + private: IDGenerator idGenerator_; std::string lastSentMessageStanzaID_; diff --git a/Swift/Controllers/Chat/MUCController.cpp b/Swift/Controllers/Chat/MUCController.cpp index 73cf748..b685e04 100644 --- a/Swift/Controllers/Chat/MUCController.cpp +++ b/Swift/Controllers/Chat/MUCController.cpp @@ -1078,6 +1078,10 @@ void MUCController::logMessage(const std::string& message, const JID& fromJID, c } } +JID MUCController::messageCorrectionJID(const JID& fromJID) { + return fromJID; +} + void MUCController::addRecentLogs() { if (!historyController_) { return; diff --git a/Swift/Controllers/Chat/MUCController.h b/Swift/Controllers/Chat/MUCController.h index 9d6428b..ba68ec6 100644 --- a/Swift/Controllers/Chat/MUCController.h +++ b/Swift/Controllers/Chat/MUCController.h @@ -85,6 +85,7 @@ namespace Swift { virtual void postHandleIncomingMessage(std::shared_ptr<MessageEvent>, const ChatWindow::ChatMessage& chatMessage) SWIFTEN_OVERRIDE; virtual void cancelReplaces() SWIFTEN_OVERRIDE; virtual void logMessage(const std::string& message, const JID& fromJID, const JID& toJID, const boost::posix_time::ptime& timeStamp, bool isIncoming) SWIFTEN_OVERRIDE; + virtual JID messageCorrectionJID(const JID& fromJID) SWIFTEN_OVERRIDE; private: void setAvailableRoomActions(const MUCOccupant::Affiliation& affiliation, const MUCOccupant::Role& role); diff --git a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp index d104fbd..0356c6a 100644 --- a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp +++ b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp @@ -137,6 +137,11 @@ class ChatsManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testCarbonsForwardedOutgoingMessageFromSecondResource); CPPUNIT_TEST(testCarbonsForwardedIncomingDuplicates); + + // Message correction tests + CPPUNIT_TEST(testChatControllerMessageCorrectionReplaceBySameResource); + CPPUNIT_TEST(testChatControllerMessageCorrectionReplaceByOtherResource); + CPPUNIT_TEST_SUITE_END(); public: @@ -217,7 +222,6 @@ public: delete chatListWindow_; delete mocks_; delete settings_; - } void testFirstOpenWindowIncoming() { @@ -732,7 +736,7 @@ public: presence->setShow(StatusShow::None); presence->setType(Presence::Unavailable); stanzaChannel_->onPresenceReceived(presence); - CPPUNIT_ASSERT_EQUAL(std::string("participantA has gone offline."), MockChatWindow::bodyFromMessage(window->lastReplacedMessage_)); + CPPUNIT_ASSERT_EQUAL(std::string("participantA has gone offline."), MockChatWindow::bodyFromMessage(window->lastReplacedLastMessage_)); } void testChatControllerMucPmUnavailableErrorHandling() { @@ -1097,7 +1101,7 @@ public: CPPUNIT_ASSERT_EQUAL(JID("room@muc.service.com"), window->lastMUCInvitationJID_); stanzaChannel_->onPresenceReceived(generateIncomingPresence(Presence::Unavailable)); - CPPUNIT_ASSERT_EQUAL(std::string(""), MockChatWindow::bodyFromMessage(window->lastReplacedMessage_)); + CPPUNIT_ASSERT_EQUAL(std::string(""), MockChatWindow::bodyFromMessage(window->lastReplacedLastMessage_)); CPPUNIT_ASSERT_EQUAL(std::string("testling@test.com has gone offline."), MockChatWindow::bodyFromMessage(window->lastAddedPresence_)); } @@ -1134,7 +1138,7 @@ public: stanzaChannel_->onPresenceReceived(presence); } CPPUNIT_ASSERT_EQUAL(std::string("someDifferentNickname has entered the room."), window->bodyFromMessage(window->lastAddedPresence_)); - CPPUNIT_ASSERT_EQUAL(std::string(), window->bodyFromMessage(window->lastReplacedMessage_)); + CPPUNIT_ASSERT_EQUAL(std::string(), window->bodyFromMessage(window->lastReplacedLastMessage_)); window->resetLastMessages(); { @@ -1145,7 +1149,7 @@ public: stanzaChannel_->onPresenceReceived(presence); } CPPUNIT_ASSERT_EQUAL(std::string(), window->bodyFromMessage(window->lastAddedPresence_)); - CPPUNIT_ASSERT_EQUAL(std::string("someDifferentNickname and Romeo have entered the room"), window->bodyFromMessage(window->lastReplacedMessage_)); + CPPUNIT_ASSERT_EQUAL(std::string("someDifferentNickname and Romeo have entered the room"), window->bodyFromMessage(window->lastReplacedLastMessage_)); window->resetLastMessages(); { @@ -1165,7 +1169,7 @@ public: stanzaChannel_->onPresenceReceived(presence); } CPPUNIT_ASSERT_EQUAL(std::string(), window->bodyFromMessage(window->lastAddedPresence_)); - CPPUNIT_ASSERT_EQUAL(std::string("someDifferentNickname, Romeo and Juliet have entered the room"), window->bodyFromMessage(window->lastReplacedMessage_)); + CPPUNIT_ASSERT_EQUAL(std::string("someDifferentNickname, Romeo and Juliet have entered the room"), window->bodyFromMessage(window->lastReplacedLastMessage_)); } template <typename CarbonsType> @@ -1265,21 +1269,21 @@ public: CPPUNIT_ASSERT_EQUAL(ChatWindow::ReceiptReceived, window->receiptChanges_[1].second); } } - + void testCarbonsForwardedIncomingDuplicates() { JID messageJID("testling@test.com/resource1"); JID jid2 = jid_.toBare().withResource("someOtherResource"); - + MockChatWindow* window = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID, uiEventStream_).Return(window); - + std::shared_ptr<Message> message(new Message()); message->setFrom(messageJID); std::string body("This is a legible message. >HEH@)oeueu"); message->setBody(body); manager_->handleIncomingMessage(message); CPPUNIT_ASSERT_EQUAL(body, MockChatWindow::bodyFromMessage(window->lastAddedMessage_)); - + // incoming carbons message from another resource and duplicate of it { auto originalMessage = std::make_shared<Message>(); @@ -1289,15 +1293,15 @@ public: originalMessage->setType(Message::Chat); std::string forwardedBody = "Some further text."; originalMessage->setBody(forwardedBody); - + auto messageWrapper = createCarbonsMessage(std::make_shared<CarbonsReceived>(), originalMessage); - + manager_->handleIncomingMessage(messageWrapper); - + CPPUNIT_ASSERT_EQUAL(forwardedBody, MockChatWindow::bodyFromMessage(window->lastAddedMessage_)); CPPUNIT_ASSERT_EQUAL(false, window->lastAddedMessageSenderIsSelf_); window->resetLastMessages(); - + messageWrapper = createCarbonsMessage(std::make_shared<CarbonsReceived>(), originalMessage); manager_->handleIncomingMessage(messageWrapper); CPPUNIT_ASSERT_EQUAL(std::string(), MockChatWindow::bodyFromMessage(window->lastAddedMessage_)); @@ -1305,6 +1309,58 @@ public: } } + void testChatControllerMessageCorrectionReplaceBySameResource() { + JID messageJID("testling@test.com/resource1"); + + MockChatWindow* window = new MockChatWindow(); + mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID, uiEventStream_).Return(window); + + auto message = std::make_shared<Message>(); + message->setFrom(messageJID); + message->setTo(jid_); + message->setType(Message::Chat); + message->setBody("text before edit"); + manager_->handleIncomingMessage(message); + + CPPUNIT_ASSERT_EQUAL(std::string("text before edit"), MockChatWindow::bodyFromMessage(window->lastAddedMessage_)); + + message = std::make_shared<Message>(); + message->setFrom(messageJID); + message->setTo(jid_); + message->setType(Message::Chat); + message->setBody("text after edit"); + message->addPayload(std::make_shared<Replace>("someID")); + manager_->handleIncomingMessage(message); + + CPPUNIT_ASSERT_EQUAL(std::string("text after edit"), MockChatWindow::bodyFromMessage(window->lastReplacedMessage_)); + } + + void testChatControllerMessageCorrectionReplaceByOtherResource() { + JID messageJID("testling@test.com/resource1"); + + MockChatWindow* window = new MockChatWindow(); + mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID, uiEventStream_).Return(window); + + auto message = std::make_shared<Message>(); + message->setFrom(messageJID); + message->setTo(jid_); + message->setType(Message::Chat); + message->setBody("text before edit"); + manager_->handleIncomingMessage(message); + + CPPUNIT_ASSERT_EQUAL(std::string("text before edit"), MockChatWindow::bodyFromMessage(window->lastAddedMessage_)); + + message = std::make_shared<Message>(); + message->setFrom(messageJID.toBare().withResource("resource2")); + message->setTo(jid_); + message->setType(Message::Chat); + message->setBody("text after edit"); + message->addPayload(std::make_shared<Replace>("someID")); + manager_->handleIncomingMessage(message); + + CPPUNIT_ASSERT_EQUAL(std::string("text after edit"), MockChatWindow::bodyFromMessage(window->lastReplacedMessage_)); + } + private: std::shared_ptr<Message> makeDeliveryReceiptTestMessage(const JID& from, const std::string& id) { std::shared_ptr<Message> message = std::make_shared<Message>(); diff --git a/Swift/Controllers/UnitTest/MockChatWindow.h b/Swift/Controllers/UnitTest/MockChatWindow.h index 84c86fc..7682781 100644 --- a/Swift/Controllers/UnitTest/MockChatWindow.h +++ b/Swift/Controllers/UnitTest/MockChatWindow.h @@ -43,10 +43,14 @@ namespace Swift { virtual void addErrorMessage(const ChatMessage& message) { lastAddedErrorMessage_ = message; } - virtual void replaceMessage(const ChatMessage& /*message*/, const std::string& /*id*/, const boost::posix_time::ptime& /*time*/) {} + + virtual void replaceMessage(const ChatMessage& message, const std::string& /*id*/, const boost::posix_time::ptime& /*time*/) { + lastReplacedMessage_ = message; + } + virtual void replaceWithAction(const ChatMessage& /*message*/, const std::string& /*id*/, const boost::posix_time::ptime& /*time*/) {} virtual void replaceLastMessage(const ChatMessage& message, const TimestampBehaviour /*timestampBehaviour*/) { - lastReplacedMessage_ = message; + lastReplacedLastMessage_ = message; } virtual void replaceSystemMessage(const ChatMessage& message, const std::string& /*id*/, const TimestampBehaviour /*timestampBehaviour*/) { lastReplacedSystemMessage_ = message; @@ -124,7 +128,7 @@ namespace Swift { } void resetLastMessages() { - lastAddedMessage_ = lastAddedAction_ = lastAddedPresence_ = lastReplacedMessage_ = lastAddedSystemMessage_ = lastReplacedSystemMessage_ = ChatMessage(); + lastAddedMessage_ = lastAddedAction_ = lastAddedPresence_ = lastReplacedLastMessage_ = lastAddedSystemMessage_ = lastReplacedSystemMessage_ = ChatMessage(); lastAddedMessageSenderName_ = lastAddedActionSenderName_ = ""; lastAddedMessageSenderIsSelf_ = lastAddedActionSenderIsSelf_ = false; } @@ -138,6 +142,7 @@ namespace Swift { bool lastAddedActionSenderIsSelf_ = false; ChatMessage lastAddedPresence_; ChatMessage lastReplacedMessage_; + ChatMessage lastReplacedLastMessage_; ChatMessage lastAddedSystemMessage_; ChatMessage lastReplacedSystemMessage_; ChatMessage lastAddedErrorMessage_; diff --git a/Swiften/Base/LogSerializers.h b/Swiften/Base/LogSerializers.h index b804808..7f73686 100644 --- a/Swiften/Base/LogSerializers.h +++ b/Swiften/Base/LogSerializers.h @@ -6,9 +6,11 @@ #pragma once +#include <map> #include <memory> #include <ostream> #include <string> +#include <utility> #include <vector> #include <boost/optional.hpp> @@ -40,11 +42,33 @@ std::ostream& operator<<(std::ostream& stream, const std::vector<T>& vec) { for (auto end = std::end(vec); it != end; ++it) { stream << ", " << *it; } - } + } stream << "]"; return stream; } +template <typename KEY, typename VALUE> +std::ostream& operator<<(std::ostream& stream, const std::pair<KEY, VALUE>& pair) { + stream << pair.first << ":" << pair.second; + return stream; +} + +template <typename KEY, typename VALUE> +std::ostream& operator<<(std::ostream& stream, const std::map<KEY, VALUE>& map) { + stream << "{"; + if (!map.empty()) { + auto it = std::begin(map); + stream << *it; + + ++it; + for (auto end = std::end(map); it != end; ++it) { + stream << ", " << *it; + } + } + stream << "}"; + return stream; +} + std::ostream& operator<<(std::ostream& stream, const Presence& presence); std::ostream& operator<<(std::ostream& stream, const BOSHError& boshError); -- cgit v0.10.2-6-g49f6