Skip to content
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
117 changes: 116 additions & 1 deletion lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ sealed class Event {
case 'message': return MessageEvent.fromJson(json);
case 'update_message': return UpdateMessageEvent.fromJson(json);
case 'delete_message': return DeleteMessageEvent.fromJson(json);
case 'update_message_flags':
switch (json['op'] as String) {
case 'add': return UpdateMessageFlagsAddEvent.fromJson(json);
case 'remove': return UpdateMessageFlagsRemoveEvent.fromJson(json);
default: return UnexpectedEvent.fromJson(json);
}
case 'reaction': return ReactionEvent.fromJson(json);
case 'heartbeat': return HeartbeatEvent.fromJson(json);
// TODO add many more event types
Expand Down Expand Up @@ -439,13 +445,122 @@ class DeleteMessageEvent extends Event {
Map<String, dynamic> toJson() => _$DeleteMessageEventToJson(this);
}

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

/// A Zulip event of type `update_message_flags`.
///
/// For the corresponding API docs, see subclasses.
sealed class UpdateMessageFlagsEvent extends Event {
@override
@JsonKey(includeToJson: true)
String get type => 'update_message_flags';

String get op;

@JsonKey(unknownEnumValue: MessageFlag.unknown)
final MessageFlag flag;
final List<int> messages;

UpdateMessageFlagsEvent({
required super.id,
required this.flag,
required this.messages,
});
}

/// An [UpdateMessageFlagsEvent] with op `add`: https://zulip.com/api/get-events#update_message_flags-add
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageFlagsAddEvent extends UpdateMessageFlagsEvent {
@override
String get op => 'add';

final bool all;

UpdateMessageFlagsAddEvent({
required super.id,
required super.flag,
required super.messages,
required this.all,
});

factory UpdateMessageFlagsAddEvent.fromJson(Map<String, dynamic> json) =>
_$UpdateMessageFlagsAddEventFromJson(json);

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

/// An [UpdateMessageFlagsEvent] with op `remove`: https://zulip.com/api/get-events#update_message_flags-remove
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageFlagsRemoveEvent extends UpdateMessageFlagsEvent {
@override
String get op => 'remove';

// final bool all; // deprecated, ignore
final Map<int, UpdateMessageFlagsMessageDetail>? messageDetails;

UpdateMessageFlagsRemoveEvent({
required super.id,
required super.flag,
required super.messages,
required this.messageDetails,
});

factory UpdateMessageFlagsRemoveEvent.fromJson(Map<String, dynamic> json) {
final result = _$UpdateMessageFlagsRemoveEventFromJson(json);
// Crunchy-shell validation
if (
result.flag == MessageFlag.read
&& true // (we assume `event_types` has `message` and `update_message_flags`)
) {
result.messageDetails as Map<int, UpdateMessageFlagsMessageDetail>;
}
return result;
}

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

/// As in [UpdateMessageFlagsRemoveEvent.messageDetails].
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageFlagsMessageDetail {
final MessageType type;
final bool? mentioned;
final List<int>? userIds;
final int? streamId;
final String? topic;
Comment on lines +536 to +538
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 we'll probably want a crunchy shell on ensuring that when type is private we always have userIds, and when it's stream we always have streamId and topic.

That is, in zulip-flutter I think we've been pretty successful in making it so that malformed server data can't make us outright crash or misbehave, and instead just interrupts talking to the server (thanks mostly to the autogenerated fromJson implementations), so I'd like to try to hold the line on that as an invariant. And I suspect that doing so in the code that consumes this data, if it's just three independent nullables like this, will be annoying and make that code hard to read.

One way to get the crunchy shell would be to have a base class and two subclasses, akin to Message itself. But another solution might be more expedient and would also suffice: just have our fromJson check those conditions, and throw if they're not met, in much the same way as the autogenerated code does when an expected field is missing. For example our fromJson can take a few lines to do that on the Map, before then calling the autogenerated implementation as usual.

The downside of that approach is that the consuming code is slightly uglier, with ! operators scattered around, than it would be with subclasses. But I think that's a good trade because there's not that much consuming code for this type, and the subclasses mean significantly more code here at the definitions. (For Message the tradeoff goes heavily the other way, because that's such a fundamental type and so much of our code consumes it.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example our fromJson can take a few lines to do that on the Map, before then calling the autogenerated implementation as usual.

How about calling the autogenerated implementation first? Then we can do an exhaustive switch on type, which will be a MessageType enum value.

Copy link
Member

Choose a reason for hiding this comment

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

Sure.


UpdateMessageFlagsMessageDetail({
required this.type,
required this.mentioned,
required this.userIds,
required this.streamId,
required this.topic,
});

factory UpdateMessageFlagsMessageDetail.fromJson(Map<String, dynamic> json) {
final result = _$UpdateMessageFlagsMessageDetailFromJson(json);
// Crunchy-shell validation
switch (result.type) {
case MessageType.stream:
result.streamId as int;
result.topic as String;
case MessageType.private:
result.userIds as List<int>;
}
return result;
}

Map<String, dynamic> toJson() => _$UpdateMessageFlagsMessageDetailToJson(this);
}

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

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

29 changes: 29 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,35 @@ class MessageListView with ChangeNotifier, _MessageSequence {
notifyListeners();
}

void maybeUpdateMessageFlags(UpdateMessageFlagsEvent event) {
final isAdd = switch (event) {
UpdateMessageFlagsAddEvent() => true,
UpdateMessageFlagsRemoveEvent() => false,
};

bool didUpdateAny = false;
if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {
for (final message in messages) {
message.flags.add(event.flag);
didUpdateAny = true;
}
} else {
for (final messageId in event.messages) {
final index = _findMessageWithId(messageId);
if (index != -1) {
final message = messages[index];
isAdd ? message.flags.add(event.flag) : message.flags.remove(event.flag);
didUpdateAny = true;
}
}
}
if (!didUpdateAny) {
return;
}

notifyListeners();
}

void maybeUpdateMessageReactions(ReactionEvent event) {
final index = _findMessageWithId(event.messageId);
if (index == -1) {
Expand Down
5 changes: 5 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,11 @@ class PerAccountStore extends ChangeNotifier {
} else if (event is DeleteMessageEvent) {
assert(debugLog("server event: delete_message ${event.messageIds}"));
// TODO handle
} else if (event is UpdateMessageFlagsEvent) {
assert(debugLog("server event: update_message_flags/${event.op} ${event.flag.toJson()}"));
for (final view in _messageListViews) {
view.maybeUpdateMessageFlags(event);
}
} else if (event is ReactionEvent) {
assert(debugLog("server event: reaction/${event.op}"));
for (final view in _messageListViews) {
Expand Down
44 changes: 31 additions & 13 deletions test/api/model/events_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,6 @@ import 'events_checks.dart';
import 'model_checks.dart';

void main() {
test('message: move flags into message object', () {
final message = eg.streamMessage();
MessageEvent mkEvent(List<MessageFlag> flags) => Event.fromJson({
'type': 'message',
'id': 1,
'message': (deepToJson(message) as Map<String, dynamic>)..remove('flags'),
'flags': flags.map((f) => f.toJson()).toList(),
}) as MessageEvent;
check(mkEvent(message.flags)).message.jsonEquals(message);
check(mkEvent([])).message.flags.deepEquals([]);
check(mkEvent([MessageFlag.read])).message.flags.deepEquals([MessageFlag.read]);
});

test('user_settings: all known settings have event handling', () {
final dataClassFieldNames = UserSettings.debugKnownNames;
final enumNames = UserSettingName.values.map((n) => n.name);
Expand All @@ -42,4 +29,35 @@ void main() {
' on the pattern of the existing cases.'
).isEmpty();
});

test('message: move flags into message object', () {
final message = eg.streamMessage();
MessageEvent mkEvent(List<MessageFlag> flags) => Event.fromJson({
'type': 'message',
'id': 1,
'message': (deepToJson(message) as Map<String, dynamic>)..remove('flags'),
'flags': flags.map((f) => f.toJson()).toList(),
}) as MessageEvent;
check(mkEvent(message.flags)).message.jsonEquals(message);
check(mkEvent([])).message.flags.deepEquals([]);
check(mkEvent([MessageFlag.read])).message.flags.deepEquals([MessageFlag.read]);
});

test('update_message_flags/remove: require messageDetails in mark-as-unread', () {
Copy link
Member

Choose a reason for hiding this comment

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

nit: put the tests in the same order as the code they test

final baseJson = {
'id': 1,
'type': 'update_message_flags',
'op': 'remove',
'flag': 'starred',
'messages': [123],
'all': false,
};
check(() => UpdateMessageFlagsRemoveEvent.fromJson(baseJson)).returnsNormally();
check(() => UpdateMessageFlagsRemoveEvent.fromJson({
...baseJson, 'flag': 'read',
})).throws();
check(() => UpdateMessageFlagsRemoveEvent.fromJson({
...baseJson, 'message_details': {'123': {'type': 'private', 'mentioned': false, 'user_ids': [2]}},
})).returnsNormally();
Comment on lines +59 to +61
Copy link
Member

Choose a reason for hiding this comment

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

This test should set 'flag': 'read' too, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eep, thanks! 89fd1f7

});
}
4 changes: 2 additions & 2 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ StreamMessage streamMessage({
int? lastEditTimestamp,
List<Reaction>? reactions,
int? timestamp,
List<String>? flags,
List<MessageFlag>? flags,
}) {
final effectiveStream = stream ?? _stream();
// The use of JSON here is convenient in order to delegate parts of the data
Expand Down Expand Up @@ -191,7 +191,7 @@ DmMessage dmMessage({
String? contentMarkdown,
int? lastEditTimestamp,
int? timestamp,
List<String>? flags,
List<MessageFlag>? flags,
}) {
assert(!to.any((user) => user.userId == from.userId));
return DmMessage.fromJson({
Expand Down
Loading