-
Notifications
You must be signed in to change notification settings - Fork 177
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
store: Add realmEmoji
, initialized from initial snapshot and updated with its event
#394
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.
Thanks! Small comments below.
lib/api/model/events.dart
Outdated
@@ -16,6 +16,7 @@ sealed class Event { | |||
|
|||
factory Event.fromJson(Map<String, dynamic> json) { | |||
switch (json['type'] as String) { | |||
case 'realm_emoji': return RealmEmojiUpdateEvent.fromJson(json); |
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.
It's true that currently the only possible op for type realm_emoji
is update
. But the presence of that op
field is a way of explicitly leaving space for potentially adding other op
values for the same type in the future. So let's respect that future-proofing that the API spec has given itself: check for op
the same as we do for other types, and if it's not the op we expect then call it an unexpected event.
(Fine to skip the intermediate base class, though.)
lib/model/store.dart
Outdated
if (event is HeartbeatEvent) { | ||
if (event is RealmEmojiUpdateEvent) { |
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.
If you put this anywhere other than the top of the list, then it won't have to touch any extra lines :-)
I'm happy to have the heartbeat at the top, as it's a boring no-op that exists for the sake of the transport protocol. This could go right after, before AlertWordsEvent, to match the order in events.dart.
lib/model/store.dart
Outdated
realmEmoji..clear()..addAll(event.realmEmoji); | ||
} else if (event is HeartbeatEvent) { |
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.
Should call notifyListeners
, no?
lib/model/store.dart
Outdated
if (event is HeartbeatEvent) { | ||
if (event is RealmEmojiUpdateEvent) { | ||
assert(debugLog("server event: realm_emoji/update")); | ||
realmEmoji..clear()..addAll(event.realmEmoji); |
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.
Since we already have the new set of emoji in the form of a map, we might as well use that map rather than copy it into the existing one: realmEmoji = event.realmEmoji
.
bcce78a
to
01ebaf6
Compare
Thanks for the review! Revision pushed. |
Thanks! Looks good; merging. |
01ebaf6
to
8f67079
Compare
No description provided.