model: Start tracking a few `user_settings`, including `display_emoji_reaction_users` #261

chrisbobbe · 2023-08-09T21:07:49Z

As mentioned in the last two commits:

f662a01 api: Add user_settings/update events, with a test for exhaustiveness
815a347 api: Add UserSettings.twentyFourHourTime

I'm not sure about the system I've set up for type-checking the user_settings/update event at the edge, and verifying that all settings in the initial snapshot get updated with that event. I'd be curious to hear thoughts on that. 🙂

Related: #121

gnprice

Thanks! Comments below.

gnprice · 2023-08-10T00:03:36Z

lib/api/model/events.dart

+}
+
+@JsonSerializable(fieldRename: FieldRename.snake)
+class UserSettingsUpdateEventTwentyFourHourTime extends UserSettingsUpdateEvent {


Rather, this demonstrates how much boilerplate it would take, with the setup from the previous commit. As I mentioned there, I think it would be better if less boilerplate were needed, particularly in the new subclass of [UserSettingsUpdateEvent]. Hmm.

Hmm yeah.

One thing we could do to mitigate this is to roll up this and the displayEmojiReactionUsers case into one class like UserSettingsUpdateEventBool. Then similarly have one subclass for int settings, and another for string settings, when we later add some such settings. That way we'd have just three of these subclasses — possibly occasionally adding another when a setting has a more complex type — but not dozens.

… OK and writing that sentence caused me to wonder if it's really true that all settings are of those three types. The API doc on the event says so:

but it sounds surprising.

And indeed the register-queue doc names a couple of settings that don't fit in that:

but only those two. So it'd be 5 subclasses for the current full API. Still a big reduction.

Perhaps a further improvement: have just UserSettingsUpdateEvent, with no subclasses, but make it generic on the type of value.

That will make json_serializable unable to generate code for it. But I think that's fine — on this event type it's really doing very little for us. In fromJson we can say something like

final name = UserSettingName.fromRawString(json['property'] as String); switch (name) { case UserSettingName.twentyFourHourTime: case UserSettingName.displayEmojiReactionUsers: return UserSettingsUpdateEvent<bool>( id: json['id'] as int, name: name, value: json['value'] as bool, );

In that version I guess the types no longer express that the event won't say it's about setting X but in fact carry data that's not of the right type for X. Instead that fact will be an invariant that our fromJson is responsible for. I think that's fine, though — crucially it's still an invariant of our code, not an assumption about the server.

It's then just a matter of whether any added as bool or the like in code that consumes these settings is less or more annoying then the extra subclasses. Not sure where that balance ends up falling.

One advantage of the separate subclasses (cumbersome as they are) is that, with the base class UserSettingsUpdateEvent being sealed, we can ensure an exhaustive check in PerAccountStore.handleEvent:

} else if (event is UserSettingsUpdateEvent) { assert(debugLog("server event: user_settings/update")); switch (event) { case UserSettingsUpdateEventTwentyFourHourTime(): userSettings?.twentyFourHourTime = event.value; case UserSettingsUpdateEventDisplayEmojiReactionUsers(): userSettings?.displayEmojiReactionUsers = event.value; case UserSettingsUpdateEventUnknown(): // do nothing } notifyListeners();

which provides the analysis error mentioned in step 3 in the added model/events test:

test('user_settings: all known settings have event handling', () { final missingEnumNames = UserSettingName.missingNames(); 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, when you see a Dart analysis error about\n' ' not exhaustively matching on `UserSettingName?`,\n' ' where we represent user_settings/update events,\n' ' please handle it by adding a new case\n' ' on the pattern of the other cases you see there.\n' ' (3) Then, when you see another Dart analysis error about\n' ' not handling the new event type in [PerAccountStore],\n' ' please do so, on the pattern of the other cases there.' ).isEmpty(); });

We do have the UserSettingName enum, and we know it's possible to enforce an exhaustive check on one of those values. So maybe that's a good alternative way to enforce an exhaustive check in PerAccountStore, instead of with a sealed class. However, in a switch on a UserSettingName, ideally each case would know what type of setting value (bool, int, etc.) it's dealing with, automatically, without making PerAccountStore repeat that information and risk getting it wrong. Does that make sense? I don't yet see a good way to do that, though.

Yeah, the exhaustive check is definitely good to achieve. I guess a couple of possible ways to do that would be:

With per-value-type subclasses like UserSettingsUpdateEventBool, have also a separate enum per value type. Then once you're looking at a UserSettingsUpdateEventBool, you can switch exhaustively on its name field because it'll come from the enum of boolean settings.

Use per-value-type subclasses like UserSettingsUpdateEventBool, and keep a single enum for all the setting names; but to do an exhaustive switch, switch first on the name field of the base class UserSettingsUpdateEvent. Then the cases look something like:

case UserSettingName.twentyFourHourTime: userSettings?.twentyFourHourTime = (event as UserSettingsUpdateEventBool).value;

Like the previous option, but skip the subclasses and just make the base class generic on the type of value, because it's not clear at that point that the subclasses are doing much for us. Then the cases look like:

case UserSettingName.twentyFourHourTime: userSettings?.twentyFourHourTime = (event as UserSettingsUpdateEvent<bool>).value;

Like the previous option, but don't make it generic and instead just give value a type of Object?, because it's not clear at that point that we're making any static use of the generic type parameter. Then the cases look like:

case UserSettingName.twentyFourHourTime: userSettings?.twentyFourHourTime = event.value as bool;

I think probably that last option beats the second or third; I'm not sure either of them ultimately have any advantages over the last one. The first option is more incommensurable; but the per-value type enums sound like kind of a pain, so I'd lean toward the last option.

What the last three options have in common is that they give up on having a static check that handleEvent (or other consuming code) agrees with the API binding on what value type corresponds to which setting. But I think that's OK, because I think that's a relatively low-risk class of potential bug — once you're actively adding code in all the places you need to, it's relatively easy to be sure they agree on the type.

The higher-risk class of potential bug is the one that exhaustiveness prevents, because it's a lot easier to forget some update-needing spot entirely.

Sure, I'm happy to do that last option.

I think probably that last option beats the second or third; I'm not sure either of them ultimately have any advantages over the last one.

My first thought was that those other options do have an advantage over the last one. I thought the last one, unlike the others, retreats from a "crunchy shell" ideal by allowing these events to flow into PerAccountStore.handleEvent without first type-guarding some part of them that we care about (value).

But that's not necessary: even if the class has dynamic value, we can still have its fromJson do that runtime type-guarding. (With an exhaustive check!) Probably that's what you had in mind; what do you think?

Yeah, definitely agree we'll want the fromJson to check it's the right type, the same way as it usually would.

gnprice · 2023-08-10T00:07:15Z

lib/api/model/initial_snapshot.dart

+  static final _byRawString = Map.fromEntries(
+   _$UserSettingNameEnumMap // thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake`


Neat, seems helpful.

gnprice · 2023-08-10T00:08:09Z

lib/api/model/initial_snapshot.dart

+  /// For test code to check that all of those properties are accounted for.
+  @visibleForTesting
+  static List<String> missingNames() {


Because this is meant to be called only from test code, let's follow the Flutter convention of having its name start with "debug". That helps mark it as something one expects not to see invoked in non-debugging source code.

(The @visibleForTesting doesn't on its own express that because it's perfectly normal to invoke a visible-for-testing method from within the same file/library — that annotation is typically used for an internal implementation detail that would be private if not for testing.)

gnprice · 2023-08-10T00:15:14Z

lib/api/model/initial_snapshot.dart

+  // 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


Cool, sounds good. This does seem like a spot where properly supporting both old and new versions would be kind of a pain. (Would have been totally doable, if the timing were such that we had to — we'd probably use a readValue that would just be one more spot that needs another line or two of code per setting — but enough of a pain to not be worth it given that we don't have to.)

gnprice · 2023-08-10T00:27:46Z

lib/api/model/events.dart

+    _$UserSettingsUpdateEventTwentyFourHourTimeFromJson(json);
+
+  @override
+  Map<String, dynamic> toJson() => _$UserSettingsUpdateEventTwentyFourHourTimeToJson(this);


In order for this toJson to work properly, it should include property — otherwise the output is indistinguishable between different settings.

In this design, the way to do that would be with a property getter with @JsonKey(includeToJson: true), the same way as for type on most of the Event subclasses. With the one-subclass-per-type design, you'll want a field identifying the setting anyway.

We aren't currently really using these toJson methods, so if it's a pain to make them exactly round-trip the fromJson then we don't need to. But if they leave out key information like property, it'd be better to omit the toJson entirely.

gnprice · 2023-08-10T00:51:13Z

lib/model/store.dart

+      assert(debugLog("server event: user_settings/update"));
+      switch (event) {
+        case UserSettingsUpdateEventTwentyFourHourTime():
+          userSettings!.twentyFourHourTime = event.value;


This will crash if userSettings is null. Why is that nullable — ah, that's for pre-server-5, right?

It's one thing to ignore the settings, but it's another to crash. Instead let's just use ?., so that we drop the update (to data we already chose to ignore on register-queue).

This event (doc) and the initial snapshot's user_settings go together: neither exists before Server 5, but they both exist starting at Server 5. So when talking to a server that behaves naturally, I don't expect this to crash: we won't be handling the event unless we've already stored the initial snapshot's data.

I see, OK.

Still, ideally I'd like to avoid crashing even if a server behaves oddly. Writing ?. is as easy as !., so we might as well.

(If we wanted to be paranoid we could plan to log if we get one of these events and userSettings is null. But that seems like an unlikely server bug, and when we start requiring Server 5 it'll become impossible because userSettings will be non-nullable (so if the server did leave it out, we'd give an error on trying to connect to it).

gnprice · 2023-08-10T00:52:33Z

lib/model/store.dart

@@ -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;


Should get a TODO(server-5) — we definitely want to clean this up to non-nullable, and the analyzer won't automatically draw the connection when we remove the ? in the InitialSnapshot type.

gnprice · 2023-08-10T00:56:42Z

lib/api/model/initial_snapshot.dart

+  static final _byRawString = Map.fromEntries(
+   _$UserSettingNameEnumMap // thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake`
+     .entries.map((entry) => MapEntry(entry.value, entry.key)),


Can be simplified a bit with Map.map, which acts on entries.

gnprice · 2023-08-10T00:59:41Z

lib/api/model/initial_snapshot.dart

+    final enumNames = values.map((n) => n.name);
+    final dataClassFieldNames = _$UserSettingsFieldMap.keys; // thanks to `createFieldMap: true`
+    return dataClassFieldNames.where((key) => !enumNames.contains(key)).toList();


Probably cleanest for this logic to live in the test code. Then all this non-test code needs to do is to export _$UserSettingsFieldMap.keys; can do that with a static property named something like UserSettings.debugKnownNames.

gnprice · 2023-08-10T01:03:57Z

test/api/model/events_test.dart

+  test('user_settings: all known settings have event handling', () {
+    final missingEnumNames = UserSettingName.missingNames();


Cool, good thought to close the loop like this.

chrisbobbe · 2023-08-10T20:27:25Z

Thanks for the review! Revision pushed, and please see my reply at #261 (comment) , about how it would be beneficial to keep enforcing an exhaustive check on the settings in PerAccountStore.handleEvent, where each case is given the setting-value's type automatically, so PerAccountStore doesn't have to copy it out by hand. It might be OK if we don't find a nice way to do that (we don't have to block on it), but I wanted to mention it. 🙂

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 zulip#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.

chrisbobbe · 2023-08-11T01:21:02Z

Thanks for the review! Revision pushed, with your preferred approach as you mentioned in #261 (comment) .

gnprice

Thanks for the revision! This is looking good; small comments below.

I'm happy with how the demo add-another-setting commit at the end turns out; that's a very mild amount of boilerplate at this point.

gnprice · 2023-08-11T20:04:34Z

lib/api/model/events.dart

+  /// The type is `dynamic`, but in handling real events from the API,
+  /// it's safe to cast [value] to the type appropriate for [property]
+  /// (e.g., to say `value as bool` for a boolean setting).
+  /// That's because [UserSettingsUpdateEvent.fromJson] does that check itself,
+  /// and it would have failed if the check didn't pass.


Suggested change

/// The type is `dynamic`, but in handling real events from the API,

/// it's safe to cast [value] to the type appropriate for [property]

/// (e.g., to say `value as bool` for a boolean setting).

/// That's because [UserSettingsUpdateEvent.fromJson] does that check itself,

/// and it would have failed if the check didn't pass.

/// 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].

Bit tighter and perhaps more assertive, which I think is appropriate for describing an invariant.

gnprice · 2023-08-11T20:05:29Z

lib/api/model/events.dart

+      case UserSettingName.displayEmojiReactionUsers:
+        return value as bool;
+      case UserSettingName.twentyFourHourTime:
+        return value as bool;


nit:

Suggested change

case UserSettingName.displayEmojiReactionUsers:

return value as bool;

case UserSettingName.twentyFourHourTime:

return value as bool;

case UserSettingName.displayEmojiReactionUsers:

case UserSettingName.twentyFourHourTime:

return value as bool;

(doesn't make much difference now, but when we have dozens of these and the bulk of them are of three types, this way will I think be significantly easier to read)

different nit: also put the settings in the same order as they appear elsewhere

(same order within a type, I guess — so that is a downside of the grouped approach)

gnprice · 2023-08-11T20:08:23Z

lib/model/store.dart

+        case UserSettingName.twentyFourHourTime:
+          userSettings?.twentyFourHourTime =        event.value as bool;
+        case UserSettingName.displayEmojiReactionUsers:
+          userSettings?.displayEmojiReactionUsers = event.value as bool;


nit:

Suggested change

case UserSettingName.twentyFourHourTime:

userSettings?.twentyFourHourTime = event.value as bool;

case UserSettingName.displayEmojiReactionUsers:

userSettings?.displayEmojiReactionUsers = event.value as bool;

case UserSettingName.twentyFourHourTime:

userSettings?.twentyFourHourTime = event.value as bool;

case UserSettingName.displayEmojiReactionUsers:

userSettings?.displayEmojiReactionUsers = event.value as bool;

gnprice · 2023-08-11T20:15:28Z

lib/api/model/events.dart

+  /// That's because [UserSettingsUpdateEvent.fromJson] does that check itself,
+  /// and it would have failed if the check didn't pass.
+  @JsonKey(readValue: _readValue)
+  final dynamic value;


Does everything work equally well if we say Object? instead of dynamic? I think it should.

If so, that way is a bit cleaner because it prevents accidentally assuming it's a particular type and going ahead and making a method call that only makes sense for that particular type (rather than explicitly downcasting first).

gnprice · 2023-08-11T20:17:48Z

lib/api/model/initial_snapshot.dart

+    required this.twentyFourHourTime,
+    this.displayEmojiReactionUsers,


nit:

Suggested change

required this.twentyFourHourTime,

this.displayEmojiReactionUsers,

required this.twentyFourHourTime,

required this.displayEmojiReactionUsers,

I think we do want any callsite to be explicit about making this null, if it so chooses.

(Any callsite is going to be in a test, other than the generated one which always passes all arguments anyway.)

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 zulip#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.

chrisbobbe · 2023-08-11T20:45:44Z

I'm happy with how the demo add-another-setting commit at the end turns out; that's a very mild amount of boilerplate at this point.

Me too, this is very satisfying!

Thanks for the review; revision pushed. 🙂

This will make [displayEmojiReactionUsers] available for zulip#121, showing message reactions. (Though it won't be live-updated yet; we'll do that soon.) Related: zulip#121

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 zulip#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.

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.

gnprice · 2023-08-14T20:44:09Z

Thanks! Looks good; merging.

chrisbobbe added the a-model Implementing our data model (PerAccountStore, etc.) label Aug 9, 2023

chrisbobbe requested a review from gnprice August 9, 2023 21:07

chrisbobbe force-pushed the pr-user-settings branch from 815a347 to b9d5f74 Compare August 9, 2023 21:18

gnprice reviewed Aug 10, 2023

View reviewed changes

chrisbobbe force-pushed the pr-user-settings branch from b9d5f74 to e385983 Compare August 10, 2023 20:26

chrisbobbe force-pushed the pr-user-settings branch from e385983 to 3a6ee54 Compare August 11, 2023 01:20

gnprice reviewed Aug 11, 2023

View reviewed changes

chrisbobbe force-pushed the pr-user-settings branch from 3a6ee54 to aaca55b Compare August 11, 2023 20:45

chrisbobbe added 4 commits August 14, 2023 13:43

api: Add InitialSnapshot.userSettings, with one user setting to start

3dd73cb

This will make [displayEmojiReactionUsers] available for zulip#121, showing message reactions. (Though it won't be live-updated yet; we'll do that soon.) Related: zulip#121

model: Add PerAccountStore.userSettings

ee0504a

api: Add UserSettings.twentyFourHourTime

4bcbffd

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.

gnprice force-pushed the pr-user-settings branch from aaca55b to 4bcbffd Compare August 14, 2023 20:44

gnprice merged commit 4bcbffd into zulip:main Aug 14, 2023

chrisbobbe deleted the pr-user-settings branch August 14, 2023 21:30

gnprice mentioned this pull request Aug 14, 2023

Require Zulip Server 5 #268

Open

gnprice mentioned this pull request May 13, 2024

Track realm settings #668

Open

gnprice mentioned this pull request Jun 27, 2024

Handle updated streams (stream op: update events) #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Start tracking a few `user_settings`, including `display_emoji_reaction_users` #261

model: Start tracking a few `user_settings`, including `display_emoji_reaction_users` #261

chrisbobbe commented Aug 9, 2023

gnprice left a comment

gnprice Aug 10, 2023

gnprice Aug 10, 2023

chrisbobbe Aug 10, 2023

gnprice Aug 10, 2023

chrisbobbe Aug 10, 2023

gnprice Aug 11, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

chrisbobbe Aug 10, 2023 •

edited

Loading

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

gnprice Aug 10, 2023

chrisbobbe commented Aug 10, 2023

chrisbobbe commented Aug 11, 2023

gnprice left a comment

gnprice Aug 11, 2023

gnprice Aug 11, 2023

gnprice Aug 11, 2023

gnprice Aug 11, 2023

gnprice Aug 11, 2023

gnprice Aug 11, 2023

chrisbobbe commented Aug 11, 2023

gnprice commented Aug 14, 2023

		static final _byRawString = Map.fromEntries(
		_$UserSettingNameEnumMap // thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake`

		test('user_settings: all known settings have event handling', () {
		final missingEnumNames = UserSettingName.missingNames();

		required this.twentyFourHourTime,
		this.displayEmojiReactionUsers,

model: Start tracking a few user_settings, including display_emoji_reaction_users #261

model: Start tracking a few user_settings, including display_emoji_reaction_users #261

Conversation

chrisbobbe commented Aug 9, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 10, 2023

chrisbobbe commented Aug 11, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 11, 2023

gnprice commented Aug 14, 2023

model: Start tracking a few `user_settings`, including `display_emoji_reaction_users` #261

model: Start tracking a few `user_settings`, including `display_emoji_reaction_users` #261

chrisbobbe Aug 10, 2023 •

edited

Loading