Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

Add join and leave messages #4923

Closed

Conversation

tox-user
Copy link
Contributor

@tox-user tox-user commented Jan 20, 2018

Adds messages that notify when user joins or leaves group chat or changes their name. Implements #4128.

Current issues:

  • when user joins a non empty group chat they will always get a join message for every participant at once
  • every time someone joins a group that we are in, we don't see their user name at first. Instead we see "Tox User" as their name. A few seconds later the name changes to their real name, which creates additional message

I am not sure how to fix them yet.

Depends on TokTok/c-toxcore#819


This change is Reviewable

@iphydf
Copy link
Contributor

iphydf commented Jan 20, 2018

Hi tox-user, we're working on a change that slightly changes the group chat api. Can we discuss somewhere?

@tox-user
Copy link
Contributor Author

Hi @iphydf could you add me on Tox? You will find my ID in my profile.

@tox-user tox-user force-pushed the feat/group-join-leave-messages branch 2 times, most recently from d1c3c5f to 0a28ac1 Compare February 27, 2018 03:07
Implements qTox#4128
Adds messages that notify when user joins or leaves group chat or changes their name.
@tox-user tox-user force-pushed the feat/group-join-leave-messages branch from 0a28ac1 to ee9c68e Compare February 27, 2018 03:11
@tox-user tox-user changed the title Add join and leave messages WIP Add join and leave messages Feb 27, 2018
@tox-user
Copy link
Contributor Author

This is ready for testing and review now.

@Diadlo
Copy link
Member

Diadlo commented Feb 28, 2018

  1. It doesn't works
  2. If you move all code in if (oldPeersList.length() > 0) (with if itself) after peer filling but before emit it will work
  3. And after fix(group): Show correct count of user on first creation #4978 merge (because otherwise you haven't any messages on first peer come)
  4. And since we show friend alias if we have peer as friend, probably you shouldn't emit peerNameChanged if friend found (probably, we should show both and correct handle it, but feel free to leave it now)

@tox-user
Copy link
Contributor Author

tox-user commented Feb 28, 2018

It's true that it doesn't work in this scenario:

  1. Client #1 creates a group and invites client #2.
  2. Client #2 joins.
  3. Client #1 will not receive the join notification.

PR #4978 itself doesn't fix it. It makes it more strange, because then client #2 gets duplicate join notifications.

@Diadlo
Copy link
Member

Diadlo commented Mar 1, 2018

@tox-user Did you move if (oldPeersList.length() > 0)?

@tox-user
Copy link
Contributor Author

tox-user commented Mar 2, 2018

That helped, but I get a notification spam when I join a bigger existing group.

@Talkless
Copy link
Contributor

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, some commit checks failed.


src/model/group.cpp, line 53 at r1 (raw file):

    // which is later changed to their real name. Because of that we treat everyone
    // who changed the name from empty to non empty as a newly joined user
    if (name != peers[peerId]) {

Same peers[peerId] lookup is performed two-three times, could it be avoided?


src/model/group.cpp, line 109 at r1 (raw file):

    if (oldPeersList.length() > 0) {
        // peers that have joined
        for (int i = 0; i < peers.length(); i++) {

is int i index really needed here? As far as I can see, ranged-based for loop could work?


src/model/group.cpp, line 118 at r1 (raw file):

        // peers that have left
        for (int i = 0; i < oldPeersList.length(); i++) {

range-based for loop instead?


src/widget/widget.cpp, line 1841 at r1 (raw file):

    const QString message = tr("%1 has joined the chat");

    Group* g = GroupList::findGroup(groupnumber);

Group* const g? Or maybe even const Group* const g (if getId() is const)?


src/widget/widget.cpp, line 1855 at r1 (raw file):

    const QString message = tr("%1 has left the chat");

    Group* g = GroupList::findGroup(groupnumber);

same constsness.


src/widget/widget.cpp, line 1869 at r1 (raw file):

    const QString message = tr("%1 is now known as %2");

    Group* g = GroupList::findGroup(groupnumber);

same constness


Comments from Reviewable

@tox-user
Copy link
Contributor Author

@Talkless thanks for reviewing, but unfortunately this PR doesn't work anymore, because of #4978. I don't know how to get it working again, so I've decided to leave it and move on to other things. Maybe I will take a look at it again some time in the future or someone else will pick it up.

@tox-user tox-user closed this Aug 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants