Skip to content

settings: Migrate to new RadioGroup API; write widget tests for two just-added settings #1602

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

settings: Migrate to new RadioGroup API; write widget tests for two just-added settings #1602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45a2e60
settings test: Check theme-setting radio buttons by checking semantics
chrisbobbe Jun 3, 2025
82dbbfe
settings test [nfc]: Pull out checkRadioButtonAppearsChecked helper
chrisbobbe Jun 17, 2025
9743cec
settings test: Add widget tests for latest settings
chrisbobbe Jun 17, 2025
5f9512e
settings: Migrate to new RadioGroup API for radio buttons
chrisbobbe Jun 3, 2025
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
86 changes: 40 additions & 46 deletions lib/widgets/settings.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,19 @@ class _ThemeSetting extends StatelessWidget {
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
final globalSettings = GlobalStoreWidget.settingsOf(context);
return Column(
children: [
ListTile(title: Text(zulipLocalizations.themeSettingTitle)),
for (final themeSettingOption in [null, ...ThemeSetting.values])
RadioListTile<ThemeSetting?>.adaptive(
title: Text(ThemeSetting.displayName(
themeSetting: themeSettingOption,
zulipLocalizations: zulipLocalizations)),
value: themeSettingOption,
// TODO(#1545) stop using the deprecated members
// ignore: deprecated_member_use
groupValue: globalSettings.themeSetting,
// ignore: deprecated_member_use
onChanged: (newValue) => _handleChange(context, newValue)),
]);
return RadioGroup<ThemeSetting?>(
groupValue: globalSettings.themeSetting,
onChanged: (newValue) => _handleChange(context, newValue),
child: Column(
children: [
ListTile(title: Text(zulipLocalizations.themeSettingTitle)),
for (final themeSettingOption in [null, ...ThemeSetting.values])
RadioListTile<ThemeSetting?>.adaptive(
title: Text(ThemeSetting.displayName(
themeSetting: themeSettingOption,
zulipLocalizations: zulipLocalizations)),
value: themeSettingOption),
]));
}
}

Expand Down Expand Up @@ -135,19 +133,17 @@ class VisitFirstUnreadSettingPage extends StatelessWidget {
final globalSettings = GlobalStoreWidget.settingsOf(context);
return Scaffold(
appBar: AppBar(title: Text(zulipLocalizations.initialAnchorSettingTitle)),
body: Column(children: [
ListTile(title: Text(zulipLocalizations.initialAnchorSettingDescription)),
for (final value in VisitFirstUnreadSetting.values)
RadioListTile.adaptive(
title: Text(_valueDisplayName(value,
zulipLocalizations: zulipLocalizations)),
value: value,
// TODO(#1545) stop using the deprecated members
// ignore: deprecated_member_use
groupValue: globalSettings.visitFirstUnread,
// ignore: deprecated_member_use
onChanged: (newValue) => _handleChange(context, newValue)),
]));
body: RadioGroup<VisitFirstUnreadSetting>(
groupValue: globalSettings.visitFirstUnread,
onChanged: (newValue) => _handleChange(context, newValue),
child: Column(children: [
ListTile(title: Text(zulipLocalizations.initialAnchorSettingDescription)),
for (final value in VisitFirstUnreadSetting.values)
RadioListTile<VisitFirstUnreadSetting>.adaptive(
title: Text(_valueDisplayName(value,
zulipLocalizations: zulipLocalizations)),
value: value),
])));
}
}

Expand Down Expand Up @@ -210,24 +206,22 @@ class MarkReadOnScrollSettingPage extends StatelessWidget {
final globalSettings = GlobalStoreWidget.settingsOf(context);
return Scaffold(
appBar: AppBar(title: Text(zulipLocalizations.markReadOnScrollSettingTitle)),
body: Column(children: [
ListTile(title: Text(zulipLocalizations.markReadOnScrollSettingDescription)),
for (final value in MarkReadOnScrollSetting.values)
RadioListTile.adaptive(
title: Text(_valueDisplayName(value,
zulipLocalizations: zulipLocalizations)),
subtitle: () {
final result = _valueDescription(value,
zulipLocalizations: zulipLocalizations);
return result == null ? null : Text(result);
}(),
value: value,
// TODO(#1545) stop using the deprecated members
// ignore: deprecated_member_use
groupValue: globalSettings.markReadOnScroll,
// ignore: deprecated_member_use
onChanged: (newValue) => _handleChange(context, newValue)),
]));
body: RadioGroup<MarkReadOnScrollSetting>(
groupValue: globalSettings.markReadOnScroll,
onChanged: (newValue) => _handleChange(context, newValue),
child: Column(children: [
ListTile(title: Text(zulipLocalizations.markReadOnScrollSettingDescription)),
for (final value in MarkReadOnScrollSetting.values)
RadioListTile<MarkReadOnScrollSetting>.adaptive(
title: Text(_valueDisplayName(value,
zulipLocalizations: zulipLocalizations)),
subtitle: () {
final result = _valueDescription(value,
zulipLocalizations: zulipLocalizations);
return result == null ? null : Text(result);
}(),
value: value),
])));
}
}

Expand Down
6 changes: 0 additions & 6 deletions test/flutter_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,3 @@ extension IconButtonChecks on Subject<IconButton> {
extension SwitchListTileChecks<T> on Subject<SwitchListTile> {
Subject<bool> get value => has((x) => x.value, 'value');
}

extension RadioListTileChecks<T> on Subject<RadioListTile<T>> {
// TODO(#1545) stop using the deprecated member
// ignore: deprecated_member_use
Subject<bool> get checked => has((x) => x.checked, 'checked');
}
2 changes: 2 additions & 0 deletions test/model/store_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ extension GlobalSettingsStoreChecks on Subject<GlobalSettingsStore> {
Subject<BrowserPreference?> get browserPreference => has((x) => x.browserPreference, 'browserPreference');
Subject<BrowserPreference> get effectiveBrowserPreference => has((x) => x.effectiveBrowserPreference, 'effectiveBrowserPreference');
Subject<UrlLaunchMode> getUrlLaunchMode(Uri url) => has((x) => x.getUrlLaunchMode(url), 'getUrlLaunchMode');
Subject<VisitFirstUnreadSetting> get visitFirstUnread => has((x) => x.visitFirstUnread, 'visitFirstUnread');
Subject<MarkReadOnScrollSetting> get markReadOnScroll => has((x) => x.markReadOnScroll, 'markReadOnScroll');
Subject<bool> getBool(BoolGlobalSetting setting) => has((x) => x.getBool(setting), 'getBool(${setting.name}');
}

Expand Down
186 changes: 176 additions & 10 deletions test/widgets/settings_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,70 @@
import 'package:checks/checks.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_checks/flutter_checks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/model/settings.dart';
import 'package:zulip/widgets/page.dart';
import 'package:zulip/widgets/settings.dart';
import 'package:zulip/widgets/store.dart';

import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/store_checks.dart';
import '../example_data.dart' as eg;
import '../test_navigation.dart';
import 'page_checks.dart';
import 'test_app.dart';

void main() {
TestZulipBinding.ensureInitialized();

late TestNavigatorObserver testNavObserver;
late Route<dynamic>? lastPushedRoute;
late Route<dynamic>? lastPoppedRoute;

Future<void> prepare(WidgetTester tester) async {
addTearDown(testBinding.reset);

testNavObserver = TestNavigatorObserver()
..onPushed = ((route, _) => lastPushedRoute = route)
..onPopped = ((route, _) => lastPoppedRoute = route);
lastPushedRoute = null;
lastPoppedRoute = null;
Comment on lines +29 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but: given how much this pattern is recurring now, I'm thinking it'd be good to centralize the code for it.

Perhaps by giving TestNavigatorObserver (or a subclass) some more structure, to track this sort of state itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed; #1668


await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
await tester.pumpWidget(TestZulipApp(
accountId: eg.selfAccount.id,
navigatorObservers: [testNavObserver],
child: SettingsPage()));
await tester.pump();
await tester.pump();
}

group('ThemeSetting', () {
Finder findRadioListTileWithTitle(String title) => find.ancestor(
of: find.text(title),
matching: find.byType(RadioListTile<ThemeSetting?>));
void checkTileOnSettingsPage(WidgetTester tester, {
required String expectedTitle,
required String expectedSubtitle,
}) {
check(find.descendant(of: find.widgetWithText(ListTile, expectedTitle),
matching: find.text(expectedSubtitle))).findsOne();
}

Finder findRadioListTileWithTitle<T>(String title) => find.ancestor(
of: find.text(title),
matching: find.byType(RadioListTile<T>));

void checkRadioButtonAppearsChecked<T>(WidgetTester tester,
String title, bool expectedIsChecked, {String? subtitle}) {
check(tester.semantics.find(findRadioListTileWithTitle<T>(title)))
.containsSemantics(
label: subtitle == null
? title
: '$title\n$subtitle',
isInMutuallyExclusiveGroup: true,
hasCheckedState: true, isChecked: expectedIsChecked);
}

group('ThemeSetting', () {
void checkThemeSetting(WidgetTester tester, {
required ThemeSetting? expectedThemeSetting,
}) {
Expand All @@ -39,9 +74,7 @@ void main() {
ThemeSetting.dark => 'Dark',
};
for (final title in ['System', 'Light', 'Dark']) {
check(tester.widget<RadioListTile<ThemeSetting?>>(
findRadioListTileWithTitle(title)))
.checked.equals(title == expectedCheckedTitle);
checkRadioButtonAppearsChecked<ThemeSetting?>(tester, title, title == expectedCheckedTitle);
}
check(testBinding.globalStore)
.settings.themeSetting.equals(expectedThemeSetting);
Expand All @@ -56,13 +89,13 @@ void main() {
check(Theme.of(element)).brightness.equals(Brightness.light);
checkThemeSetting(tester, expectedThemeSetting: ThemeSetting.light);

await tester.tap(findRadioListTileWithTitle('Dark'));
await tester.tap(findRadioListTileWithTitle<ThemeSetting?>('Dark'));
await tester.pump();
await tester.pump(Duration(milliseconds: 250)); // wait for transition
check(Theme.of(element)).brightness.equals(Brightness.dark);
checkThemeSetting(tester, expectedThemeSetting: ThemeSetting.dark);

await tester.tap(findRadioListTileWithTitle('System'));
await tester.tap(findRadioListTileWithTitle<ThemeSetting?>('System'));
await tester.pump();
await tester.pump(Duration(milliseconds: 250)); // wait for transition
check(Theme.of(element)).brightness.equals(Brightness.light);
Expand Down Expand Up @@ -127,7 +160,140 @@ void main() {
}, variant: TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));
});

// TODO(#1571): test visitFirstUnread setting UI
group('VisitFirstUnreadSetting', () {
String settingTitle(VisitFirstUnreadSetting setting) => switch (setting) {
VisitFirstUnreadSetting.always => 'First unread message',
VisitFirstUnreadSetting.conversations => 'First unread message in conversation views, newest message elsewhere',
VisitFirstUnreadSetting.never => 'Newest message',
};

void checkPage(WidgetTester tester, {
required VisitFirstUnreadSetting expectedSetting,
}) {
for (final setting in VisitFirstUnreadSetting.values) {
final thisSettingTitle = settingTitle(setting);
checkRadioButtonAppearsChecked<VisitFirstUnreadSetting>(tester,
thisSettingTitle, setting == expectedSetting);
}
}

testWidgets('smoke', (tester) async {
await prepare(tester);

// "conversations" is the default, and it appears in the SettingsPage
// (as the setting tile's subtitle)
check(GlobalStoreWidget.settingsOf(tester.element(find.byType(SettingsPage))))
.visitFirstUnread.equals(VisitFirstUnreadSetting.conversations);
checkTileOnSettingsPage(tester,
expectedTitle: 'Open message feeds at',
expectedSubtitle: settingTitle(VisitFirstUnreadSetting.conversations));

await tester.tap(find.text('Open message feeds at'));
await tester.pump();
check(lastPushedRoute).isA<MaterialWidgetRoute>()
.page.isA<VisitFirstUnreadSettingPage>();
await tester.pump((lastPushedRoute as TransitionRoute).transitionDuration);
Copy link
Member

Choose a reason for hiding this comment

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

(again not for this PR)

If the nav observer tracks a log of both pushes and pops in a single list (or I guess even just tracks the last single such action), then this could perhaps even look something like:

Suggested change
await tester.pump((lastPushedRoute as TransitionRoute).transitionDuration);
await tester.pump(navObserver.lastTransitionDuration);

and exactly the same in the pop case (when the last thing that happened was a pop, the lastTransitionDuration getter would look at its reverseTransitionDuration).

checkPage(tester, expectedSetting: VisitFirstUnreadSetting.conversations);

await tester.tap(findRadioListTileWithTitle<VisitFirstUnreadSetting>(
settingTitle(VisitFirstUnreadSetting.always)));
await tester.pump();
checkPage(tester, expectedSetting: VisitFirstUnreadSetting.always);

await tester.tap(findRadioListTileWithTitle<VisitFirstUnreadSetting>(
settingTitle(VisitFirstUnreadSetting.conversations)));
await tester.pump();
checkPage(tester, expectedSetting: VisitFirstUnreadSetting.conversations);

await tester.tap(findRadioListTileWithTitle<VisitFirstUnreadSetting>(
settingTitle(VisitFirstUnreadSetting.never)));
await tester.pump();
checkPage(tester, expectedSetting: VisitFirstUnreadSetting.never);

await tester.tap(find.backButton());
check(lastPoppedRoute).isA<MaterialWidgetRoute>()
.page.isA<VisitFirstUnreadSettingPage>();
await tester.pump((lastPoppedRoute as TransitionRoute).reverseTransitionDuration);
check(GlobalStoreWidget.settingsOf(tester.element(find.byType(SettingsPage))))
.visitFirstUnread.equals(VisitFirstUnreadSetting.never);

checkTileOnSettingsPage(tester,
expectedTitle: 'Open message feeds at',
expectedSubtitle: settingTitle(VisitFirstUnreadSetting.never));
});
});

group('MarkReadOnScrollSetting', () {
Copy link
Member

Choose a reason for hiding this comment

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

If we add one or two more of these, we'll likely want to try abstracting them out a bit more — the body of the test case is repetitive enough that it risks getting hard to read without glossing over.

But explicit is always a good way to write the first version, and I think it works fine for two of these.

String settingTitle(MarkReadOnScrollSetting setting) => switch (setting) {
MarkReadOnScrollSetting.always => 'Always',
MarkReadOnScrollSetting.conversations => 'Only in conversation views',
MarkReadOnScrollSetting.never => 'Never',
};

String? settingSubtitle(MarkReadOnScrollSetting setting) => switch (setting) {
MarkReadOnScrollSetting.always => null,
MarkReadOnScrollSetting.conversations =>
'Messages will be automatically marked as read only when viewing a single topic or direct message conversation.',
MarkReadOnScrollSetting.never => null,
};

void checkPage(WidgetTester tester, {
required MarkReadOnScrollSetting expectedSetting,
}) {
for (final setting in MarkReadOnScrollSetting.values) {
final thisSettingTitle = settingTitle(setting);
checkRadioButtonAppearsChecked<MarkReadOnScrollSetting>(tester,
thisSettingTitle,
setting == expectedSetting,
subtitle: settingSubtitle(setting));
}
}

testWidgets('smoke', (tester) async {
await prepare(tester);

// "conversations" is the default, and it appears in the SettingsPage
// (as the setting tile's subtitle)
check(GlobalStoreWidget.settingsOf(tester.element(find.byType(SettingsPage))))
.markReadOnScroll.equals(MarkReadOnScrollSetting.conversations);
checkTileOnSettingsPage(tester,
expectedTitle: 'Mark messages as read on scroll',
expectedSubtitle: settingTitle(MarkReadOnScrollSetting.conversations));

await tester.tap(find.text('Mark messages as read on scroll'));
await tester.pump();
check(lastPushedRoute).isA<MaterialWidgetRoute>()
.page.isA<MarkReadOnScrollSettingPage>();
await tester.pump((lastPushedRoute as TransitionRoute).transitionDuration);
checkPage(tester, expectedSetting: MarkReadOnScrollSetting.conversations);

await tester.tap(findRadioListTileWithTitle<MarkReadOnScrollSetting>(
settingTitle(MarkReadOnScrollSetting.always)));
await tester.pump();
checkPage(tester, expectedSetting: MarkReadOnScrollSetting.always);

await tester.tap(findRadioListTileWithTitle<MarkReadOnScrollSetting>(
settingTitle(MarkReadOnScrollSetting.conversations)));
await tester.pump();
checkPage(tester, expectedSetting: MarkReadOnScrollSetting.conversations);

await tester.tap(findRadioListTileWithTitle<MarkReadOnScrollSetting>(
settingTitle(MarkReadOnScrollSetting.never)));
await tester.pump();
checkPage(tester, expectedSetting: MarkReadOnScrollSetting.never);

await tester.tap(find.byType(BackButton));
check(lastPoppedRoute).isA<MaterialWidgetRoute>()
.page.isA<MarkReadOnScrollSettingPage>();
await tester.pump((lastPoppedRoute as TransitionRoute).reverseTransitionDuration);
check(GlobalStoreWidget.settingsOf(tester.element(find.byType(SettingsPage))))
.markReadOnScroll.equals(MarkReadOnScrollSetting.never);

checkTileOnSettingsPage(tester,
expectedTitle: 'Mark messages as read on scroll',
expectedSubtitle: settingTitle(MarkReadOnScrollSetting.never));
});
});

// TODO maybe test GlobalSettingType.experimentalFeatureFlag settings
// Or maybe not; after all, it's a developer-facing feature, so
Expand Down