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

Add unread message count in query channels response #247

Merged
merged 5 commits into from
May 8, 2020

Conversation

b-onc
Copy link
Contributor

@b-onc b-onc commented May 6, 2020

No description provided.

@b-onc b-onc added 🙏 Feature Request A new feature request sdk:client labels May 6, 2020
@b-onc b-onc requested review from buh and VojtaStavik May 6, 2020 10:18
@Stream-SDK-Bot
Copy link
Collaborator

1 Warning
⚠️ Please provide a summary in the Pull Request description

Generated by 🚫 Danger

Copy link
Contributor

@buh buh left a comment

Choose a reason for hiding this comment

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

You only added parsing the new messaged unread count. But please check the full channel unread count flow. You need to remove the unread count calculation by messageRead.

Sources/Client/Model/Event.swift Outdated Show resolved Hide resolved
Sources/Client/Model/Message/MessageRead.swift Outdated Show resolved Hide resolved
@b-onc
Copy link
Contributor Author

b-onc commented May 7, 2020

You only added parsing the new messaged unread count. But please check the full channel unread count flow. You need to remove the unread count calculation by messageRead.

@buh That's not totally possible. MessageRead doesn't include mentioned messages, and it's not sent for new & deleted messages. I think we should keep the current solution for now.

@b-onc b-onc requested a review from buh May 7, 2020 11:51
Copy link
Contributor

@buh buh left a comment

Choose a reason for hiding this comment

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

It shouldn't be sent for new and delete messages.
I think you need to check the unread count flow.
So, we have a number from the request, if we will calculate by messages probably the number will be less because of messages limits. We should use the maximum otherwise it will be useless.
Unread count calculation required a request with state and watch. Now it looks like a mess with calculation and response value. Probably we need to request the mentioned unread count from the backend and omit it on our side.

Sources/Client/Model/Message/MessageRead.swift Outdated Show resolved Hide resolved
Bahadir Oncel added 2 commits May 7, 2020 17:32
messagesUnreadCount -> unreadMessagesCount
channelsUnreadCount -> unreadChannelsCount

These are internal so no API changes
@b-onc b-onc requested review from buh and VojtaStavik May 8, 2020 11:38
guard let eventCid = event.cid, let watchingChannels = watchingChannelsAtomic[eventCid] else {
return
}

// Update watching channels for unread count and watcher count.
watchingChannels.forEach {
if let channel = $0.value {
if let channel = $0.value, channel.cid == eventCid {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no needed because watchingChannels contains channels with the same cid:

guard let eventCid = event.cid, let watchingChannels = watchingChannelsAtomic[eventCid] else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've added it since it was like that for notificationMessageNew part. Will remove it.

Sources/Client/Client/Client+Events.swift Show resolved Hide resolved
Comment on lines 87 to 103
if unreadMessageRead.unreadMessageCount > 0 {
// Calculate mentioned message unread count
// This is approximate since it'll be limited for the messages we've fetched
for message in messages.reversed() {
if message.user.isCurrent {
continue
}

if message.user != currentUser, message.mentionedUsers.contains(currentUser) {
unreadCount.mentionedMessages += 1
if message.created > unreadMessageRead.lastReadDate {
if message.user != currentUser, message.mentionedUsers.contains(currentUser) {
unreadCount.mentionedMessages += 1
}
} else {
break
}
} else {
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

                  // Calculate mentioned message unread count
                  // This is approximate since it'll be limited for the messages we've fetched
                  for message in messages.reversed() where !message.user.isCurrent {
                     if message.created > unreadMessageRead.lastReadDate {
                         if message.mentionedUsers.contains(currentUser) {
                             unreadCount.mentionedMessages += 1
                         }
                     } else {
                         break
                     }
                  }

@b-onc b-onc merged commit 364c183 into master May 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the add-unread-count-to-query-channels branch May 8, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 Feature Request A new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants