From 3dd73cb641812f8d3d6fa16fb47521e269aa3cca Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Aug 2023 12:18:36 -0700 Subject: [PATCH 1/4] api: Add InitialSnapshot.userSettings, with one user setting to start This will make [displayEmojiReactionUsers] available for #121, showing message reactions. (Though it won't be live-updated yet; we'll do that soon.) Related: #121 --- lib/api/model/initial_snapshot.dart | 29 +++++++++++++++++++++++++++ lib/api/model/initial_snapshot.g.dart | 14 +++++++++++++ test/example_data.dart | 2 ++ 3 files changed, 45 insertions(+) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 4cd2305647..e04298ab62 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -30,6 +30,14 @@ class InitialSnapshot { final List streams; + // Servers pre-5.0 don't have `user_settings`, and instead provide whatever + // user settings they support at toplevel in the initial snapshot. Since we're + // likely to desupport pre-5.0 servers before wide release, we prefer to + // ignore the toplevel fields and use `user_settings` where present instead, + // even at the expense of functionality with pre-5.0 servers. + // TODO(server-5) remove pre-5.0 comment + final UserSettings? userSettings; // TODO(server-5) + final int maxFileUploadSizeMib; @JsonKey(readValue: _readUsersIsActiveFallbackTrue) @@ -71,6 +79,7 @@ class InitialSnapshot { required this.recentPrivateConversations, required this.subscriptions, required this.streams, + required this.userSettings, required this.maxFileUploadSizeMib, required this.realmUsers, required this.realmNonActiveUsers, @@ -102,3 +111,23 @@ class RecentDmConversation { Map toJson() => _$RecentDmConversationToJson(this); } + +/// The `user_settings` dictionary. +/// +/// For docs, search for "user_settings:" +/// in . +@JsonSerializable(fieldRename: FieldRename.snake) +class UserSettings { + final bool? displayEmojiReactionUsers; // TODO(server-6) + + // TODO more, as needed + + UserSettings({ + required this.displayEmojiReactionUsers, + }); + + factory UserSettings.fromJson(Map json) => + _$UserSettingsFromJson(json); + + Map toJson() => _$UserSettingsToJson(this); +} diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 34832c30eb..5e3c091068 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -31,6 +31,10 @@ InitialSnapshot _$InitialSnapshotFromJson(Map json) => streams: (json['streams'] as List) .map((e) => ZulipStream.fromJson(e as Map)) .toList(), + userSettings: json['user_settings'] == null + ? null + : UserSettings.fromJson( + json['user_settings'] as Map), maxFileUploadSizeMib: json['max_file_upload_size_mib'] as int, realmUsers: (InitialSnapshot._readUsersIsActiveFallbackTrue(json, 'realm_users') @@ -59,6 +63,7 @@ Map _$InitialSnapshotToJson(InitialSnapshot instance) => 'recent_private_conversations': instance.recentPrivateConversations, 'subscriptions': instance.subscriptions, 'streams': instance.streams, + 'user_settings': instance.userSettings, 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, 'realm_users': instance.realmUsers, 'realm_non_active_users': instance.realmNonActiveUsers, @@ -79,3 +84,12 @@ Map _$RecentDmConversationToJson( 'max_message_id': instance.maxMessageId, 'user_ids': instance.userIds, }; + +UserSettings _$UserSettingsFromJson(Map json) => UserSettings( + displayEmojiReactionUsers: json['display_emoji_reaction_users'] as bool?, + ); + +Map _$UserSettingsToJson(UserSettings instance) => + { + 'display_emoji_reaction_users': instance.displayEmojiReactionUsers, + }; diff --git a/test/example_data.dart b/test/example_data.dart index 460c2981b6..740976a9e9 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -208,6 +208,7 @@ InitialSnapshot initialSnapshot({ List? recentPrivateConversations, List? subscriptions, List? streams, + UserSettings? userSettings, int? maxFileUploadSizeMib, List? realmUsers, List? realmNonActiveUsers, @@ -224,6 +225,7 @@ InitialSnapshot initialSnapshot({ recentPrivateConversations: recentPrivateConversations ?? [], subscriptions: subscriptions ?? [], // TODO add subscriptions to default streams: streams ?? [], // TODO add streams to default + userSettings: userSettings, // TODO add userSettings to default maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, realmUsers: realmUsers ?? [], realmNonActiveUsers: realmNonActiveUsers ?? [], From ee0504a01745210a7a7af20324c4f00b9d6dd07b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Aug 2023 13:02:44 -0700 Subject: [PATCH 2/4] model: Add PerAccountStore.userSettings --- lib/model/store.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index 9132d30c20..e05205174f 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -151,6 +151,7 @@ class PerAccountStore extends ChangeNotifier { required InitialSnapshot initialSnapshot, }) : zulipVersion = initialSnapshot.zulipVersion, maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib, + userSettings = initialSnapshot.userSettings, users = Map.fromEntries( initialSnapshot.realmUsers .followedBy(initialSnapshot.realmNonActiveUsers) @@ -172,6 +173,9 @@ class PerAccountStore extends ChangeNotifier { final String zulipVersion; // TODO get from account; update there on initial snapshot final int maxFileUploadSizeMib; // No event for this. + // Data attached to the self-account on the realm. + final UserSettings? userSettings; // TODO(#135) update with user_settings/update event + // Users and data about them. final Map users; From 9cb2e4d0f6300693186c9231a570e03050bd6d18 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Aug 2023 13:35:47 -0700 Subject: [PATCH 3/4] api: Add user_settings/update events, with a test for exhaustiveness It's been tricky to find a way to verify that the event-handling code keeps up with the settings we add in [UserSettings], the data class we use in the initial snapshot. See #261 for some alternatives we considered. But at least this solution works, with type-checking of the event at the edge, and a mechanism to ensure that all user settings we store in our initial snapshot get updated by the user_settings/update event. --- lib/api/model/events.dart | 53 +++++++++++++++++++++++++++ lib/api/model/events.g.dart | 23 ++++++++++++ lib/api/model/initial_snapshot.dart | 35 ++++++++++++++++-- lib/api/model/initial_snapshot.g.dart | 8 ++++ lib/model/store.dart | 13 ++++++- test/api/model/events_test.dart | 21 +++++++++++ 6 files changed, 149 insertions(+), 4 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index f09f0b24b1..a961f67166 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -1,5 +1,6 @@ import 'package:json_annotation/json_annotation.dart'; +import 'initial_snapshot.dart'; import 'model.dart'; part 'events.g.dart'; @@ -16,6 +17,11 @@ sealed class Event { factory Event.fromJson(Map json) { switch (json['type'] as String) { case 'alert_words': return AlertWordsEvent.fromJson(json); + case 'user_settings': + switch (json['op'] as String) { + case 'update': return UserSettingsUpdateEvent.fromJson(json); + default: return UnexpectedEvent.fromJson(json); + } case 'realm_user': switch (json['op'] as String) { case 'add': return RealmUserAddEvent.fromJson(json); @@ -74,6 +80,53 @@ class AlertWordsEvent extends Event { Map toJson() => _$AlertWordsEventToJson(this); } +/// A Zulip event of type `user_settings` with op `update`. +@JsonSerializable(fieldRename: FieldRename.snake) +class UserSettingsUpdateEvent extends Event { + @override + @JsonKey(includeToJson: true) + String get type => 'user_settings'; + + @JsonKey(includeToJson: true) + String get op => 'update'; + + /// The name of the setting, or null if we don't recognize it. + @JsonKey(unknownEnumValue: JsonKey.nullForUndefinedEnumValue) + final UserSettingName? property; + + /// The new value, or null if we don't recognize the setting. + /// + /// This will have the type appropriate for [property]; for example, + /// if the setting is boolean, then `value is bool` will always be true. + /// This invariant is enforced by [UserSettingsUpdateEvent.fromJson]. + @JsonKey(readValue: _readValue) + final Object? value; + + /// [value], with a check that its type corresponds to [property] + /// (e.g., `value as bool`). + static Object? _readValue(Map json, String key) { + final value = json['value']; + switch (UserSettingName.fromRawString(json['property'] as String)) { + case UserSettingName.displayEmojiReactionUsers: + return value as bool; + case null: + return null; + } + } + + UserSettingsUpdateEvent({ + required super.id, + required this.property, + required this.value, + }); + + factory UserSettingsUpdateEvent.fromJson(Map json) => + _$UserSettingsUpdateEventFromJson(json); + + @override + Map toJson() => _$UserSettingsUpdateEventToJson(this); +} + /// A Zulip event of type `realm_user`. /// /// The corresponding API docs are in several places for diff --git a/lib/api/model/events.g.dart b/lib/api/model/events.g.dart index 5183ec3145..83dee577f1 100644 --- a/lib/api/model/events.g.dart +++ b/lib/api/model/events.g.dart @@ -23,6 +23,29 @@ Map _$AlertWordsEventToJson(AlertWordsEvent instance) => 'alert_words': instance.alertWords, }; +UserSettingsUpdateEvent _$UserSettingsUpdateEventFromJson( + Map json) => + UserSettingsUpdateEvent( + id: json['id'] as int, + property: $enumDecodeNullable(_$UserSettingNameEnumMap, json['property'], + unknownValue: JsonKey.nullForUndefinedEnumValue), + value: UserSettingsUpdateEvent._readValue(json, 'value'), + ); + +Map _$UserSettingsUpdateEventToJson( + UserSettingsUpdateEvent instance) => + { + 'id': instance.id, + 'type': instance.type, + 'op': instance.op, + 'property': _$UserSettingNameEnumMap[instance.property], + 'value': instance.value, + }; + +const _$UserSettingNameEnumMap = { + UserSettingName.displayEmojiReactionUsers: 'display_emoji_reaction_users', +}; + RealmUserAddEvent _$RealmUserAddEventFromJson(Map json) => RealmUserAddEvent( id: json['id'] as int, diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index e04298ab62..0a3acf15e4 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -1,3 +1,4 @@ +import 'package:flutter/foundation.dart'; import 'package:json_annotation/json_annotation.dart'; import 'model.dart'; @@ -116,11 +117,14 @@ class RecentDmConversation { /// /// For docs, search for "user_settings:" /// in . -@JsonSerializable(fieldRename: FieldRename.snake) +@JsonSerializable(fieldRename: FieldRename.snake, createFieldMap: true) class UserSettings { - final bool? displayEmojiReactionUsers; // TODO(server-6) + bool? displayEmojiReactionUsers; // TODO(server-6) - // TODO more, as needed + // TODO more, as needed. When adding a setting here, please also: + // (1) add it to the [UserSettingName] enum below + // (2) then re-run the command to refresh the .g.dart files + // (3) handle the event that signals an update to the setting UserSettings({ required this.displayEmojiReactionUsers, @@ -130,4 +134,29 @@ class UserSettings { _$UserSettingsFromJson(json); Map toJson() => _$UserSettingsToJson(this); + + /// A list of [UserSettings]'s properties, as strings. + // _$…FieldMap is thanks to `createFieldMap: true` + @visibleForTesting + static final Iterable debugKnownNames = _$UserSettingsFieldMap.keys; +} + +/// The name of a user setting that has a property in [UserSettings]. +/// +/// In Zulip event-handling code (for [UserSettingsUpdateEvent]), +/// we switch exhaustively on a value of this type +/// to ensure that every setting in [UserSettings] responds to the event. +@JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true) +enum UserSettingName { + displayEmojiReactionUsers; + + /// Get a [UserSettingName] from a raw, snake-case string we recognize, else null. + /// + /// Example: + /// 'display_emoji_reaction_users' -> UserSettingName.displayEmojiReactionUsers + static UserSettingName? fromRawString(String raw) => _byRawString[raw]; + + // _$…EnumMap is thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake` + static final _byRawString = _$UserSettingNameEnumMap + .map((key, value) => MapEntry(value, key)); } diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 5e3c091068..dbe2af8bea 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -89,7 +89,15 @@ UserSettings _$UserSettingsFromJson(Map json) => UserSettings( displayEmojiReactionUsers: json['display_emoji_reaction_users'] as bool?, ); +const _$UserSettingsFieldMap = { + 'displayEmojiReactionUsers': 'display_emoji_reaction_users', +}; + Map _$UserSettingsToJson(UserSettings instance) => { 'display_emoji_reaction_users': instance.displayEmojiReactionUsers, }; + +const _$UserSettingNameEnumMap = { + UserSettingName.displayEmojiReactionUsers: 'display_emoji_reaction_users', +}; diff --git a/lib/model/store.dart b/lib/model/store.dart index e05205174f..07a746ca4d 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -174,7 +174,7 @@ class PerAccountStore extends ChangeNotifier { final int maxFileUploadSizeMib; // No event for this. // Data attached to the self-account on the realm. - final UserSettings? userSettings; // TODO(#135) update with user_settings/update event + final UserSettings? userSettings; // TODO(server-5) // Users and data about them. final Map users; @@ -219,6 +219,17 @@ class PerAccountStore extends ChangeNotifier { } else if (event is AlertWordsEvent) { assert(debugLog("server event: alert_words")); // We don't yet store this data, so there's nothing to update. + } else if (event is UserSettingsUpdateEvent) { + assert(debugLog("server event: user_settings/update ${event.property?.name ?? '[unrecognized]'}")); + if (event.property == null) { + // unrecognized setting; do nothing + return; + } + switch (event.property!) { + case UserSettingName.displayEmojiReactionUsers: + userSettings?.displayEmojiReactionUsers = event.value as bool; + } + notifyListeners(); } else if (event is RealmUserAddEvent) { assert(debugLog("server event: realm_user/add")); users[event.person.userId] = event.person; diff --git a/test/api/model/events_test.dart b/test/api/model/events_test.dart index b05771fd1e..e86cc1c4c4 100644 --- a/test/api/model/events_test.dart +++ b/test/api/model/events_test.dart @@ -1,6 +1,7 @@ import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/initial_snapshot.dart'; import '../../example_data.dart' as eg; import '../../stdlib_checks.dart'; @@ -20,4 +21,24 @@ void main() { check(mkEvent([])).message.flags.deepEquals([]); check(mkEvent(['read'])).message.flags.deepEquals(['read']); }); + + test('user_settings: all known settings have event handling', () { + final dataClassFieldNames = UserSettings.debugKnownNames; + final enumNames = UserSettingName.values.map((n) => n.name); + final missingEnumNames = dataClassFieldNames.where((key) => !enumNames.contains(key)).toList(); + check( + missingEnumNames, + because: + 'You have added these fields to [UserSettings]\n' + 'without handling the corresponding forms of the\n' + 'user_settings/update event in [PerAccountStore]:\n' + ' $missingEnumNames\n' + 'To do that, please follow these steps:\n' + ' (1) Add corresponding members to the [UserSettingName] enum.\n' + ' (2) Then, re-run the command to refresh the .g.dart files.\n' + ' (3) Resolve the Dart analysis errors about not exhaustively\n' + ' matching on that enum, by adding new `switch` cases\n' + ' on the pattern of the existing cases.' + ).isEmpty(); + }); } From 4bcbffd529663737c6e3480dfd477bfcd70aa7c8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 9 Aug 2023 13:54:36 -0700 Subject: [PATCH 4/4] api: Add UserSettings.twentyFourHourTime Not because we have an immediate need to read this user setting. Rather, this demonstrates how much boilerplate it would take to handle a new setting, with the setup from the previous commit. --- lib/api/model/events.dart | 1 + lib/api/model/events.g.dart | 1 + lib/api/model/initial_snapshot.dart | 3 +++ lib/api/model/initial_snapshot.g.dart | 4 ++++ lib/model/store.dart | 2 ++ 5 files changed, 11 insertions(+) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index a961f67166..4a1f19abb7 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -107,6 +107,7 @@ class UserSettingsUpdateEvent extends Event { static Object? _readValue(Map json, String key) { final value = json['value']; switch (UserSettingName.fromRawString(json['property'] as String)) { + case UserSettingName.twentyFourHourTime: case UserSettingName.displayEmojiReactionUsers: return value as bool; case null: diff --git a/lib/api/model/events.g.dart b/lib/api/model/events.g.dart index 83dee577f1..0a43c905d9 100644 --- a/lib/api/model/events.g.dart +++ b/lib/api/model/events.g.dart @@ -43,6 +43,7 @@ Map _$UserSettingsUpdateEventToJson( }; const _$UserSettingNameEnumMap = { + UserSettingName.twentyFourHourTime: 'twenty_four_hour_time', UserSettingName.displayEmojiReactionUsers: 'display_emoji_reaction_users', }; diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 0a3acf15e4..7ab896aee1 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -119,6 +119,7 @@ class RecentDmConversation { /// in . @JsonSerializable(fieldRename: FieldRename.snake, createFieldMap: true) class UserSettings { + bool twentyFourHourTime; bool? displayEmojiReactionUsers; // TODO(server-6) // TODO more, as needed. When adding a setting here, please also: @@ -127,6 +128,7 @@ class UserSettings { // (3) handle the event that signals an update to the setting UserSettings({ + required this.twentyFourHourTime, required this.displayEmojiReactionUsers, }); @@ -148,6 +150,7 @@ class UserSettings { /// to ensure that every setting in [UserSettings] responds to the event. @JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true) enum UserSettingName { + twentyFourHourTime, displayEmojiReactionUsers; /// Get a [UserSettingName] from a raw, snake-case string we recognize, else null. diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index dbe2af8bea..c0e89375ba 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -86,18 +86,22 @@ Map _$RecentDmConversationToJson( }; UserSettings _$UserSettingsFromJson(Map json) => UserSettings( + twentyFourHourTime: json['twenty_four_hour_time'] as bool, displayEmojiReactionUsers: json['display_emoji_reaction_users'] as bool?, ); const _$UserSettingsFieldMap = { + 'twentyFourHourTime': 'twenty_four_hour_time', 'displayEmojiReactionUsers': 'display_emoji_reaction_users', }; Map _$UserSettingsToJson(UserSettings instance) => { + 'twenty_four_hour_time': instance.twentyFourHourTime, 'display_emoji_reaction_users': instance.displayEmojiReactionUsers, }; const _$UserSettingNameEnumMap = { + UserSettingName.twentyFourHourTime: 'twenty_four_hour_time', UserSettingName.displayEmojiReactionUsers: 'display_emoji_reaction_users', }; diff --git a/lib/model/store.dart b/lib/model/store.dart index 07a746ca4d..7dc870df2d 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -226,6 +226,8 @@ class PerAccountStore extends ChangeNotifier { return; } switch (event.property!) { + case UserSettingName.twentyFourHourTime: + userSettings?.twentyFourHourTime = event.value as bool; case UserSettingName.displayEmojiReactionUsers: userSettings?.displayEmojiReactionUsers = event.value as bool; }