Skip to content

Add a loading indicator for PerAccountStore #852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 9, 2024
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
11 changes: 11 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,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
Copy link
Member Author

Choose a reason for hiding this comment

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

Useful for a widget test added later.

set isLoading(bool value) {
if (_isLoading == value) return;
_isLoading = value;
notifyListeners();
}

////////////////////////////////
// Data attached to the realm or the server.

Expand Down Expand Up @@ -770,6 +779,7 @@ class UpdateMachine {
result = await getEvents(store.connection,
queueId: queueId, lastEventId: lastEventId);
} catch (e) {
store.isLoading = true;
switch (e) {
case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
assert(debugLog('Lost event queue for $store. Replacing…'));
Expand Down Expand Up @@ -797,6 +807,7 @@ class UpdateMachine {
}
}

store.isLoading = false;
final events = result.events;
for (final event in events) {
await store.handleEvent(event);
Expand Down
3 changes: 2 additions & 1 deletion lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import '../model/localizations.dart';
import '../model/narrow.dart';
import 'about_zulip.dart';
import 'app_bar.dart';
import 'inbox.dart';
import 'login.dart';
import 'message_list.dart';
Expand Down Expand Up @@ -252,7 +253,7 @@ class HomePage extends StatelessWidget {
}

return Scaffold(
appBar: AppBar(title: const Text("Home")),
appBar: ZulipAppBar(title: const Text("Home")),
body: Center(
child: Column(mainAxisAlignment: MainAxisAlignment.center, children: [
DefaultTextStyle.merge(
Expand Down
32 changes: 32 additions & 0 deletions lib/widgets/app_bar.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import 'package:flutter/material.dart';

import 'store.dart';

/// A custom [AppBar] with a loading indicator.
///
/// This should be used for most of the pages with access to [PerAccountStore].
class ZulipAppBar extends AppBar {
ZulipAppBar({
super.key,
required super.title,
super.backgroundColor,
super.shape,
super.actions,
}) : super(bottom: _ZulipAppBarBottom(backgroundColor: backgroundColor));
}

class _ZulipAppBarBottom extends StatelessWidget implements PreferredSizeWidget {
const _ZulipAppBarBottom({this.backgroundColor});

final Color? backgroundColor;

@override
Size get preferredSize => const Size.fromHeight(4.0);

@override
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
if (!store.isLoading) return const SizedBox.shrink();
return LinearProgressIndicator(minHeight: 4.0, backgroundColor: backgroundColor);
}
}
3 changes: 2 additions & 1 deletion lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import '../api/model/model.dart';
import '../model/narrow.dart';
import '../model/recent_dm_conversations.dart';
import '../model/unreads.dart';
import 'app_bar.dart';
import 'icons.dart';
import 'message_list.dart';
import 'page.dart';
Expand Down Expand Up @@ -160,7 +161,7 @@ class _InboxPageState extends State<InboxPage> with PerAccountStoreAwareStateMix
}

return Scaffold(
appBar: AppBar(title: const Text('Inbox')),
appBar: ZulipAppBar(title: const Text('Inbox')),
body: SafeArea(
// Don't pad the bottom here; we want the list content to do that.
bottom: false,
Expand Down
7 changes: 7 additions & 0 deletions lib/widgets/lightbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ class _LightboxPageLayoutState extends State<_LightboxPageLayout> {
.add_Hms()
.format(DateTime.fromMillisecondsSinceEpoch(widget.message.timestamp * 1000));

// We use plain [AppBar] instead of [ZulipAppBar], even though this page
// has a [PerAccountStore], because:
// * There's already a progress indicator with a different meaning
// (loading the image).
// * The app bar can be hidden, so wouldn't always be visible anyway.
// * This is a page where the store loading indicator isn't especially
// necessary: https://github.com/zulip/zulip-flutter/pull/852#issuecomment-2264211917
appBar = AppBar(
centerTitle: false,
foregroundColor: appBarForegroundColor,
Expand Down
4 changes: 3 additions & 1 deletion lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import '../model/store.dart';
import '../model/typing_status.dart';
import 'action_sheet.dart';
import 'actions.dart';
import 'app_bar.dart';
import 'compose_box.dart';
import 'content.dart';
import 'dialog.dart';
Expand Down Expand Up @@ -268,7 +269,8 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
}

return Scaffold(
appBar: AppBar(title: MessageListAppBarTitle(narrow: narrow),
appBar: ZulipAppBar(
title: MessageListAppBarTitle(narrow: narrow),
backgroundColor: appBarBackgroundColor,
shape: removeAppBarBottomBorder
? const Border()
Expand Down
5 changes: 3 additions & 2 deletions lib/widgets/profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import '../api/model/model.dart';
import '../model/content.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import 'app_bar.dart';
import 'content.dart';
import 'message_list.dart';
import 'page.dart';
Expand Down Expand Up @@ -102,7 +103,7 @@ class ProfilePage extends StatelessWidget {
];

return Scaffold(
appBar: AppBar(title: Text(user.fullName)),
appBar: ZulipAppBar(title: Text(user.fullName)),
body: SingleChildScrollView(
child: Center(
child: ConstrainedBox(
Expand All @@ -121,7 +122,7 @@ class _ProfileErrorPage extends StatelessWidget {
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(title: const Text('Error')),
appBar: ZulipAppBar(title: const Text('Error')),
body: const SingleChildScrollView(
child: Padding(
padding: EdgeInsets.symmetric(horizontal: 16, vertical: 32),
Expand Down
4 changes: 3 additions & 1 deletion lib/widgets/recent_dm_conversations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import '../model/narrow.dart';
import '../model/recent_dm_conversations.dart';
import '../model/unreads.dart';
import 'app_bar.dart';
import 'content.dart';
import 'icons.dart';
import 'message_list.dart';
Expand Down Expand Up @@ -58,7 +59,8 @@ class _RecentDmConversationsPageState extends State<RecentDmConversationsPage> w
final zulipLocalizations = ZulipLocalizations.of(context);
final sorted = model!.sorted;
return Scaffold(
appBar: AppBar(title: Text(zulipLocalizations.recentDmConversationsPageTitle)),
appBar: ZulipAppBar(
title: Text(zulipLocalizations.recentDmConversationsPageTitle)),
body: SafeArea(
// Don't pad the bottom here; we want the list content to do that.
bottom: false,
Expand Down
3 changes: 2 additions & 1 deletion lib/widgets/subscription_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'package:flutter/material.dart';
import '../api/model/model.dart';
import '../model/narrow.dart';
import '../model/unreads.dart';
import 'app_bar.dart';
import 'icons.dart';
import 'message_list.dart';
import 'page.dart';
Expand Down Expand Up @@ -89,7 +90,7 @@ class _SubscriptionListPageState extends State<SubscriptionListPage> with PerAcc
_sortSubs(unpinned);

return Scaffold(
appBar: AppBar(title: const Text("Channels")),
appBar: ZulipAppBar(title: const Text("Channels")),
body: SafeArea(
// Don't pad the bottom here; we want the list content to do that.
bottom: false,
Expand Down
1 change: 1 addition & 0 deletions test/model/store_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ extension GlobalStoreChecks on Subject<GlobalStore> {

extension PerAccountStoreChecks on Subject<PerAccountStore> {
Subject<ApiConnection> get connection => has((x) => x.connection, 'connection');
Subject<bool> get isLoading => has((x) => x.isLoading, 'isLoading');
Subject<Uri> get realmUrl => has((x) => x.realmUrl, 'realmUrl');
Subject<String> get zulipVersion => has((x) => x.zulipVersion, 'zulipVersion');
Subject<int> get maxFileUploadSizeMib => has((x) => x.maxFileUploadSizeMib, 'maxFileUploadSizeMib');
Expand Down
6 changes: 5 additions & 1 deletion test/model/store_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,12 @@ void main() {
updateMachine.debugAdvanceLoop();
async.flushMicrotasks();
await Future<void>.delayed(Duration.zero);
check(store).isLoading.isTrue();

// The global store has a new store.
check(globalStore.perAccountSync(store.accountId)).not((it) => it.identicalTo(store));
updateFromGlobalStore();
check(store).isLoading.isFalse();

// The new UpdateMachine updates the new store.
updateMachine.debugPauseLoop();
Expand All @@ -435,8 +437,9 @@ void main() {
// Make the request, inducing an error in it.
prepareError();
updateMachine.debugAdvanceLoop();
async.flushMicrotasks();
async.elapse(Duration.zero);
checkLastRequest(lastEventId: 1);
check(store).isLoading.isTrue();

// Polling doesn't resume immediately; there's a timer.
check(async.pendingTimers).length.equals(1);
Expand All @@ -452,6 +455,7 @@ void main() {
async.flushTimers();
checkLastRequest(lastEventId: 1);
check(updateMachine.lastEventId).equals(2);
check(store).isLoading.isFalse();
});
}

Expand Down
38 changes: 38 additions & 0 deletions test/widgets/app_bar_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import 'package:checks/checks.dart';
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/widgets/app_bar.dart';
import 'package:zulip/widgets/profile.dart';

import '../example_data.dart' as eg;
import '../model/binding.dart';
import '../model/test_store.dart';
import 'test_app.dart';

void main() {
TestZulipBinding.ensureInitialized();

testWidgets('show progress indicator when loading', (tester) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());

final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUser(eg.selfUser);

await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
child: ProfilePage(userId: eg.selfUser.userId)));
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


final finder = find.descendant(
of: find.byType(ZulipAppBar),
matching: find.byType(LinearProgressIndicator));

await tester.pumpAndSettle();
final rectBefore = tester.getRect(find.byType(ZulipAppBar));
check(finder.evaluate()).isEmpty();
store.isLoading = true;

await tester.pump();
check(tester.getRect(find.byType(ZulipAppBar))).equals(rectBefore);
check(finder.evaluate()).single;
});
}