(Chat): Display system messages about adding/removing members immediately after an update is performed #14146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (18)
|
friofry
changed the title
friofry
force-pushed
the
ab/issue-9867-display-add-remove-messages-in-chat
branch
from
a1798f6
to
25e60cd
Compare
igor-sirotin
approved these changes
Mar 25, 2024
| var (chats, messages) = result | ||
| self.signalChatsAndMessagesUpdates(chats, messages) | ||
|
|
||
| proc processGroupChatUpdateAfterSend*(self: Service, response: RpcResponse[JsonNode]): (ChatDto, seq[MessageDto]) = |
There was a problem hiding this comment.
Suggested change
| proc processGroupChatUpdateAfterSend*(self: Service, response: RpcResponse[JsonNode]): (ChatDto, seq[MessageDto]) = | |
| proc processGroupChatUpdateResponse*(self: Service, response: RpcResponse[JsonNode]): (ChatDto, seq[MessageDto]) = |
There was a problem hiding this comment.
Also, do we actually need to return (ChatDto, seq[MessageDto]), if we're discarding it in both usages?
| let chat = chatMap[msg.chatId] | ||
| self.events.emit(SIGNAL_SENDING_SUCCESS, MessageSendingSuccess(message: msg, chat: chat)) | ||
|
|
||
| proc processMessageUpdateAfterSend*(self: Service, response: RpcResponse[JsonNode]): (seq[ChatDto], seq[MessageDto]) = |
There was a problem hiding this comment.
I know it's not your code, but this AfterSend is quite weird.
The name of the function should describe what it does, not when it will be called. 😄
Suggested change
| proc processMessageUpdateAfterSend*(self: Service, response: RpcResponse[JsonNode]): (seq[ChatDto], seq[MessageDto]) = | |
| proc processMessageUpdateResponse*(self: Service, response: RpcResponse[JsonNode]): (seq[ChatDto], seq[MessageDto]) = |
There was a problem hiding this comment.
Yes, that makes sense!
Since protocol/messenger.go uses the MessengerResponse type, it might be better to use the same name in proc. Something like proc processMessengerResponse*(self: Service, response: RpcResponse[JsonNode]): .
friofry
force-pushed
the
ab/issue-9867-display-add-remove-messages-in-chat
branch
2 times, most recently
from
01938c3
to
faf09f8
Compare
mprakhov
reviewed
Mar 25, 2024
| proc parseGroupChatResponse*(self: Service, response: RpcResponse[JsonNode]): (ChatDto, seq[MessageDto]) = | ||
| var chat: ChatDto | ||
| var messages: seq[MessageDto] = @[] | ||
| if response.result{"chat"} != nil: |
There was a problem hiding this comment.
the result can be an error. Could you please log that we got an error?
mprakhov
reviewed
Mar 25, 2024
| proc processMessageUpdateAfterSend*(self: Service, response: RpcResponse[JsonNode]): (seq[ChatDto], seq[MessageDto]) = | ||
| result = self.parseChatResponse(response) | ||
| var (chats, messages) = result | ||
| proc parseGroupChatResponse*(self: Service, response: RpcResponse[JsonNode]): (ChatDto, seq[MessageDto]) = |
There was a problem hiding this comment.
Do we really need this function? I mean, the code can exist in processGroupChatUpdateAfterSend
There was a problem hiding this comment.
There are 6 endpoints in api_group_chats.go that return GroupChatResponse. So I tried to follow the existing approach with parseChatReponse.
I'm not sure if this function will be useful. Do you think it should be removed?
There was a problem hiding this comment.
If it is used only in one place - it make sense to put this code there
mprakhov
approved these changes
Mar 26, 2024
friofry
force-pushed
the
ab/issue-9867-display-add-remove-messages-in-chat
branch
from
faf09f8
to
aa916c2
Compare
jrainville
approved these changes
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #9876, #10702
What does the PR do
chats/service.nim
addGroupMembers/removeMemberFromGroupChatnow analyzes and processes theGroupChatResponseprocessMessageUpdateAfterSend, the procedureprocessGroupChatUpdateAfterSendhas been added. Now they both use thesignalChatsAndMessagesUpdatesproc, which emits signals to notify UI of new chats/messagesparseGroupChatResponsemethod to parse GroupChatResponse, since this structure has 'chat' and ChatResponse has 'chats'Screenshot of functionality (including design for comparison)
system_messages.mp4