-
Notifications
You must be signed in to change notification settings - Fork 15
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
refactor: process conversation typing event - WPB-10178 #2194
base: develop
Are you sure you want to change the base?
Conversation
import WireDataModel | ||
|
||
/// Keeping track of typing users timeouts | ||
final class ConversationTypingUsersTimeout: NSObject, ZMTimerClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check this page here for specifics regarding the business logic related to typing users timeouts.
uiContext.typingUsers?.update( | ||
typingUsers: Set(users), | ||
in: conversation | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying implementation (typingUsers?.update..
) was not accessible from WireDomain (it was part of WireSyncEngine) so in this PR, TypingUsers
object and the related NSManagedObjectContext extension were moved from WireSyncEngine to WireDataModel.
At the app level, the ConversationInputBarViewController
relies on this code. Now that it is accessible in WireDataModel, we can reuse the same methods from WireDomain so when we plug our new quick sync mechanism, UI won't break (although it should probably be refactored as well eventually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely needs refactoring... we shouldn't store anything on the managed object context. Do you think it's something we can do in this PR or it's too big?
public struct TypingUsersInfo { | ||
let users: Set<NSManagedObjectID> | ||
let conversationID: NSManagedObjectID | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to move this to a separate file and add doc comments.
conversationLocalStore.obtainPermanentIDs( | ||
user: user, | ||
conversation: conversation | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that can be internal to the stores?
uiContext.typingUsers?.update( | ||
typingUsers: Set(users), | ||
in: conversation | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely needs refactoring... we shouldn't store anything on the managed object context. Do you think it's something we can do in this PR or it's too big?
Key points
This PR is part of the quick sync refactoring plan and is related to processing the multiple events we receive from the backend or the push channel.
Specifically, this PR is about porting the existing implementation of the
ConversationTyping
event.The legacy code for processing that event was fully part of WireSyncEngine, some objects (
TypingUsers
) were moved from WireSyncEngine to WireDataModel for backward compatibility so when we plug our new quick sync mechanism UI don't break. (see comment for specifics)Testing
ConversationTypingUsersTimeout
read/write methods (add, remove, contains..)ConversationTypingEventProcessor
repo and local store methods invocations.ConversationRepository
a notification with the typing users data is correctly sent.Checklist
[WPB-XXX]
.