Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Talk-Widget direct mentions #11929

Conversation

Purva92Gupta
Copy link

☑️ Resolves

  • These changes will display the actual message in the Talk-widget for direct mentions instead of showing "You were mentioned". This makes it more informative and inline with the actual message lines.
  • For mention all (Everyone in the group), nothing will be displayed under Talk-widget Mentions.

- Get last unread mention message using last unread mention id
- Only direct mentions are displayed in Talk-Widget
lib/Service/ParticipantService.php Outdated Show resolved Hide resolved
lib/Service/RoomFormatter.php Outdated Show resolved Hide resolved
Comment on lines 1960 to 1961
->where('c.id = :mentionId')
->setParameter('mentionId', $mentionId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this works in the doctrine query builder, we never use this pattern. You will always find code like the following when looking around:

Suggested change
->where('c.id = :mentionId')
->setParameter('mentionId', $mentionId)
->where($query->expr()->eq('c.id', $query->createNamedParameter($mentionId)))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These statements are removed with the removal of the function getLastUnreadMentionMessage() in ParticipantService.php. Also noticed the common way of building queries in this project.

} elseif ($participant->getAttendee()->getLastMentionMessage() > $participant->getAttendee()->getLastReadMessage()) {
$subtitle = $this->l10n->t('You were mentioned');
} elseif ($participant->getAttendee()->getLastMentionDirect() > $participant->getAttendee()->getLastReadMessage()) {
$subtitle = $this->chatManager->getComment($room, (string)$participant->getAttendee()->getLastMentionDirect())->getMessage();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message could be expired or deleted, there is no check for it's existence before using getMessage()

Also the message is not "parsed" so it would show Hi @userid instead of Hi Display Name. See the above block if ($lastMessage instanceof IComment) { for how to do that (and basically the code from here could be moved up and combined, so we don't parse $lastMessage when we don't use it anyway and instead parse the lastMention comment (when it exists)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the checks for expired/deleted messages. And, also added the parse function to display the correct mention name instead of @mentionid.
I think similar checks/changes will be required in RoomFormatter.php for lastUnreadDirectMention, I will look at it tomorrow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar checks and parsing have also been implemented for RoomFormatter to check deleted or expired messages. Also, the actual name of the users are displayed instead of @userid.

@nickvergessen
Copy link
Member

There are still too many unneeded/undesired changes here while the actual code could still be improved, so I'm closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants