From 7d2e5e200d8449a4492c6fafa4811197e6fbe40b Mon Sep 17 00:00:00 2001 From: Thanos Doukoudakis Date: Fri, 24 Mar 2017 14:15:52 +0000 Subject: Reset the chat state to active after a few seconds Fix for swift-217 When a user sends a composing Chat State Notification a timer will start. If the user doesn't send or cancel the message before the timer expires, an active CSN will be sent. Test Info: Build on Windows and unit test pass. Tested the new functionality with Windows and Linux Client. Added some test cases to cover the scenario that user goes idle while a CSN composing state has been sent. Updated ChatStateNotifierTest to use gtest. Updated ChatsManagerTest to use a valid TimerFactory object instead of nullptr. Change-Id: I35201947e4f042805a6d9df1340a0335effcd657 diff --git a/Swift/Controllers/Chat/ChatController.cpp b/Swift/Controllers/Chat/ChatController.cpp index 9cef9fc..8cbf059 100644 --- a/Swift/Controllers/Chat/ChatController.cpp +++ b/Swift/Controllers/Chat/ChatController.cpp @@ -55,7 +55,7 @@ ChatController::ChatController(const JID& self, StanzaChannel* stanzaChannel, IQ : ChatControllerBase(self, stanzaChannel, iqRouter, chatWindowFactory, contact, nickResolver, presenceOracle, avatarManager, useDelayForLatency, eventStream, eventController, timerFactory, entityCapsProvider, historyController, mucRegistry, highlightManager, chatMessageParser, autoAcceptMUCInviteDecider), userWantsReceipts_(userWantsReceipts), settings_(settings), clientBlockListManager_(clientBlockListManager) { isInMUC_ = isInMUC; lastWasPresence_ = false; - chatStateNotifier_ = new ChatStateNotifier(stanzaChannel, contact, entityCapsProvider); + chatStateNotifier_ = new ChatStateNotifier(stanzaChannel, contact, entityCapsProvider, timerFactory, 20000); chatStateTracker_ = new ChatStateTracker(); nickResolver_ = nickResolver; presenceOracle_->onPresenceChange.connect(boost::bind(&ChatController::handlePresenceChange, this, _1)); diff --git a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp index 1958408..2f77ec7 100644 --- a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp +++ b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include @@ -179,7 +180,8 @@ public: vcardManager_ = new VCardManager(jid_, iqRouter_, vcardStorage_); mocks_->ExpectCall(chatListWindowFactory_, ChatListWindowFactory::createChatListWindow).With(uiEventStream_).Return(chatListWindow_); clientBlockListManager_ = new ClientBlockListManager(iqRouter_); - manager_ = new ChatsManager(jid_, stanzaChannel_, iqRouter_, eventController_, chatWindowFactory_, joinMUCWindowFactory_, nickResolver_, presenceOracle_, directedPresenceSender_, uiEventStream_, chatListWindowFactory_, true, nullptr, mucRegistry_, entityCapsProvider_, mucManager_, mucSearchWindowFactory_, profileSettings_, ftOverview_, xmppRoster_, false, settings_, nullptr, wbManager_, highlightManager_, clientBlockListManager_, emoticons_, vcardManager_); + timerFactory_ = new DummyTimerFactory(); + manager_ = new ChatsManager(jid_, stanzaChannel_, iqRouter_, eventController_, chatWindowFactory_, joinMUCWindowFactory_, nickResolver_, presenceOracle_, directedPresenceSender_, uiEventStream_, chatListWindowFactory_, true, timerFactory_, mucRegistry_, entityCapsProvider_, mucManager_, mucSearchWindowFactory_, profileSettings_, ftOverview_, xmppRoster_, false, settings_, nullptr, wbManager_, highlightManager_, clientBlockListManager_, emoticons_, vcardManager_); manager_->setAvatarManager(avatarManager_); } @@ -190,6 +192,7 @@ public: delete eventNotifier_; delete avatarManager_; delete manager_; + delete timerFactory_; delete clientBlockListManager_; delete vcardManager_; delete vcardStorage_; @@ -213,6 +216,7 @@ public: delete chatListWindow_; delete mocks_; delete settings_; + } void testFirstOpenWindowIncoming() { @@ -1322,6 +1326,8 @@ private: std::map emoticons_; int handledHighlightActions_; std::set soundsPlayed_; + DummyTimerFactory* timerFactory_; + }; CPPUNIT_TEST_SUITE_REGISTRATION(ChatsManagerTest); diff --git a/Swiften/Chat/ChatStateNotifier.cpp b/Swiften/Chat/ChatStateNotifier.cpp index cbb9b0b..48a65ab 100644 --- a/Swiften/Chat/ChatStateNotifier.cpp +++ b/Swiften/Chat/ChatStateNotifier.cpp @@ -6,6 +6,7 @@ #include +#include #include #include @@ -14,16 +15,26 @@ #include #include #include +#include +#include + namespace Swift { -ChatStateNotifier::ChatStateNotifier(StanzaChannel* stanzaChannel, const JID& contact, EntityCapsProvider* entityCapsManager) : stanzaChannel_(stanzaChannel), entityCapsManager_(entityCapsManager), contact_(contact) { + +ChatStateNotifier::ChatStateNotifier(StanzaChannel* stanzaChannel, const JID& contact, EntityCapsProvider* entityCapsManager, TimerFactory* timerFactory, int idleTimeInMilliSecs) : stanzaChannel_(stanzaChannel), entityCapsManager_(entityCapsManager), contact_(contact) { setContact(contact); entityCapsManager_->onCapsChanged.connect(boost::bind(&ChatStateNotifier::handleCapsChanged, this, _1)); + assert(timerFactory); + idleTimer_ = timerFactory->createTimer(idleTimeInMilliSecs); + assert(!!idleTimer_); + idleTimer_->onTick.connect(boost::bind(&ChatStateNotifier::userBecameIdleWhileTyping, this)); } ChatStateNotifier::~ChatStateNotifier() { entityCapsManager_->onCapsChanged.disconnect(boost::bind(&ChatStateNotifier::handleCapsChanged, this, _1)); + idleTimer_->stop(); + idleTimer_->onTick.disconnect(boost::bind(&ChatStateNotifier::userBecameIdleWhileTyping, this)); } void ChatStateNotifier::setContact(const JID& contact) { @@ -39,24 +50,36 @@ void ChatStateNotifier::setContactIsOnline(bool online) { } void ChatStateNotifier::setUserIsTyping() { + idleTimer_->stop(); bool should = contactShouldReceiveStates(); if (should && !userIsTyping_) { userIsTyping_ = true; changeState(ChatState::Composing); } + if (should) { + idleTimer_->start(); + } } void ChatStateNotifier::userSentMessage() { + idleTimer_->stop(); userIsTyping_ = false; } void ChatStateNotifier::userCancelledNewMessage() { + idleTimer_->stop(); if (userIsTyping_) { userIsTyping_ = false; changeState(ChatState::Active); } } +void ChatStateNotifier::userBecameIdleWhileTyping() { + // For now we are returning to active state. When support for the Paused, Inactive and Gone states + // is implemeted, this function should Implement the Pause/Inactive functionality. + userCancelledNewMessage(); +} + void ChatStateNotifier::receivedMessageFromContact(bool hasActiveElement) { contactHasSentActive_ = hasActiveElement; } diff --git a/Swiften/Chat/ChatStateNotifier.h b/Swiften/Chat/ChatStateNotifier.h index a7af9e4..2b24c76 100644 --- a/Swiften/Chat/ChatStateNotifier.h +++ b/Swiften/Chat/ChatStateNotifier.h @@ -18,10 +18,13 @@ namespace Swift { class StanzaChannel; class EntityCapsProvider; + class TimerFactory; + class Timer; + class SWIFTEN_API ChatStateNotifier { public: - ChatStateNotifier(StanzaChannel* stanzaChannel, const JID& contact, EntityCapsProvider* entityCapsManager); + ChatStateNotifier(StanzaChannel* stanzaChannel, const JID& contact, EntityCapsProvider* entityCapsManager, TimerFactory* timerFactory, int idleTimeInMilliSecs); ~ChatStateNotifier(); void setContact(const JID& contact); @@ -36,6 +39,7 @@ namespace Swift { void setContactIsOnline(bool online); private: + void userBecameIdleWhileTyping(); bool contactShouldReceiveStates(); void changeState(ChatState::ChatStateType type); void handleCapsChanged(const JID& contact); @@ -48,5 +52,7 @@ namespace Swift { bool contactHasSentActive_; bool userIsTyping_; bool contactIsOnline_; + std::shared_ptr idleTimer_; + }; } diff --git a/Swiften/Chat/UnitTest/ChatStateNotifierTest.cpp b/Swiften/Chat/UnitTest/ChatStateNotifierTest.cpp index 7eeb531..efd37d9 100644 --- a/Swiften/Chat/UnitTest/ChatStateNotifierTest.cpp +++ b/Swiften/Chat/UnitTest/ChatStateNotifierTest.cpp @@ -6,163 +6,178 @@ #include -#include -#include +#include #include #include #include +#include using namespace Swift; -class ChatStateNotifierTest : public CppUnit::TestFixture { - CPPUNIT_TEST_SUITE(ChatStateNotifierTest); - CPPUNIT_TEST(testStartTypingReply_CapsNotIncluded); - CPPUNIT_TEST(testStartTypingReply_CapsIncluded); - CPPUNIT_TEST(testCancelledNewMessage); - CPPUNIT_TEST(testContinueTypingReply_CapsIncluded); - CPPUNIT_TEST(testTypeReplies_WentOffline); - CPPUNIT_TEST(testContactShouldReceiveStates_CapsOnly); - CPPUNIT_TEST(testContactShouldReceiveStates_CapsNorActive); - CPPUNIT_TEST(testContactShouldReceiveStates_ActiveOverrideOn); - CPPUNIT_TEST(testContactShouldReceiveStates_ActiveOverrideOff); - CPPUNIT_TEST_SUITE_END(); - -public: - void setUp() { +class ChatStateNotifierTest : public ::testing::Test { + +protected: + virtual void SetUp() { stanzaChannel = new DummyStanzaChannel(); stanzaChannel->setAvailable(true); entityCapsProvider = new DummyEntityCapsProvider(); - notifier_ = new ChatStateNotifier(stanzaChannel, JID("foo@bar.com/baz"), entityCapsProvider); + timerFactory_ = new DummyTimerFactory(); + notifier_ = new ChatStateNotifier(stanzaChannel, JID("foo@bar.com/baz"), entityCapsProvider, timerFactory_, 2); notifier_->setContactIsOnline(true); } - void tearDown() { + virtual void TearDown() { delete notifier_; + delete timerFactory_; delete entityCapsProvider; delete stanzaChannel; } - void testStartTypingReply_CapsNotIncluded() { - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(0, getComposingCount()); - } - - void testSendTwoMessages() { - setContactHas85Caps(); - notifier_->setUserIsTyping(); - notifier_->userSentMessage(); - notifier_->setUserIsTyping(); - notifier_->userSentMessage(); - CPPUNIT_ASSERT_EQUAL(2, getComposingCount()); - } - - void testCancelledNewMessage() { - setContactHas85Caps(); - notifier_->setUserIsTyping(); - notifier_->userCancelledNewMessage(); - CPPUNIT_ASSERT_EQUAL(1, getComposingCount()); - CPPUNIT_ASSERT_EQUAL(1, getActiveCount()); - CPPUNIT_ASSERT_EQUAL(ChatState::Active, stanzaChannel->sentStanzas[stanzaChannel->sentStanzas.size()-1]->getPayload()->getChatState()); - } - - - void testContactShouldReceiveStates_CapsOnly() { - setContactHas85Caps(); - std::shared_ptr message(new Message()); - notifier_->addChatStateRequest(message); - CPPUNIT_ASSERT(message->getPayload()); - CPPUNIT_ASSERT_EQUAL(ChatState::Active, message->getPayload()->getChatState()); - } - - void testContactShouldReceiveStates_CapsNorActive() { - std::shared_ptr message(new Message()); - notifier_->addChatStateRequest(message); - CPPUNIT_ASSERT(!message->getPayload()); - } - - void testContactShouldReceiveStates_ActiveOverrideOn() { - notifier_->receivedMessageFromContact(true); - std::shared_ptr message(new Message()); - notifier_->addChatStateRequest(message); - CPPUNIT_ASSERT(message->getPayload()); - CPPUNIT_ASSERT_EQUAL(ChatState::Active, message->getPayload()->getChatState()); - } - - void testContactShouldReceiveStates_ActiveOverrideOff() { - setContactHas85Caps(); - notifier_->receivedMessageFromContact(false); - /* I originally read the MUST NOT send after receiving without Active and - * thought this should check for false, but I later found it was OPTIONAL - * (MAY) behaviour only for if you didn't receive caps. - */ - std::shared_ptr message(new Message()); - notifier_->addChatStateRequest(message); - CPPUNIT_ASSERT(message->getPayload()); - CPPUNIT_ASSERT_EQUAL(ChatState::Active, message->getPayload()->getChatState()); - } - - - void testStartTypingReply_CapsIncluded() { - setContactHas85Caps(); - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(1, getComposingCount()); - } - - void testContinueTypingReply_CapsIncluded() { - setContactHas85Caps(); - notifier_->setUserIsTyping(); - notifier_->setUserIsTyping(); - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(1, getComposingCount()); - notifier_->userSentMessage(); - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(2, getComposingCount()); - + void setContactHas85Caps() { + DiscoInfo::ref caps(new DiscoInfo()); + caps->addFeature(DiscoInfo::ChatStatesFeature); + entityCapsProvider->caps[JID("foo@bar.com/baz")] = caps; + entityCapsProvider->onCapsChanged(JID("foo@bar.com/baz")); } - void testTypeReplies_WentOffline() { - setContactHas85Caps(); - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(1, getComposingCount()); - notifier_->setContactIsOnline(false); - notifier_->userSentMessage(); - notifier_->setUserIsTyping(); - CPPUNIT_ASSERT_EQUAL(1, getComposingCount()); - } - - private: - void setContactHas85Caps() { - DiscoInfo::ref caps(new DiscoInfo()); - caps->addFeature(DiscoInfo::ChatStatesFeature); - entityCapsProvider->caps[JID("foo@bar.com/baz")] = caps; - entityCapsProvider->onCapsChanged(JID("foo@bar.com/baz")); - } - - int getComposingCount() const { - int result = 0; - for (auto&& stanza : stanzaChannel->sentStanzas) { - if (stanza->getPayload() && stanza->getPayload()->getChatState() == ChatState::Composing) { - result++; - } + int getComposingCount() const { + int result = 0; + for (auto&& stanza : stanzaChannel->sentStanzas) { + if (stanza->getPayload() && stanza->getPayload()->getChatState() == ChatState::Composing) { + result++; } - return result; } + return result; + } - int getActiveCount() const { - int result = 0; - for (auto&& stanza : stanzaChannel->sentStanzas) { - if (stanza->getPayload() && stanza->getPayload()->getChatState() == ChatState::Active) { - result++; - } + int getActiveCount() const { + int result = 0; + for (auto&& stanza : stanzaChannel->sentStanzas) { + if (stanza->getPayload() && stanza->getPayload()->getChatState() == ChatState::Active) { + result++; } - return result; } + return result; + } - private: - DummyStanzaChannel* stanzaChannel; - DummyEntityCapsProvider* entityCapsProvider; - ChatStateNotifier* notifier_; + DummyStanzaChannel* stanzaChannel; + DummyEntityCapsProvider* entityCapsProvider; + DummyTimerFactory* timerFactory_; + ChatStateNotifier* notifier_; }; -CPPUNIT_TEST_SUITE_REGISTRATION(ChatStateNotifierTest); +TEST_F(ChatStateNotifierTest, testStartTypingReply_CapsNotIncluded) { + notifier_->setUserIsTyping(); + ASSERT_EQ(0, getComposingCount()); +} + +TEST_F(ChatStateNotifierTest, testSendTwoMessages) { + setContactHas85Caps(); + notifier_->setUserIsTyping(); + notifier_->userSentMessage(); + notifier_->setUserIsTyping(); + notifier_->userSentMessage(); + ASSERT_EQ(2, getComposingCount()); +} + +TEST_F(ChatStateNotifierTest, testCancelledNewMessage) { + setContactHas85Caps(); + notifier_->setUserIsTyping(); + notifier_->userCancelledNewMessage(); + ASSERT_EQ(1, getComposingCount()); + ASSERT_EQ(1, getActiveCount()); + ASSERT_EQ(ChatState::Active, stanzaChannel->sentStanzas[stanzaChannel->sentStanzas.size() - 1]->getPayload()->getChatState()); +} + +TEST_F(ChatStateNotifierTest, testIdleWhileTypingNewMessage) { + setContactHas85Caps(); + //The channel should be empty + ASSERT_EQ(0, getComposingCount()); + ASSERT_EQ(0, getActiveCount()); + notifier_->setUserIsTyping(); + timerFactory_->setTime(1); + //1 Composing stanza is expected + ASSERT_EQ(1, getComposingCount()); + ASSERT_EQ(0, getActiveCount()); + timerFactory_->setTime(2); + //The idleTimer period has expired, the channel should have 1 composing and 1 active status stanza + ASSERT_EQ(1, getComposingCount()); + ASSERT_EQ(1, getActiveCount()); + ASSERT_EQ(ChatState::Active, stanzaChannel->sentStanzas[stanzaChannel->sentStanzas.size() - 1]->getPayload()->getChatState()); + timerFactory_->setTime(4); + //At the second tick no further state stanzas should be sent. + ASSERT_EQ(1, getComposingCount()); + ASSERT_EQ(1, getActiveCount()); + ASSERT_EQ(ChatState::Active, stanzaChannel->sentStanzas[stanzaChannel->sentStanzas.size() - 1]->getPayload()->getChatState()); +} + +TEST_F(ChatStateNotifierTest, testIdleWhileTypingNewMessageNoCaps) { + notifier_->setUserIsTyping(); + timerFactory_->setTime(3); + ASSERT_EQ(0, getComposingCount()); + ASSERT_EQ(0, getActiveCount()); +} +TEST_F(ChatStateNotifierTest, testContactShouldReceiveStates_CapsOnly) { + setContactHas85Caps(); + std::shared_ptr message(new Message()); + notifier_->addChatStateRequest(message); + EXPECT_TRUE(message->getPayload()); + ASSERT_EQ(ChatState::Active, message->getPayload()->getChatState()); +} + +TEST_F(ChatStateNotifierTest, testContactShouldReceiveStates_CapsNorActive) { + std::shared_ptr message(new Message()); + notifier_->addChatStateRequest(message); + EXPECT_TRUE(!message->getPayload()); +} + +TEST_F(ChatStateNotifierTest, testContactShouldReceiveStates_ActiveOverrideOn) { + notifier_->receivedMessageFromContact(true); + std::shared_ptr message(new Message()); + notifier_->addChatStateRequest(message); + EXPECT_TRUE(message->getPayload()); + ASSERT_EQ(ChatState::Active, message->getPayload()->getChatState()); +} + +TEST_F(ChatStateNotifierTest, testContactShouldReceiveStates_ActiveOverrideOff) { + setContactHas85Caps(); + notifier_->receivedMessageFromContact(false); + /* I originally read the MUST NOT send after receiving without Active and + * thought this should check for false, but I later found it was OPTIONAL + * (MAY) behaviour only for if you didn't receive caps. + */ + std::shared_ptr message(new Message()); + notifier_->addChatStateRequest(message); + EXPECT_TRUE(message->getPayload()); + ASSERT_EQ(ChatState::Active, message->getPayload()->getChatState()); +} + + +TEST_F(ChatStateNotifierTest, testStartTypingReply_CapsIncluded) { + setContactHas85Caps(); + notifier_->setUserIsTyping(); + ASSERT_EQ(1, getComposingCount()); +} + +TEST_F(ChatStateNotifierTest, testContinueTypingReply_CapsIncluded) { + setContactHas85Caps(); + notifier_->setUserIsTyping(); + notifier_->setUserIsTyping(); + notifier_->setUserIsTyping(); + ASSERT_EQ(1, getComposingCount()); + notifier_->userSentMessage(); + notifier_->setUserIsTyping(); + ASSERT_EQ(2, getComposingCount()); + +} + +TEST_F(ChatStateNotifierTest, testTypeReplies_WentOffline) { + setContactHas85Caps(); + notifier_->setUserIsTyping(); + ASSERT_EQ(1, getComposingCount()); + notifier_->setContactIsOnline(false); + notifier_->userSentMessage(); + notifier_->setUserIsTyping(); + ASSERT_EQ(1, getComposingCount()); +} -- cgit v0.10.2-6-g49f6