Add a loading indicator for PerAccountStore #852

PIG208 · 2024-07-30T21:59:56Z

Preview

isLoading=false	isLoading=true

Fixes #465.

PIG208 · 2024-07-30T22:04:53Z

lib/model/store.dart

@@ -295,6 +295,15 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
  final GlobalStore _globalStore;
  final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events

+  bool get isLoading => _isLoading;
+  bool _isLoading = false;
+  @visibleForTesting


Useful for a widget test added later.

PIG208 · 2024-07-30T22:09:48Z

test/widgets/app_bar_test.dart

+    await store.addUser(eg.selfUser);
+
+    await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
+      child: ProfilePage(userId: eg.selfUser.userId)));


I intentionally only test this with one of the many call sites. This test does not prevent the bug where a new page is added without ZulipAppBar, as we don't have a way to exhaustively list pages that have access to a PerAccountStoreWidget yet.

chrisbobbe · 2024-07-31T18:19:54Z

Looks like CI is failing; could you fix that please?

PIG208 · 2024-07-31T18:28:40Z

Oops, just pushed again to fix that.

chrisbobbe

Thanks!

I'm curious if you explored an approach with a toast: #465 (comment)

Would it be easy to put some code in one central place, ensuring that a toast showing the org's name is visible for each account in the loading state? (E.g., for CZO: "Connecting to Zulip Community…")

Or, a refinement (that could be annoying to implement): filter that list of toasts down to the account corresponding to the foregrounded screen, and no need to show the org's name.

In the screenshots, I'm noticing the color isn't a great match for the app bar, and the split between the app bar and the sticky recipient header is looking kind of awkward.

If we're going with this linear-indicator approach, let's definitely track that it's missing on the lightbox screen, which needs it because it's a per-account screen too.

chrisbobbe · 2024-07-31T21:22:44Z

lib/widgets/message_list.dart

-          : null, // i.e., inherit
-      ),
+          : null),


Let's keep this comment. A reader might see null and think it means there's no bottom border. That would be wrong; what null actually does is make the app bar inherit its bottom border from the AppBarTheme.

PIG208 · 2024-07-31T22:55:35Z

Thanks for the review! I didn't experiment with Toast. It sounds like another viable option.

Could you elaborate on this?

Or, a refinement (that could be annoying to implement): filter that list of toasts down to the account corresponding to the foregrounded screen, and no need to show the org's name.

Visually, matching the background color should be fairly simple, because LinearProgressIndicator is shipped with the app bar (this doesn't address the lightbox issue with its app bar, though).

diff --git lib/widgets/app_bar.dart lib/widgets/app_bar.dart
index da49a639..a1c4645b 100644
--- lib/widgets/app_bar.dart
+++ lib/widgets/app_bar.dart
@@ -12,6 +12,6 @@ class ZulipAppBar extends AppBar {
     bottom: PreferredSize(
       preferredSize: const Size.fromHeight(4.0),
       child: (isLoading)
-        ? const LinearProgressIndicator(minHeight: 4.0)
+        ? LinearProgressIndicator(backgroundColor: backgroundColor, minHeight: 4.0)
         : const SizedBox.shrink()));
 }

PIG208 · 2024-08-01T19:19:22Z

Do we have a toast library in mind to use? Or are we writing one from scratch? Looks like SnackBar is the official implementation.

gnprice · 2024-08-01T22:46:01Z

Chat thread about what type of indicator to use:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/banner.2Fsnack.20bar.2Fprogress.20indicator.20for.20connection.20error/near/1909308

gnprice · 2024-08-01T23:36:43Z

If we're going with this linear-indicator approach, let's definitely track that it's missing on the lightbox screen, which needs it because it's a per-account screen too.

I think it's actually fine to not have the indicator on the lightbox screen. Critically, there's very little there to get live-updated — even if you're looking at stale data there and you think it's up to date, it's hard to get misled about much as a result. In particular it never gets updated by a new message arriving.

It should definitely get a comment saying we've chosen not to put the indicator there, though, and why.

chrisbobbe · 2024-08-02T00:26:36Z

Sure, I guess that all makes sense.

chrisbobbe · 2024-08-02T21:25:16Z

So the current revision is close to what we want, right? Is this the only thing missing:

It should definitely get a comment saying we've chosen not to put the indicator there, though, and why.

?

PIG208 · 2024-08-02T21:51:27Z

Right, will update this later.

PIG208 · 2024-08-02T22:11:08Z

Updated the PR to mention the ZulipAppBar exceptions in comments after its dart doc.

PIG208 · 2024-08-05T18:15:08Z

Updated the PR to handle isLoading when there are transient connection errors.

chrisbobbe · 2024-08-06T02:23:10Z

Thanks! LGTM, over to Greg for his review.

PIG208 · 2024-08-08T01:17:06Z

(Rebased the PR)

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the previous reviews!

Generally this looks good. I also put a build with this PR (among others) onto my phone last week before going on vacation, and the behavior was very helpful.

A couple of small comments below. I've also pushed some additional commits on top:
c384847 wip squash? plain AppBar where isLoading always false
667adf8 app_bar [nfc]: Move ZulipAppBar.isLoading to within widget
a6965aa app_bar [nfc]: Explain why the lightbox skips ZulipAppBar

The first one reflects one comment, and can be squashed; I added it just because that change is needed for the next commit.

gnprice · 2024-08-09T20:18:21Z

lib/widgets/app.dart

+      appBar: ZulipAppBar(
        title: Text(zulipLocalizations.chooseAccountPageTitle),
-        actions: const [ChooseAccountPageOverflowButton()]),
+        actions: const [ChooseAccountPageOverflowButton()],
+        isLoading: false),


How about just AppBar, if we're going to hard-code isLoading here to false?

gnprice · 2024-08-09T20:29:26Z

lib/widgets/inbox.dart

-      appBar: AppBar(title: const Text('Inbox')),
+      appBar: ZulipAppBar(
+        title: const Text('Inbox'),
+        isLoading: store.isLoading),


I think this pattern can be simplified. I'll push a separate commit on top to do so.

When an error occurs to a getEvents call, there isn't a visual indicator for the retry attempts. This prepares the data model for the feature. Signed-off-by: Zixuan James Li <[email protected]>

Ideally we may have test to exhaustively ensure that all pages specific to a single PerAccountStore use ZulipAppBar. Some pages with `AppBar`s are skipped, such as AboutZulip (no PerAccountStore access) and lightboxes (progress indicator is occupied for other purposes, and the AppBar can be hidden). Fixes zulip#465.

This keeps call sites simpler.

Just listing that there is an exception doesn't help much to provide guidance for someone writing a new page on whether they should also skip ZulipAppBar there. The "why" is much more helpful for that. Right at the call site is also more helpful than at the definition of ZulipAppBar, because the call site is the place someone will definitely be looking if they're contemplating whether this deviation from the pattern is appropriate, a mistake, or just an oversight.

PIG208 · 2024-08-09T22:38:40Z

Thanks! Rebased the PR and squashed the commit.

gnprice · 2024-08-09T23:17:44Z

Looks good! Thanks for the revision — merging.

PIG208 force-pushed the connection branch from d2e4c36 to 4c593e2 Compare July 30, 2024 22:01

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jul 30, 2024

PIG208 force-pushed the connection branch from 4c593e2 to 408cbfc Compare July 30, 2024 22:04

PIG208 commented Jul 30, 2024

View reviewed changes

PIG208 requested a review from chrisbobbe July 30, 2024 22:10

PIG208 force-pushed the connection branch from 408cbfc to f7b25e2 Compare July 31, 2024 18:28

chrisbobbe reviewed Jul 31, 2024

View reviewed changes

PIG208 force-pushed the connection branch from f7b25e2 to be79b7f Compare July 31, 2024 23:36

PIG208 force-pushed the connection branch from be79b7f to 90d3e6d Compare August 2, 2024 22:09

PIG208 force-pushed the connection branch from 90d3e6d to 39deb93 Compare August 5, 2024 18:13

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 6, 2024

PIG208 force-pushed the connection branch from 39deb93 to 69e19aa Compare August 8, 2024 01:16

gnprice reviewed Aug 9, 2024

View reviewed changes

PIG208 added 2 commits August 9, 2024 18:37

store: Add field for tracking loading status.

5380574

When an error occurs to a getEvents call, there isn't a visual indicator for the retry attempts. This prepares the data model for the feature. Signed-off-by: Zixuan James Li <[email protected]>

gnprice added 2 commits August 9, 2024 18:38

app_bar [nfc]: Move ZulipAppBar.isLoading to within widget

50d39ea

This keeps call sites simpler.

PIG208 force-pushed the connection branch from a6965aa to 1fb76ac Compare August 9, 2024 22:38

gnprice merged commit 1fb76ac into zulip:main Aug 9, 2024
1 check passed

PIG208 deleted the connection branch August 12, 2024 18:16

gnprice mentioned this pull request Aug 14, 2024

Slow sometimes to replace event queue after expiry #809

Open

PIG208 mentioned this pull request Oct 3, 2024

Loading indicator does not go away when the event queue appears to be active #979

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a loading indicator for PerAccountStore #852

Add a loading indicator for PerAccountStore #852

PIG208 commented Jul 30, 2024

PIG208 Jul 30, 2024

PIG208 Jul 30, 2024

chrisbobbe commented Jul 31, 2024

PIG208 commented Jul 31, 2024

chrisbobbe left a comment •

edited

Loading

chrisbobbe Jul 31, 2024

PIG208 commented Jul 31, 2024 •

edited

Loading

PIG208 commented Aug 1, 2024 •

edited

Loading

gnprice commented Aug 1, 2024

gnprice commented Aug 1, 2024

chrisbobbe commented Aug 2, 2024

chrisbobbe commented Aug 2, 2024 •

edited

Loading

PIG208 commented Aug 2, 2024

PIG208 commented Aug 2, 2024

PIG208 commented Aug 5, 2024

chrisbobbe commented Aug 6, 2024

PIG208 commented Aug 8, 2024

gnprice left a comment

gnprice Aug 9, 2024

gnprice Aug 9, 2024

PIG208 commented Aug 9, 2024

gnprice commented Aug 9, 2024

Add a loading indicator for PerAccountStore #852

Add a loading indicator for PerAccountStore #852

Conversation

PIG208 commented Jul 30, 2024

PIG208 Jul 30, 2024

Choose a reason for hiding this comment

PIG208 Jul 30, 2024

Choose a reason for hiding this comment

chrisbobbe commented Jul 31, 2024

PIG208 commented Jul 31, 2024

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jul 31, 2024

Choose a reason for hiding this comment

PIG208 commented Jul 31, 2024 • edited Loading

PIG208 commented Aug 1, 2024 • edited Loading

gnprice commented Aug 1, 2024

gnprice commented Aug 1, 2024

chrisbobbe commented Aug 2, 2024

chrisbobbe commented Aug 2, 2024 • edited Loading

PIG208 commented Aug 2, 2024

PIG208 commented Aug 2, 2024

PIG208 commented Aug 5, 2024

chrisbobbe commented Aug 6, 2024

PIG208 commented Aug 8, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 9, 2024

Choose a reason for hiding this comment

gnprice Aug 9, 2024

Choose a reason for hiding this comment

PIG208 commented Aug 9, 2024

gnprice commented Aug 9, 2024

chrisbobbe left a comment •

edited

Loading

PIG208 commented Jul 31, 2024 •

edited

Loading

PIG208 commented Aug 1, 2024 •

edited

Loading

chrisbobbe commented Aug 2, 2024 •

edited

Loading