-
Notifications
You must be signed in to change notification settings - Fork 314
Track user status #1629
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
base: main
Are you sure you want to change the base?
Track user status #1629
Conversation
Thanks!! CI is failing; could you take a look? |
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, this is exciting! Comments below on the first 4 commits:
5f8cf0e33 api: Add InitialSnapshot.userStatus
1202dbd14 api [nfc]: Add ReactionType.fromApiValue
99b96b76a api: Add user_status event
f333f8935 store: Add UserStore.getUserStatus, with event updates
This is an area where we're reasonably happy with the zulip-mobile implementation, so I recommend reading it. It handles the nuance I mention below, and it discusses the concept of a "zero value" (or perhaps links to discussion; I don't remember 🙂). Could you take a look and see what might be helpful there? For example I think the model code has a name like "user statuses" (plural); it seems like an API wart that the initial snapshot uses the singular user_status
for a map about many users.
lib/api/model/model.dart
Outdated
@@ -158,6 +158,31 @@ class RealmEmojiItem { | |||
Map<String, dynamic> toJson() => _$RealmEmojiItemToJson(this); | |||
} | |||
|
|||
/// An value in [InitialSnapshot.userStatus]. |
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.
nit: "A value"
lib/api/model/events.dart
Outdated
static String? _stringFromJson(Object? json) { | ||
final value = json as String; | ||
return value.isNotEmpty ? value : null; | ||
} | ||
|
||
static ReactionType? _reactionTypeFromJson(Object? json) { | ||
final value = json as String; | ||
return value.isNotEmpty ? ReactionType.fromApiValue(value) : null; | ||
} |
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.
There's a nuance that's not really clear in the API doc, but that we figured out and implemented in zulip-mobile:
The meaning of user_status
is really an update to a user's status; it's not a complete representation of what their status is.
In particular, the event's status_text
, emoji_name
, emoji_code
, reaction_type
may be null, and null means that that part of the status didn't change. So for example, if your current status is a house emoji and the text "Working remotely", and we handle an event with status_text: null
and non-null emoji fields, then your status should then be "Working remotely" with the different (not-house) emoji. The fields could be the empty string, which is a "zero value": if the event has status_text: ""
, then that means the status text was cleared (set to "zero"), which is a kind of change.
It's tricky to see this behavior, because I think the current web app always includes all the fields when it makes update-status requests. But some clients do send partial updates (maybe zulip-mobile?), and we should avoid misinterpreting e.g. status_text: null
as clearing out the status text, when in fact it means the status didn't change.
So concretely, for this code, we should allow the fields to be null (so no json as String
), and preserve the distinction between ""
and null
(so no conversion of ""
to null).
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.
Relatedly, we found that user_status
in the initial snapshot isn't a full snapshot of everyone's status either. Each entry represents a change from the baseline of the "zero value" status—
{
away: false, // (which we're ignoring; it's deprecated)
status_text: '',
emoji_name: '',
reaction_type: '',
emoji_code: ''
}
—which explains why a user with that "zero value" status doesn't appear in the initial snapshot.
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.
a2a9b53
to
ae0b51f
Compare
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 for the revision! Looks like some existing tests are failing, and build_runner
at one of the first few commits; do you know why that's happening?
@@ -61,6 +61,7 @@ sealed class Event { | |||
default: return UnexpectedEvent.fromJson(json); | |||
} | |||
// case 'muted_topics': … // TODO(#422) we ignore this feature on older servers | |||
case 'user_status': return UserStatusEvent.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.
api: Add user_status event
tools/check build_runner
is failing at this commit; I think that can be fixed with tools/check build_runner --fix
.
The screenshots look great to me! |
Status emojis are only shown for self-1:1 and 1:1 conversation items. They're ignored for group conversations as that's what the Web does.
ae0b51f
to
d51cc98
Compare
Thank you @chrisbobbe for the previous review. It's good to know about the intricate detail in the API you mentioned. A new revision is pushed with tests included too, PTAL. The CI errors are also solved; thanks for mentioning the suggested fix. |
Support for tracking the user status.
Message List:
Recent DMs Page:
New DM Sheet
User Autocomplete:
Profile Page:
Fixes: #197