Skip to content

model: Add data models for typing events. #783

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

Merged
merged 4 commits into from
Jul 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 91 additions & 5 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ sealed class Event {
case 'remove': return UpdateMessageFlagsRemoveEvent.fromJson(json);
default: return UnexpectedEvent.fromJson(json);
}
case 'typing': return TypingEvent.fromJson(json);
case 'reaction': return ReactionEvent.fromJson(json);
case 'heartbeat': return HeartbeatEvent.fromJson(json);
// TODO add many more event types
Expand Down Expand Up @@ -707,6 +708,9 @@ class DeleteMessageEvent extends Event {

final List<int> messageIds;
// final int messageId; // Not present; we support the bulk_message_deletion capability
// The server never actually sends "direct" here yet (it's "private" instead),
// but we accept both forms for forward-compatibility.
@MessageTypeConverter()
final MessageType messageType;
Comment on lines +713 to 714
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@MessageTypeConverter()
final MessageType messageType;
// The server never actually sends "direct" here yet (it's "private" instead),
// but we accept both forms for forward-compatibility.
@MessageTypeConverter()
final MessageType messageType;

It's a bit of a mismatch with the existing API, so it deserves a short comment acknowledging that.

Similarly on UpdateMessageFlagsMessageDetail.

final int? streamId;
final String? topic;
Expand All @@ -733,12 +737,28 @@ class DeleteMessageEvent extends Event {
Map<String, dynamic> toJson() => _$DeleteMessageEventToJson(this);
}

/// As in [DeleteMessageEvent.messageType]
/// or [UpdateMessageFlagsMessageDetail.type].
@JsonEnum(fieldRename: FieldRename.snake)
/// As in [DeleteMessageEvent.messageType],
/// [UpdateMessageFlagsMessageDetail.type],
/// or [TypingEvent.messageType].
@JsonEnum(alwaysCreate: true)
enum MessageType {
stream,
private;
direct;
}

class MessageTypeConverter extends JsonConverter<MessageType, String> {
const MessageTypeConverter();

@override
MessageType fromJson(String json) {
if (json == 'private') json = 'direct'; // TODO(server-future)
return $enumDecode(_$MessageTypeEnumMap, json);
}

@override
String toJson(MessageType object) {
return _$MessageTypeEnumMap[object]!;
}
}

/// A Zulip event of type `update_message_flags`.
Expand Down Expand Up @@ -820,6 +840,9 @@ class UpdateMessageFlagsRemoveEvent extends UpdateMessageFlagsEvent {
/// As in [UpdateMessageFlagsRemoveEvent.messageDetails].
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageFlagsMessageDetail {
// The server never actually sends "direct" here yet (it's "private" instead),
// but we accept both forms for forward-compatibility.
@MessageTypeConverter()
final MessageType type;
final bool? mentioned;
final List<int>? userIds;
Expand All @@ -841,7 +864,7 @@ class UpdateMessageFlagsMessageDetail {
case MessageType.stream:
result.streamId as int;
result.topic as String;
case MessageType.private:
case MessageType.direct:
result.userIds as List<int>;
}
return result;
Expand All @@ -850,6 +873,69 @@ class UpdateMessageFlagsMessageDetail {
Map<String, dynamic> toJson() => _$UpdateMessageFlagsMessageDetailToJson(this);
}

/// A Zulip event of type `typing`:
/// https://zulip.com/api/get-events#typing-start
/// https://zulip.com/api/get-events#typing-stop
@JsonSerializable(fieldRename: FieldRename.snake)
class TypingEvent extends Event {
Copy link
Member

Choose a reason for hiding this comment

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

nit in commit message:

api: Handle typing events.

This commit defines the events, but doesn't yet handle them — that's the next commit, where handleEvent starts acting on these events.

(The separation of commits seems good, though, as there's plenty of complexity in that next commit.)

@override
@JsonKey(includeToJson: true)
String get type => 'typing';

final TypingOp op;
@MessageTypeConverter()
final MessageType messageType;
Copy link
Member

Choose a reason for hiding this comment

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

This is "stream" or "private", but in the API these events say "direct" on current servers. So we should handle that.

The enum can still have two values, but we'll just want some additional code around that so that it accepts both names for MessageType.private (or MessageType.direct as we'll call it soon).

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to add that additional logic in a way that applies to all the places we use this enum, even if some of them don't yet ever say 'direct' in the server API.

All of them should eventually say 'direct', ideally.

And, the important part for making it OK to add that logic in a way that applies to all of them: for anything in the Zulip API that would be reasonable to think of as a "message type", if it does ever start sending a value of 'direct', then the one thing that could possibly mean is the same thing that 'private' means today.

OTOH if the most convenient way to add the logic winds up being separate for each place the enum is used, then this PR might as well stick to this one spot where it's actually needed, and we can handle the others later.

Copy link
Member Author

Choose a reason for hiding this comment

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

OTOH if the most convenient way to add the logic winds up being separate for each place the enum is used, then this PR might as well stick to this one spot where it's actually needed, and we can handle the others later.

I put the change that affected other events in a prep commit: 9410969
This might not be the cleanest way to do it, but all we need is annotating the field with the converter.

@JsonKey(readValue: _readSenderId)
final int senderId;
@JsonKey(name: 'recipients', fromJson: _recipientIdsFromJson)
final List<int>? recipientIds;
final int? streamId;
final String? topic;

TypingEvent({
required super.id,
required this.op,
required this.messageType,
required this.senderId,
required this.recipientIds,
required this.streamId,
required this.topic,
});

static Object? _readSenderId(Map<Object?, Object?> json, String key) {
return (json['sender'] as Map<String, dynamic>)['user_id'];
}

static List<int>? _recipientIdsFromJson(Object? json) {
if (json == null) return null;
return (json as List<Object?>).map(
(item) => (item as Map<String, Object?>)['user_id'] as int).toList();
}

factory TypingEvent.fromJson(Map<String, dynamic> json) {
final result = _$TypingEventFromJson(json);
// Crunchy-shell validation
switch (result.messageType) {
case MessageType.stream:
result.streamId as int;
result.topic as String;
case MessageType.direct:
result.recipientIds as List<int>;
}
return result;
}

@override
Map<String, dynamic> toJson() => _$TypingEventToJson(this);
}

/// As in [TypingEvent.op].
@JsonEnum(fieldRename: FieldRename.snake)
enum TypingOp {
start,
stop
}

/// A Zulip event of type `reaction`, with op `add` or `remove`.
///
/// See:
Expand Down
47 changes: 38 additions & 9 deletions lib/api/model/events.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ class InitialSnapshot {

final List<CustomProfileField> customProfileFields;

// TODO(server-8): Remove the default values.
@JsonKey(defaultValue: 15000)
final int serverTypingStartedExpiryPeriodMilliseconds;
@JsonKey(defaultValue: 5000)
final int serverTypingStoppedWaitPeriodMilliseconds;
@JsonKey(defaultValue: 10000)
final int serverTypingStartedWaitPeriodMilliseconds;

// final List<…> mutedTopics; // TODO(#422) we ignore this feature on older servers

final Map<String, RealmEmojiItem> realmEmoji;
Expand Down Expand Up @@ -86,6 +94,9 @@ class InitialSnapshot {
required this.zulipMergeBase,
required this.alertWords,
required this.customProfileFields,
required this.serverTypingStartedExpiryPeriodMilliseconds,
required this.serverTypingStoppedWaitPeriodMilliseconds,
required this.serverTypingStartedWaitPeriodMilliseconds,
required this.realmEmoji,
required this.recentPrivateConversations,
required this.subscriptions,
Expand Down
18 changes: 18 additions & 0 deletions lib/api/model/initial_snapshot.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/model/narrow.dart
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class DmNarrow extends Narrow implements SendableNarrow {
UpdateMessageFlagsMessageDetail detail, {
required int selfUserId,
}) {
assert(detail.type == MessageType.private);
assert(detail.type == MessageType.direct);
return DmNarrow.withOtherUsers(detail.userIds!, selfUserId: selfUserId);
}

Expand Down
12 changes: 12 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import 'message.dart';
import 'message_list.dart';
import 'recent_dm_conversations.dart';
import 'stream.dart';
import 'typing_status.dart';
import 'unreads.dart';

export 'package:drift/drift.dart' show Value;
Expand Down Expand Up @@ -242,6 +243,10 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
.followedBy(initialSnapshot.realmNonActiveUsers)
.followedBy(initialSnapshot.crossRealmBots)
.map((user) => MapEntry(user.userId, user))),
typingStatus: TypingStatus(
selfUserId: account.userId,
typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds),
),
streams: streams,
messages: MessageStoreImpl(),
unreads: Unreads(
Expand All @@ -266,6 +271,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
required this.selfUserId,
required this.userSettings,
required this.users,
required this.typingStatus,
required StreamStoreImpl streams,
required MessageStoreImpl messages,
required this.unreads,
Expand Down Expand Up @@ -319,6 +325,8 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {

final Map<int, User> users;

final TypingStatus typingStatus;

////////////////////////////////
// Streams, topics, and stuff about them.

Expand Down Expand Up @@ -383,6 +391,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
recentDmConversationsView.dispose();
unreads.dispose();
_messages.dispose();
typingStatus.dispose();
super.dispose();
}

Expand Down Expand Up @@ -485,6 +494,9 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
assert(debugLog("server event: update_message_flags/${event.op} ${event.flag.toJson()}"));
_messages.handleUpdateMessageFlagsEvent(event);
unreads.handleUpdateMessageFlagsEvent(event);
} else if (event is TypingEvent) {
assert(debugLog("server event: typing/${event.op} ${event.messageType}"));
typingStatus.handleTypingEvent(event);
} else if (event is ReactionEvent) {
assert(debugLog("server event: reaction/${event.op}"));
_messages.handleReactionEvent(event);
Expand Down
Loading