Store, report, and display presence! #1619

chrisbobbe · 2025-06-23T23:12:58Z

TODO tests, but here's a draft for presence!

Screenshots coming soon.

Fixes: #196
Fixes: #1607

chrisbobbe · 2025-06-23T23:22:33Z

On the profile page:

TODO (followup): The "offline" status should be represented with an open circle, like on web, not a solid circle, like here. That's in order to show meaning by shape, not just color, for color-blind users.

Light	Dark

chrisbobbe · 2025-06-23T23:24:25Z

Direct-messages screen. See that "Chris Bobbe (Test Account)" is online, "Alya Abbott" is idle, and everyone else is offline.

Light	Dark

chrisbobbe · 2025-06-23T23:32:49Z

Message list:

Light	Dark

gnprice · 2025-06-23T23:34:02Z

lib/model/presence.dart

+      case AppLifecycleState.resumed:
+        // > […] the default running mode for a running application that has
+        // > input focus and is visible.
+        result = await updatePresence(connection,
+          pingOnly: pingOnly,
+          status: PresenceStatus.active,
+          newUserInput: true);
+      case AppLifecycleState.inactive:
+        // > At least one view of the application is visible, but none have
+        // > input focus. The application is otherwise running normally.
+        // For example, we expect this state when the user is selecting a file
+        // to upload.
+        result = await updatePresence(connection,
+          pingOnly: pingOnly,
+          status: PresenceStatus.active,
+          newUserInput: false);


- The newUserInput param is now usually true instead of always false. This seems more correct to me, and the change seems low-stakes (the doc says it's used to implement usage statistics); see the doc: https://zulip.com/api/update-presence#parameter-new_user_input

This sounds reasonable to me, but probably good to check with Tim in case he has thoughts based on how that's actually used on the server — perhaps @-mention him in the #mobile-team thread.

#mobile-team > presence @ 💬

Quoting Tim:

Probably true is a better value if one wants to do something quick. Most accurately, you should be passing true if any only if the user has interacted with the app at all since the last presence request.

I think on a mobile device, always true is a fairly good simulation of that. But I'd definitely an M6 issue for refining it to implement that detail on the spec fully and then move on.

gnprice

Thanks for the swift implementation! Generally all looks great. Comments below.

gnprice · 2025-06-23T23:37:52Z

lib/model/presence.dart

+  }) async {
+    if (realmPresenceDisabled) return;
+
+    late UpdatePresenceResult result;


Suggested change

late UpdatePresenceResult result;

final UpdatePresenceResult result;

right? (I.e., the analyzer should be able to verify this is always initialized before it gets used.)

gnprice · 2025-06-23T23:45:06Z

lib/model/store.dart

-      ),
+        typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds)),
+      presence: Presence(core: core,
+        serverPresencePingIntervalSeconds: Duration(seconds: initialSnapshot.serverPresencePingIntervalSeconds),


nit: once it's a Duration rather than a plain number, the units no longer belong in the name:

Suggested change

serverPresencePingIntervalSeconds: Duration(seconds: initialSnapshot.serverPresencePingIntervalSeconds),

serverPresencePingInterval: Duration(seconds: initialSnapshot.serverPresencePingIntervalSeconds),

(cf typingStartedExpiryPeriod / serverTypingStartedExpiryPeriodMilliseconds above)

gnprice · 2025-06-23T23:46:03Z

lib/model/presence.dart

+    while (true) {
+      // We put the wait upfront because we already have data when [start] is
+      // called; it comes from /register.
+      await Future<void>.delayed(serverPresencePingIntervalSeconds);


This is actually the spot that prompted the comment just above about this field's name — this line looks wrong, as it's taking what looks like a number of seconds but then using it in a way that doesn't appear to be about seconds.

gnprice · 2025-06-23T23:48:45Z

lib/widgets/content.dart

@@ -1662,17 +1664,23 @@ class Avatar extends StatelessWidget {
    required this.userId,
    required this.size,
    required this.borderRadius,
+    this.backgroundColor,
+    this.omitPresenceStatus = false,


nit: the negative feels more confusing to read; and I think "status" is redundant here

Suggested change

this.omitPresenceStatus = false,

this.showPresence = true,

gnprice · 2025-06-23T23:49:57Z

lib/widgets/content.dart

+    this.backgroundColor,
+    this.omitPresenceStatus = false,
  });


The backgroundColor is only meaningful if presence will be shown, right? Probably worth an assert here to avoid passing it when it wouldn't do anything.

(and/or on AvatarShape)

gnprice · 2025-06-23T23:53:26Z

lib/widgets/content.dart

+    Widget result = child;
+
+    if (userIdForPresence != null) {
+      final presenceCircleSize = size / 4; // TODO(design) is this right?


Yeah, good question. Probably good to raise in #mobile-design, as a follow-up.

Given we don't show them on the one place where an avatar is a drastically different size from elsewhere (namely the top of the profile page), this way seems fine to ship.

I think one assumption this layout makes is that borderRadius is no bigger than presenceCircleSize. If bigger, it looks like it would cut off part of the circle.

Could make that assumption explicit with an assert… or could also move the ClipRRect to apply only to the child, not this whole Stack. Then the presence circle in that case would stick out a little past the avatar's rounded corner, which I think would be fine.

gnprice · 2025-06-23T23:58:11Z

lib/widgets/content.dart

+
+/// The green or orange-gradient circle representing [PresenceStatus].
+///
+/// [backgroundColor] is required and must not be [Colors.transparent].


This bit appears to not match the code — that parameter is optional.

gnprice · 2025-06-23T23:59:59Z

lib/widgets/content.dart

+          userId: userId,
+          size: size,
+          backgroundColor: backgroundColor,
+          explicitOffline: true)));


This flag seems orthogonal to being used as a WidgetSpan. Probably just make it a parameter of this method, and have the one caller specify it explicitly.

… hmm, I guess the padding would look odd if there's nothing there.

Maybe just mention in this method's doc, then.

gnprice · 2025-06-24T00:04:41Z

lib/widgets/content.dart

+    Color? color;
+    LinearGradient? gradient;
+
+    switch (status) {


nit: join these to the switch that sets them

Suggested change

Color? color;

LinearGradient? gradient;

switch (status) {

Color? color;

LinearGradient? gradient;

switch (status) {

gnprice · 2025-06-24T00:13:34Z

lib/widgets/content.dart

  });

  final int userId;
  final double size;
  final double borderRadius;
+  final Color? backgroundColor;
+  final bool omitPresenceStatus;


Looking at other call sites of the Avatar constructor, and thinking about whether they should set this flag to omit presence.

autocomplete → keep

"My profile" in main menu → probably omit? seems like there's no useful information there; and pending presence: Handle presence events #1618, could be confusing by suggesting you're offline

lightbox → keep

msglist senders → see other comment

new DMs, two places → keep

top of profile → skip, as this version already does

users in custom profile fields → keep

gnprice · 2025-06-24T00:37:09Z

CI shows some test failures, in test/model/store_test.dart .

chrisbobbe · 2025-06-24T00:39:09Z

Thanks for the review! Revision pushed. This one omits the presence circle on sender rows in the message list, to match zulip-mobile and web.

gnprice

Thanks! All looks good except a nit below.

This one omits the presence circle on sender rows in the message list, to match zulip-mobile and web.

To be explicit for the thread: it also omits it on the "My profile" item in the main menu.

gnprice · 2025-06-24T00:53:16Z

lib/model/presence.dart

+  static void debugReset() {
+    _debugEnable = true;


nit:

Suggested change

static void debugReset() {

_debugEnable = true;

static void debugReset() {

debugEnable = true;

That way it goes through the setter above, so it's clear to the compiler that this field never gets written outside debug mode.

gnprice · 2025-06-24T00:58:32Z

Then I'll want to go ahead and merge, to simplify release management. So I guess let's file one more follow-up issue calling for tests.

chrisbobbe · 2025-06-24T01:01:55Z

Thanks! Done (#1620), and fixed that nit; PTAL.

We plan to write tests for this as a followup: zulip#1620. Notable differences from zulip-mobile: - Here, we make report-presence requests more frequently: our "app state" listener triggers a request immediately, instead of scheduling it when the "ping interval" expires. This approach anticipates the requests being handled much more efficiently, with presence_last_update_id (zulip#1611) -- but it shouldn't regress on performance now, because these immediate requests are done (for now) as "ping only", i.e., asking the server not to compute a presence data payload. - The newUserInput param is now usually true instead of always false. This seems more correct to me, and the change seems low-stakes (the doc says it's used to implement usage statistics); see the doc: https://zulip.com/api/update-presence#parameter-new_user_input Fixes: zulip#196

We plan to write tests for this as a followup: zulip#1620. Fixes: zulip#1607

gnprice · 2025-06-24T01:04:25Z

Thanks! Looks good; merging.

Tweaked one bit in the commit messages:

-    We plan to write tests for this as a followup: #1620
+    We plan to write tests for this as a followup: #1620.
 
     Fixes: #1607

because it was looking almost like an analogue of the "Fixes:" lines, and then it was bugging me because the name had spaces in it 🙂

I happened to notice this message getting printed repeatedly in the debug logs (reformatted a bit): [ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: NetworkException: HTTP request failed. Client is already closed. (ClientException: HTTP request failed. Client is already closed., uri=https://chat.zulip.org/api/v1/users/me/presence) #0 ApiConnection.send (package:zulip/api/core.dart:175) <asynchronous suspension> zulip#1 Presence._maybePingAndRecordResponse (package:zulip/model/presence.dart:93) <asynchronous suspension> zulip#2 Presence._poll (package:zulip/model/presence.dart:121) <asynchronous suspension> That'd be a symptom of an old Presence continuing to run its polling loop after the ApiConnection has been closed, which happens when the PerAccountStore is disposed. Looks like when we introduced Presence in 5d43df2 (zulip#1619), we forgot to call its `dispose` method. Fix that now. The presence model doesn't currently have any tests. So rather than try to add a test for just this, we'll leave it as something to include when we write those tests, zulip#1620.

I happened to notice this message getting printed repeatedly in the debug logs (reformatted a bit): [ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: NetworkException: HTTP request failed. Client is already closed. (ClientException: HTTP request failed. Client is already closed., uri=https://chat.zulip.org/api/v1/users/me/presence) #0 ApiConnection.send (package:zulip/api/core.dart:175) <asynchronous suspension> #1 Presence._maybePingAndRecordResponse (package:zulip/model/presence.dart:93) <asynchronous suspension> #2 Presence._poll (package:zulip/model/presence.dart:121) <asynchronous suspension> That'd be a symptom of an old Presence continuing to run its polling loop after the ApiConnection has been closed, which happens when the PerAccountStore is disposed. Looks like when we introduced Presence in 5d43df2 (#1619), we forgot to call its `dispose` method. Fix that now. The presence model doesn't currently have any tests. So rather than try to add a test for just this, we'll leave it as something to include when we write those tests, #1620.

store: Add realm- and server-level presence settings to PerAccountStore

3284ea5

chrisbobbe requested a review from gnprice June 23, 2025 23:12

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 23, 2025

chrisbobbe force-pushed the pr-presence branch from 31ac407 to c6fe6be Compare June 23, 2025 23:31

gnprice reviewed Jun 23, 2025

View reviewed changes

gnprice reviewed Jun 24, 2025

View reviewed changes

chrisbobbe force-pushed the pr-presence branch from c6fe6be to 6ad758c Compare June 24, 2025 00:38

gnprice reviewed Jun 24, 2025

View reviewed changes

chrisbobbe mentioned this pull request Jun 24, 2025

Write tests for presence (storing and reporting, and UI) #1620

Open

chrisbobbe force-pushed the pr-presence branch from 6ad758c to 17a5cda Compare June 24, 2025 01:01

chrisbobbe marked this pull request as ready for review June 24, 2025 01:01

chrisbobbe added 2 commits June 23, 2025 18:03

presence: Show presence on avatars throughout the app, and in profile

f11c52f

We plan to write tests for this as a followup: zulip#1620. Fixes: zulip#1607

gnprice force-pushed the pr-presence branch from 17a5cda to f11c52f Compare June 24, 2025 01:04

gnprice merged commit f11c52f into zulip:main Jun 24, 2025
1 check passed

chrisbobbe deleted the pr-presence branch June 24, 2025 01:06

gnprice mentioned this pull request Jul 19, 2025

presence: Dispose Presence object when store disposed #1719

Merged

	late UpdatePresenceResult result;
	final UpdatePresenceResult result;

	serverPresencePingIntervalSeconds: Duration(seconds: initialSnapshot.serverPresencePingIntervalSeconds),
	serverPresencePingInterval: Duration(seconds: initialSnapshot.serverPresencePingIntervalSeconds),

Store, report, and display presence! #1619

Store, report, and display presence! #1619

Uh oh!

Conversation

chrisbobbe commented Jun 23, 2025

Uh oh!

chrisbobbe commented Jun 23, 2025

Uh oh!

chrisbobbe commented Jun 23, 2025

Uh oh!

chrisbobbe commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 24, 2025

Uh oh!

chrisbobbe commented Jun 24, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 24, 2025

Uh oh!

chrisbobbe commented Jun 24, 2025

Uh oh!

gnprice commented Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!