refactor(group): move peer tracking logic to Group from GroupChatForm

* increase signal granularity
* reduce state in GroupChatForm
* remove differentiation of "joined" and "online" peers, it doesn't exist in toxcore and can't be tracked reliably in qTox
* add system message when peer name changes, even if due to alias
* add system message when self name changes, for clarity

src/model/group.cpp

@@ -53,8 +53,8 @@ void Group::setName(const QString& newTitle)
     if (!shortTitle.isEmpty() && title != shortTitle) {
         title = shortTitle;
         emit displayedNameChanged(title);
-        emit titleChangedByUser(groupId, title);
-        emit titleChanged(groupId, selfName, title);
+        emit titleChangedByUser(title);
+        emit titleChanged(selfName, title);
     }
 }
 
@@ -64,7 +64,7 @@ void Group::setTitle(const QString& author, const QString& newTitle)
     if (!shortTitle.isEmpty() && title != shortTitle) {
         title = shortTitle;
         emit displayedNameChanged(title);
-        emit titleChanged(groupId, author, title);
+        emit titleChanged(author, title);
     }
 }
 
@@ -80,40 +80,52 @@ QString Group::getDisplayedName() const
 
 void Group::regeneratePeerList()
 {
+    // NOTE: there's a bit of a race here. Core emits a signal for both groupPeerlistChanged and groupPeerNameChanged
+    // back-to-back when a peer joins our group. If we get both before we process this slot, core->getGroupPeerNames
+    // will contain the new peer name, and we'll ignore the name changed signal, and emit a single userJoined with
+    // the correct name. But, if we receive the name changed signal a little later, we will emit userJoined before we
+    // have their username, using just their ToxPk, then shortly after emit another peerNameChanged signal. This can
+    // cause double-updated to UI and chatlog, but is unavoidable given the API of toxcore.
     const Core* core = Core::getInstance();
-
     QStringList peers = core->getGroupPeerNames(toxGroupNum);
-    const auto oldPeers = peerDisplayNames.keys();
+    const auto oldPeerNames = peerDisplayNames;
     peerDisplayNames.clear();
     const int nPeers = peers.size();
     for (int i = 0; i < nPeers; ++i) {
         const auto pk = core->getGroupPeerPk(toxGroupNum, i);
-
-        Friend* f = FriendList::findFriend(pk);
-        if (f != nullptr && f->hasAlias()) {
-            peerDisplayNames[pk] = f->getDisplayedName();
-            empty_nick[pk] = false;
-            continue;
-        }
-
-        empty_nick[pk] = peers[i].isEmpty();
         peerDisplayNames[pk] = FriendList::decideNickname(pk, peers[i]);
     }
-    if (avGroupchat) {
-        stopAudioOfDepartedPeers(oldPeers, peerDisplayNames);
+    for (const auto& pk: oldPeerNames.keys()) {
+        if (!peerDisplayNames.contains(pk)) {
+            emit userLeft(pk, oldPeerNames.value(pk));
+            stopAudioOfDepartedPeers(pk);
+        }
+    }
+    for (const auto& pk: peerDisplayNames.keys()) {
+        if (!oldPeerNames.contains(pk)) {
+            emit userJoined(pk, peerDisplayNames.value(pk));
+        }
+    }
+    for (const auto& pk: peerDisplayNames.keys()) {
+        if (oldPeerNames.contains(pk) && oldPeerNames.value(pk) != peerDisplayNames.value(pk)) {
+            emit peerNameChanged(pk, oldPeerNames.value(pk), peerDisplayNames.value(pk));
+        }
+    }
+    if (oldPeerNames.size() != nPeers) {
+        emit numPeersChanged(nPeers);
     }
-    emit userListChanged(groupId, peerDisplayNames);
-}
-
-bool Group::peerHasNickname(ToxPk pk)
-{
-    return !empty_nick[pk];
 }
 
 void Group::updateUsername(ToxPk pk, const QString newName)
 {
-    peerDisplayNames[pk] = newName;
-    emit userListChanged(groupId, peerDisplayNames);
+    const QString displayName = FriendList::decideNickname(pk, newName);
+    assert(peerDisplayNames.contains(pk));
+    if (peerDisplayNames[pk] != displayName) {
+        // there could be no actual change even if their username changed due to an alias being set
+        const auto oldName = peerDisplayNames[pk];
+        peerDisplayNames[pk] = displayName;
+        emit peerNameChanged(pk, oldName, displayName);
+    }
 }
 
 bool Group::isAvGroupchat() const
@@ -186,12 +198,10 @@ QString Group::getSelfName() const
     return selfName;
 }
 
-void Group::stopAudioOfDepartedPeers(const QList<ToxPk>& oldPks, const QMap<ToxPk, QString>& newPks)
+void Group::stopAudioOfDepartedPeers(const ToxPk& peerPk)
 {
-    Core* core = Core::getInstance();
-    for(const auto& pk: oldPks) {
-        if(!newPks.contains(pk)) {
-            core->getAv()->invalidateGroupCallPeerSource(toxGroupNum, pk);
-        }
+    if (avGroupchat) {
+        Core* core = Core::getInstance();
+        core->getAv()->invalidateGroupCallPeerSource(toxGroupNum, peerPk);
     }
 }

src/model/group.h

@@ -59,18 +59,20 @@ public:
     QString getSelfName() const;
 
 signals:
-    void titleChangedByUser(const GroupId& groupId, const QString& title);
-    void titleChanged(const GroupId& groupId, const QString& author, const QString& title);
-    void userListChanged(const GroupId& groupId, const QMap<ToxPk, QString>& toxpks);
+    void titleChangedByUser(const QString& title);
+    void titleChanged(const QString& author, const QString& title);
+    void userJoined(const ToxPk& user, const QString& name);
+    void userLeft(const ToxPk& user, const QString& name);
+    void numPeersChanged(int numPeers);
+    void peerNameChanged(const ToxPk& peer, const QString& oldName, const QString& newName);
 
 private:
-    void stopAudioOfDepartedPeers(const QList<ToxPk>& oldPks, const QMap<ToxPk, QString>& newPks);
+    void stopAudioOfDepartedPeers(const ToxPk& peerPk);
 
 private:
     QString selfName;
     QString title;
     QMap<ToxPk, QString> peerDisplayNames;
-    QMap<ToxPk, bool> empty_nick;
     bool hasNewMessages;
     bool userWasMentioned;
     int toxGroupNum;

src/widget/form/groupchatform.cpp

@@ -126,11 +126,14 @@ GroupChatForm::GroupChatForm(Group* chatGroup)
     connect(headWidget, &ChatFormHeader::micMuteToggle, this, &GroupChatForm::onMicMuteToggle);
     connect(headWidget, &ChatFormHeader::volMuteToggle, this, &GroupChatForm::onVolMuteToggle);
     connect(headWidget, &ChatFormHeader::nameChanged, chatGroup, &Group::setName);
-    connect(group, &Group::userListChanged, this, &GroupChatForm::onUserListChanged);
     connect(group, &Group::titleChanged, this, &GroupChatForm::onTitleChanged);
+    connect(group, &Group::userJoined, this, &GroupChatForm::onUserJoined);
+    connect(group, &Group::userLeft, this, &GroupChatForm::onUserLeft);
+    connect(group, &Group::peerNameChanged, this, &GroupChatForm::onPeerNameChanged);
+    connect(group, &Group::numPeersChanged, this, &GroupChatForm::updateUserCount);
     connect(&Settings::getInstance(), &Settings::blackListChanged, this, &GroupChatForm::updateUserNames);
 
-    onUserListChanged();
+    updateUserNames();
     setAcceptDrops(true);
     Translator::registerHandler(std::bind(&GroupChatForm::retranslateUi, this), this);
 }
@@ -165,32 +168,8 @@ void GroupChatForm::onSendTriggered()
     }
 }
 
-/**
- * @brief This slot is intended to connect to Group::userListChanged signal.
- * Brief list of actions made by slot:
- *      1) sets text of how many people are in the group;
- *      2) creates lexicographically sorted comma-separated list of user names, each name in its own
- *      label;
- *      3) sets call button style depending on peer count and etc.
- */
-void GroupChatForm::onUserListChanged()
+void GroupChatForm::onTitleChanged(const QString& author, const QString& title)
 {
-    updateUserCount();
-    updateUserNames();
-    sendJoinLeaveMessages();
-
-    // Enable or disable call button
-    const int peersCount = group->getPeersCount();
-    const bool online = peersCount > 1;
-    headWidget->updateCallButtons(online, inCall);
-    if (inCall && (!online || !group->isAvGroupchat())) {
-        leaveGroupCall();
-    }
-}
-
-void GroupChatForm::onTitleChanged(const GroupId& groupId, const QString& author, const QString& title)
-{
-    Q_UNUSED(groupId);
     if (author.isEmpty()) {
         return;
     }
@@ -306,61 +285,22 @@ void GroupChatForm::updateUserNames()
     }
 }
 
-void GroupChatForm::sendJoinLeaveMessages()
+void GroupChatForm::onUserJoined(const ToxPk& user, const QString& name)
 {
-    const auto peers = group->getPeerList();
+    addSystemInfoMessage(tr("%1 has joined the group").arg(name), ChatMessage::INFO, QDateTime::currentDateTime());
+    updateUserNames();
+}
 
-    // no need to do anything without any peers
-    if (peers.isEmpty()) {
-        return;
-    }
+void GroupChatForm::onUserLeft(const ToxPk& user, const QString& name)
+{
+    addSystemInfoMessage(tr("%1 has left the group").arg(name), ChatMessage::INFO, QDateTime::currentDateTime());
+    updateUserNames();
+}
 
-    // generate user list from the current group if it's empty
-    if (groupLast.isEmpty()) {
-        groupLast = group->getPeerList();
-        return;
-    }
-
-    // user joins
-    for (const auto& peerPk : peers.keys()) {
-        const QString name = FriendList::decideNickname(peerPk, peers.value(peerPk));
-        if (!firstTime.value(peerPk, false)) {
-            if (!groupLast.contains(peerPk)) {
-                if (group->peerHasNickname(peerPk)) {
-                    firstTime[peerPk] = true;
-                    groupLast.insert(peerPk, name);
-                    addSystemInfoMessage(tr("%1 is online").arg(name), ChatMessage::INFO, QDateTime::currentDateTime());
-                    continue;
-                }
-                addSystemInfoMessage(tr("A new user has connected to the group"), ChatMessage::INFO, QDateTime::currentDateTime());
-            }
-            firstTime[peerPk] = true;
-            continue;
-        }
-        if (!groupLast.contains(peerPk)) {
-            groupLast.insert(peerPk, name);
-            addSystemInfoMessage(tr("%1 has joined the group").arg(name), ChatMessage::INFO, QDateTime::currentDateTime());
-        } else {
-            Friend *f = FriendList::findFriend(peerPk);
-            if (groupLast[peerPk] != name
-                    && peers.value(peerPk) == name
-                    && peerPk != Core::getInstance()->getSelfPublicKey() // ignore myself
-                    && !(f != nullptr && f->hasAlias()) // ignore friends with aliases
-                    ) {
-                addSystemInfoMessage(tr("%1 is now known as %2").arg(groupLast[peerPk], name), ChatMessage::INFO, QDateTime::currentDateTime());
-                groupLast[peerPk] = name;
-            }
-        }
-    }
-    // user leaves
-    for (const auto& peerPk : groupLast.keys()) {
-        const QString name = FriendList::decideNickname(peerPk, groupLast.value(peerPk));
-        if (!peers.contains(peerPk)) {
-            groupLast.remove(peerPk);
-            firstTime.remove(peerPk);
-            addSystemInfoMessage(tr("%1 has left the group").arg(name), ChatMessage::INFO, QDateTime::currentDateTime());
-        }
-    }
+void GroupChatForm::onPeerNameChanged(const ToxPk& peer, const QString& oldName, const QString& newName)
+{
+    addSystemInfoMessage(tr("%1 is now known as %2").arg(oldName, newName), ChatMessage::INFO, QDateTime::currentDateTime());
+    updateUserNames();
 }
 
 void GroupChatForm::peerAudioPlaying(ToxPk peerPk)
@@ -510,15 +450,19 @@ void GroupChatForm::keyReleaseEvent(QKeyEvent* ev)
 /**
  * @brief Updates users' count label text
  */
-void GroupChatForm::updateUserCount()
+void GroupChatForm::updateUserCount(int numPeers)
 {
-    const int peersCount = group->getPeersCount();
-    nusersLabel->setText(tr("%n user(s) in chat", "Number of users in chat", peersCount));
+    nusersLabel->setText(tr("%n user(s) in chat", "Number of users in chat", numPeers));
+    const bool online = numPeers > 1;
+    headWidget->updateCallButtons(online, inCall);
+    if (inCall && (!online || !group->isAvGroupchat())) {
+        leaveGroupCall();
+    }
 }
 
 void GroupChatForm::retranslateUi()
 {
-    updateUserCount();
+    updateUserCount(group->getPeersCount());
 }
 
 void GroupChatForm::onLabelContextMenuRequested(const QPoint& localPos)

src/widget/form/groupchatform.h

@@ -49,8 +49,10 @@ private slots:
     void onMicMuteToggle();
     void onVolMuteToggle();
     void onCallClicked();
-    void onUserListChanged();
-    void onTitleChanged(const GroupId& groupId, const QString& author, const QString& title);
+    void onUserJoined(const ToxPk& user, const QString& name);
+    void onUserLeft(const ToxPk& user, const QString& name);
+    void onPeerNameChanged(const ToxPk& peer, const QString& oldName, const QString& newName);
+    void onTitleChanged(const QString& author, const QString& title);
     void searchInBegin(const QString& phrase, const ParameterSearch& parameter) override;
     void onSearchUp(const QString& phrase, const ParameterSearch& parameter) override;
     void onSearchDown(const QString& phrase, const ParameterSearch& parameter) override;
@@ -66,7 +68,7 @@ protected:
 
 private:
     void retranslateUi();
-    void updateUserCount();
+    void updateUserCount(int numPeers);
     void updateUserNames();
     void sendJoinLeaveMessages();
     void leaveGroupCall();
@@ -75,8 +77,6 @@ private:
     Group* group;
     QMap<ToxPk, QLabel*> peerLabels;
     QMap<ToxPk, QTimer*> peerAudioTimers;
-    QMap<ToxPk, QString> groupLast;
-    QMap<ToxPk, bool> firstTime;
     FlowLayout* namesListLayout;
     QLabel* nusersLabel;
     TabCompleter* tabber;

src/widget/friendlistwidget.cpp

@@ -283,8 +283,7 @@ void FriendListWidget::addGroupWidget(GroupWidget* widget)
     groupLayout.addSortedWidget(widget);
     Group* g = widget->getGroup();
     connect(g, &Group::titleChanged,
-            [=](const GroupId& groupId, const QString& author, const QString& name) {
-        Q_UNUSED(groupId);
+            [=](const QString& author, const QString& name) {
         Q_UNUSED(author);
         renameGroupWidget(widget, name);
     });

src/widget/groupwidget.cpp

@@ -55,11 +55,11 @@ GroupWidget::GroupWidget(std::shared_ptr<GroupChatroom> chatroom, bool compact)
     Group* g = chatroom->getGroup();
     nameLabel->setText(g->getName());
 
-    updateUserCount();
+    updateUserCount(g->getPeersCount());
     setAcceptDrops(true);
 
     connect(g, &Group::titleChanged, this, &GroupWidget::updateTitle);
-    connect(g, &Group::userListChanged, this, &GroupWidget::updateUserCount);
+    connect(g, &Group::numPeersChanged, this, &GroupWidget::updateUserCount);
     connect(nameLabel, &CroppingLabel::editFinished, g, &Group::setName);
     Translator::registerHandler(std::bind(&GroupWidget::retranslateUi, this), this);
 }
@@ -69,9 +69,8 @@ GroupWidget::~GroupWidget()
     Translator::unregister(this);
 }
 
-void GroupWidget::updateTitle(const GroupId& groupId, const QString& author, const QString& newName)
+void GroupWidget::updateTitle(const QString& author, const QString& newName)
 {
-    Q_UNUSED(groupId);
     Q_UNUSED(author);
     nameLabel->setText(newName);
 }
@@ -159,10 +158,9 @@ void GroupWidget::mouseMoveEvent(QMouseEvent* ev)
     }
 }
 
-void GroupWidget::updateUserCount()
+void GroupWidget::updateUserCount(int numPeers)
 {
-    int peersCount = chatroom->getGroup()->getPeersCount();
-    statusMessageLabel->setText(tr("%n user(s) in chat", "Number of users in chat", peersCount));
+    statusMessageLabel->setText(tr("%n user(s) in chat", "Number of users in chat", numPeers));
 }
 
 void GroupWidget::setAsActiveChatroom()
@@ -262,5 +260,6 @@ void GroupWidget::setName(const QString& name)
 
 void GroupWidget::retranslateUi()
 {
-    updateUserCount();
+    const Group* group = chatroom->getGroup();
+    updateUserCount(group->getPeersCount());
 }

src/widget/groupwidget.h

@@ -57,8 +57,8 @@ protected:
 
 private slots:
     void retranslateUi();
-    void updateTitle(const GroupId& groupId, const QString& author, const QString& newName);
-    void updateUserCount();
+    void updateTitle(const QString& author, const QString& newName);
+    void updateUserCount(int numPeers);
 
 public:
     GroupId groupId;

src/widget/widget.cpp

@@ -1810,9 +1810,10 @@ void Widget::onGroupTitleChanged(uint32_t groupnumber, const QString& author, co
     widget->searchName(ui->searchContactText->text(), filterGroups(filter));
 }
 
-void Widget::titleChangedByUser(const GroupId& groupId, const QString& title)
+void Widget::titleChangedByUser(const QString& title)
 {
-    const auto& group = GroupList::findGroup(groupId);
+    const auto* group = qobject_cast<Group*>(sender());
+    assert(group != nullptr);
     emit changeGroupTitle(group->getId(), title);
 }
 

src/widget/widget.h

@@ -176,7 +176,7 @@ public slots:
     void onGroupPeerlistChanged(uint32_t groupnumber);
     void onGroupPeerNameChanged(uint32_t groupnumber, const ToxPk& peerPk, const QString& newName);
     void onGroupTitleChanged(uint32_t groupnumber, const QString& author, const QString& title);
-    void titleChangedByUser(const GroupId& groupId, const QString& title);
+    void titleChangedByUser(const QString& title);
     void onGroupPeerAudioPlaying(int groupnumber, ToxPk peerPk);
     void onGroupSendFailed(uint32_t groupnumber);
     void onFriendTypingChanged(uint32_t friendnumber, bool isTyping);