From 840e52d60f17e497efb9d0139d9ef0d04f634919 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 2 Oct 2025 15:01:26 -0700 Subject: [PATCH 1/4] inbox: Fix bug where channel unread badge text color was slightly wrong UnreadCountBadge has a needlessly loose API for specifying a background color, with three options: 1. Channel-colorized: callers are supposed to pass a ChannelColorSwatch and the implementation picks `unreadCountBadgeBackground` off of that. 2. Any other Color. (ChannelColorSwatch is a Color.) 3. A default color, chosen by the implementation, if `null` is passed. We don't actually have any use for option 2, at least currently, so we'll remove it next. The buggy caller here was using option 2 when it should have used option 1: it wanted a channel-colored background, but it itself picked `unreadCountBadgeBackground` off of the swatch instead of passing the whole swatch to UnreadCountBadge for it to do that. That would have been fine except that the channel-colorized case has its own associated *text* color, and that's conditioned on whether the passed background color is a ChannelColorSwatch. --- lib/widgets/inbox.dart | 2 +- test/widgets/inbox_test.dart | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 341d75bd0e..517a06f2ec 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -463,7 +463,7 @@ class _StreamHeaderItem extends _HeaderItem with _LongPressable { @override Color uncollapsedBackgroundColor(context) => colorSwatchFor(context, subscription).barBackground; @override Color? unreadCountBadgeBackgroundColor(context) => - colorSwatchFor(context, subscription).unreadCountBadgeBackground; + colorSwatchFor(context, subscription); @override Future onCollapseButtonTap() async { await super.onCollapseButtonTap(); diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index fe96b427ab..63aad6292c 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -9,6 +9,7 @@ import 'package:zulip/widgets/color.dart'; import 'package:zulip/widgets/home.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/channel_colors.dart'; +import 'package:zulip/widgets/theme.dart'; import 'package:zulip/widgets/unread_count_badge.dart'; import '../example_data.dart' as eg; @@ -206,6 +207,28 @@ void main() { await setupVarious(tester); }); + testWidgets('UnreadCountBadge text color for a channel', (tester) async { + // Regression test for a bug where + // DesignVariables.labelCounterUnread was used for the text instead of + // DesignVariables.unreadCountBadgeTextForChannel. + final channel = eg.stream(); + final subscription = eg.subscription(channel); + await setupPage(tester, + streams: [channel], + subscriptions: [subscription], + unreadMessages: generateStreamMessages(stream: channel, count: 1, flags: [])); + + final text = tester.widget( + find.descendant( + of: find.byWidget(findRowByLabel(tester, channel.name)!), + matching: find.descendant( + of: find.byType(UnreadCountBadge), + matching: find.text('1')))); + + final expectedTextColor = DesignVariables.light.unreadCountBadgeTextForChannel; + check(text).style.isNotNull().color.isNotNull().isSameColorAs(expectedTextColor); + }); + // TODO test that tapping a conversation row opens the message list // for the conversation From 22b03d52c5f01dcb105c71d67d538cccdbd02fe1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 2 Oct 2025 13:44:39 -0700 Subject: [PATCH 2/4] unread_count_badge [nfc]: Remove extra API for specific background color Previously, this widget had three options for the background color: 1. Colorized for a channel: callers could pass a ChannelColorSwatch, and the implementation would pick `unreadCountBadgeBackground` off of that. 2. Any `Color`. 3. A default, chosen by the implementation. One caller was using option 2 when option 1 would be a better fit: it wanted a channel-colored background, but it was picking `unreadCountBadgeBackground` itself from the ChannelColorSwatch instead of letting the implementation do that. After changing that caller to just pass the ChannelColorSwatch (option 1), no more callers use option 2, so we remove it. Now UnreadCountBadge has a tighter interface. Next we'll encapsulate finding the channel color swatch itself, so callers only need to have a channel ID if they want a channel-colored background. --- lib/widgets/inbox.dart | 7 ++++--- lib/widgets/unread_count_badge.dart | 12 +++++------- test/widgets/unread_count_badge_test.dart | 7 +------ 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 517a06f2ec..ef50ab6d8e 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -6,6 +6,7 @@ import '../model/narrow.dart'; import '../model/recent_dm_conversations.dart'; import '../model/unreads.dart'; import 'action_sheet.dart'; +import 'channel_colors.dart'; import 'icons.dart'; import 'message_list.dart'; import 'page.dart'; @@ -249,7 +250,7 @@ abstract class _HeaderItem extends StatelessWidget { Color collapsedIconColor(BuildContext context); Color uncollapsedIconColor(BuildContext context); Color uncollapsedBackgroundColor(BuildContext context); - Color? unreadCountBadgeBackgroundColor(BuildContext context); + ChannelColorSwatch? unreadCountBadgeBackgroundColor(BuildContext context); Future onCollapseButtonTap() async { if (!collapsed) { @@ -332,7 +333,7 @@ class _AllDmsHeaderItem extends _HeaderItem { @override Color uncollapsedIconColor(context) => DesignVariables.of(context).labelMenuButton; @override Color uncollapsedBackgroundColor(context) => DesignVariables.of(context).dmHeaderBg; - @override Color? unreadCountBadgeBackgroundColor(context) => null; + @override ChannelColorSwatch? unreadCountBadgeBackgroundColor(context) => null; @override Future onCollapseButtonTap() async { await super.onCollapseButtonTap(); @@ -462,7 +463,7 @@ class _StreamHeaderItem extends _HeaderItem with _LongPressable { colorSwatchFor(context, subscription).iconOnBarBackground; @override Color uncollapsedBackgroundColor(context) => colorSwatchFor(context, subscription).barBackground; - @override Color? unreadCountBadgeBackgroundColor(context) => + @override ChannelColorSwatch? unreadCountBadgeBackgroundColor(context) => colorSwatchFor(context, subscription); @override Future onCollapseButtonTap() async { diff --git a/lib/widgets/unread_count_badge.dart b/lib/widgets/unread_count_badge.dart index 7d2cdc1f27..ccfe2f164b 100644 --- a/lib/widgets/unread_count_badge.dart +++ b/lib/widgets/unread_count_badge.dart @@ -19,13 +19,12 @@ class UnreadCountBadge extends StatelessWidget { final int count; final bool bold; - /// The badge's background color. + /// An optional [ChannelColorSwatch], to override the default color scheme + /// with a channel-colorized one. /// - /// Pass a [ChannelColorSwatch] if this badge represents messages in one - /// specific stream. The appropriate color from the swatch will be used. - /// - /// If null, the default neutral background will be used. - final Color? backgroundColor; + /// Pass this if the badge represents messages in one specific stream. + /// The appropriate color from the swatch will be used. + final ChannelColorSwatch? backgroundColor; @override Widget build(BuildContext context) { @@ -33,7 +32,6 @@ class UnreadCountBadge extends StatelessWidget { final effectiveBackgroundColor = switch (backgroundColor) { ChannelColorSwatch(unreadCountBadgeBackground: var color) => color, - Color() => backgroundColor, null => designVariables.bgCounterUnread, }; diff --git a/test/widgets/unread_count_badge_test.dart b/test/widgets/unread_count_badge_test.dart index 5c07f9524a..b7babca1c4 100644 --- a/test/widgets/unread_count_badge_test.dart +++ b/test/widgets/unread_count_badge_test.dart @@ -22,7 +22,7 @@ void main() { }); group('background', () { - Future prepare(WidgetTester tester, Color? backgroundColor) async { + Future prepare(WidgetTester tester, ChannelColorSwatch? backgroundColor) async { addTearDown(testBinding.reset); await tester.pumpWidget(TestZulipApp( child: UnreadCountBadge(count: 1, backgroundColor: backgroundColor))); @@ -40,11 +40,6 @@ void main() { check(findBackgroundColor(tester)).isNotNull().isSameColorAs(const Color(0x26666699)); }); - testWidgets('specified color', (tester) async { - await prepare(tester, Colors.pink); - check(findBackgroundColor(tester)).isNotNull().isSameColorAs(Colors.pink); - }); - testWidgets('stream color', (tester) async { final swatch = ChannelColorSwatch.light(0xff76ce90); await prepare(tester, swatch); From b977d674090cebc2b4e93647a1c6a269e204dd10 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 2 Oct 2025 14:32:39 -0700 Subject: [PATCH 3/4] unread_count_badge test [nfc]: Pull out prepare function --- test/widgets/unread_count_badge_test.dart | 26 ++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/test/widgets/unread_count_badge_test.dart b/test/widgets/unread_count_badge_test.dart index b7babca1c4..9441b15325 100644 --- a/test/widgets/unread_count_badge_test.dart +++ b/test/widgets/unread_count_badge_test.dart @@ -13,22 +13,22 @@ void main() { TestZulipBinding.ensureInitialized(); group('UnreadCountBadge', () { - testWidgets('smoke test; no crash', (tester) async { + Future prepare(WidgetTester tester, { + required Widget child, + }) async { addTearDown(testBinding.reset); - await tester.pumpWidget(const TestZulipApp( - child: UnreadCountBadge(count: 1, backgroundColor: null))); + await tester.pumpWidget(TestZulipApp( + child: child)); await tester.pump(); + } + + testWidgets('smoke test; no crash', (tester) async { + await prepare(tester, + child: UnreadCountBadge(count: 1, backgroundColor: null)); tester.widget(find.text("1")); }); group('background', () { - Future prepare(WidgetTester tester, ChannelColorSwatch? backgroundColor) async { - addTearDown(testBinding.reset); - await tester.pumpWidget(TestZulipApp( - child: UnreadCountBadge(count: 1, backgroundColor: backgroundColor))); - await tester.pump(); - } - Color? findBackgroundColor(WidgetTester tester) { final widget = tester.widget(find.byType(DecoratedBox)); final decoration = widget.decoration as BoxDecoration; @@ -36,13 +36,15 @@ void main() { } testWidgets('default color', (tester) async { - await prepare(tester, null); + await prepare(tester, + child: UnreadCountBadge(count: 1, backgroundColor: null)); check(findBackgroundColor(tester)).isNotNull().isSameColorAs(const Color(0x26666699)); }); testWidgets('stream color', (tester) async { final swatch = ChannelColorSwatch.light(0xff76ce90); - await prepare(tester, swatch); + await prepare(tester, + child: UnreadCountBadge(count: 1, backgroundColor: swatch)); check(findBackgroundColor(tester)).isNotNull().isSameColorAs(swatch.unreadCountBadgeBackground); }); }); From 3dfb29f3b7cf66d3c719d0fd44393e4f83017de2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 2 Oct 2025 14:30:25 -0700 Subject: [PATCH 4/4] unread_count_badge [nfc]: Encapsulate finding ChannelColorSwatch from ID --- lib/widgets/inbox.dart | 17 +++++------ lib/widgets/recent_dm_conversations.dart | 2 +- lib/widgets/subscription_list.dart | 2 +- lib/widgets/unread_count_badge.dart | 37 ++++++++++++++--------- test/widgets/checks.dart | 2 +- test/widgets/subscription_list_test.dart | 9 ++++-- test/widgets/unread_count_badge_test.dart | 26 +++++++++++++--- 7 files changed, 61 insertions(+), 34 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index ef50ab6d8e..24832480c0 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -6,7 +6,6 @@ import '../model/narrow.dart'; import '../model/recent_dm_conversations.dart'; import '../model/unreads.dart'; import 'action_sheet.dart'; -import 'channel_colors.dart'; import 'icons.dart'; import 'message_list.dart'; import 'page.dart'; @@ -250,7 +249,9 @@ abstract class _HeaderItem extends StatelessWidget { Color collapsedIconColor(BuildContext context); Color uncollapsedIconColor(BuildContext context); Color uncollapsedBackgroundColor(BuildContext context); - ChannelColorSwatch? unreadCountBadgeBackgroundColor(BuildContext context); + + /// A channel ID, if this represents a channel, else null. + int? get channelId; Future onCollapseButtonTap() async { if (!collapsed) { @@ -308,7 +309,7 @@ abstract class _HeaderItem extends StatelessWidget { if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign), Padding(padding: const EdgeInsetsDirectional.only(end: 16), child: UnreadCountBadge( - backgroundColor: unreadCountBadgeBackgroundColor(context), + channelIdForBackground: channelId, bold: true, count: count)), ]))); @@ -333,7 +334,7 @@ class _AllDmsHeaderItem extends _HeaderItem { @override Color uncollapsedIconColor(context) => DesignVariables.of(context).labelMenuButton; @override Color uncollapsedBackgroundColor(context) => DesignVariables.of(context).dmHeaderBg; - @override ChannelColorSwatch? unreadCountBadgeBackgroundColor(context) => null; + @override int? get channelId => null; @override Future onCollapseButtonTap() async { await super.onCollapseButtonTap(); @@ -430,7 +431,7 @@ class _DmItem extends StatelessWidget { const SizedBox(width: 12), if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign), Padding(padding: const EdgeInsetsDirectional.only(end: 16), - child: UnreadCountBadge(backgroundColor: null, + child: UnreadCountBadge(channelIdForBackground: null, count: count)), ])))); } @@ -463,8 +464,7 @@ class _StreamHeaderItem extends _HeaderItem with _LongPressable { colorSwatchFor(context, subscription).iconOnBarBackground; @override Color uncollapsedBackgroundColor(context) => colorSwatchFor(context, subscription).barBackground; - @override ChannelColorSwatch? unreadCountBadgeBackgroundColor(context) => - colorSwatchFor(context, subscription); + @override int? get channelId => subscription.streamId; @override Future onCollapseButtonTap() async { await super.onCollapseButtonTap(); @@ -527,7 +527,6 @@ class _TopicItem extends StatelessWidget { :topic, :count, :hasMention, :lastUnreadId) = data; final store = PerAccountStoreWidget.of(context); - final subscription = store.subscriptions[streamId]!; final designVariables = DesignVariables.of(context); final visibilityIcon = iconDataForTopicVisibilityPolicy( @@ -567,7 +566,7 @@ class _TopicItem extends StatelessWidget { if (visibilityIcon != null) _IconMarker(icon: visibilityIcon), Padding(padding: const EdgeInsetsDirectional.only(end: 16), child: UnreadCountBadge( - backgroundColor: colorSwatchFor(context, subscription), + channelIdForBackground: streamId, count: count)), ])))); } diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index f9eb688a10..719568e73d 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -233,7 +233,7 @@ class RecentDmConversationsItem extends StatelessWidget { const SizedBox(width: 12), unreadCount > 0 ? Padding(padding: const EdgeInsetsDirectional.only(end: 16), - child: UnreadCountBadge(backgroundColor: null, + child: UnreadCountBadge(channelIdForBackground: null, count: unreadCount)) : const SizedBox(), ])))); diff --git a/lib/widgets/subscription_list.dart b/lib/widgets/subscription_list.dart index ad286cf71f..8337bda7a4 100644 --- a/lib/widgets/subscription_list.dart +++ b/lib/widgets/subscription_list.dart @@ -337,7 +337,7 @@ class SubscriptionItem extends StatelessWidget { opacity: opacity, child: UnreadCountBadge( count: unreadCount, - backgroundColor: swatch, + channelIdForBackground: subscription.streamId, bold: true)), ] else if (showMutedUnreadBadge) ...[ const SizedBox(width: 12), diff --git a/lib/widgets/unread_count_badge.dart b/lib/widgets/unread_count_badge.dart index ccfe2f164b..b14bd6972d 100644 --- a/lib/widgets/unread_count_badge.dart +++ b/lib/widgets/unread_count_badge.dart @@ -1,6 +1,6 @@ import 'package:flutter/widgets.dart'; -import 'channel_colors.dart'; +import 'store.dart'; import 'text.dart'; import 'theme.dart'; @@ -12,33 +12,42 @@ class UnreadCountBadge extends StatelessWidget { const UnreadCountBadge({ super.key, required this.count, - required this.backgroundColor, + required this.channelIdForBackground, this.bold = false, }); final int count; final bool bold; - /// An optional [ChannelColorSwatch], to override the default color scheme - /// with a channel-colorized one. + /// An optional [Subscription.streamId], for a channel-colorized background. /// - /// Pass this if the badge represents messages in one specific stream. - /// The appropriate color from the swatch will be used. - final ChannelColorSwatch? backgroundColor; + /// Useful when this badge represents messages in one specific channel. + /// + /// If null, the default neutral background will be used. + final int? channelIdForBackground; @override Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); final designVariables = DesignVariables.of(context); - final effectiveBackgroundColor = switch (backgroundColor) { - ChannelColorSwatch(unreadCountBadgeBackground: var color) => color, - null => designVariables.bgCounterUnread, - }; + final Color textColor; + final Color backgroundColor; + if (channelIdForBackground != null) { + textColor = designVariables.unreadCountBadgeTextForChannel; + + final subscription = store.subscriptions[channelIdForBackground!]; + final swatch = colorSwatchFor(context, subscription); + backgroundColor = swatch.unreadCountBadgeBackground; + } else { + textColor = designVariables.labelCounterUnread; + backgroundColor = designVariables.bgCounterUnread; + } return DecoratedBox( decoration: BoxDecoration( borderRadius: BorderRadius.circular(3), - color: effectiveBackgroundColor, + color: backgroundColor, ), child: Padding( padding: const EdgeInsets.fromLTRB(4, 0, 4, 1), @@ -47,9 +56,7 @@ class UnreadCountBadge extends StatelessWidget { fontSize: 16, height: (18 / 16), fontFeatures: const [FontFeature.enable('smcp')], // small caps - color: backgroundColor is ChannelColorSwatch - ? designVariables.unreadCountBadgeTextForChannel - : designVariables.labelCounterUnread, + color: textColor, ).merge(weightVariableTextStyle(context, wght: bold ? 600 : null)), count.toString()))); diff --git a/test/widgets/checks.dart b/test/widgets/checks.dart index 856b3ebebe..90b640bd8e 100644 --- a/test/widgets/checks.dart +++ b/test/widgets/checks.dart @@ -95,7 +95,7 @@ extension PerAccountStoreWidgetChecks on Subject { extension UnreadCountBadgeChecks on Subject { Subject get count => has((b) => b.count, 'count'); Subject get bold => has((b) => b.bold, 'bold'); - Subject get backgroundColor => has((b) => b.backgroundColor, 'backgroundColor'); + Subject get channelIdForBackground => has((b) => b.channelIdForBackground, 'channelIdForBackground'); } extension UnicodeEmojiWidgetChecks on Subject { diff --git a/test/widgets/subscription_list_test.dart b/test/widgets/subscription_list_test.dart index d0eb50624d..49034f2166 100644 --- a/test/widgets/subscription_list_test.dart +++ b/test/widgets/subscription_list_test.dart @@ -2,6 +2,7 @@ import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:legacy_checks/legacy_checks.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/widgets/color.dart'; @@ -273,8 +274,12 @@ void main() { check(getItemCount()).equals(1); check(tester.widget(find.byIcon(iconDataForStream(stream))).color) .isNotNull().isSameColorAs(swatch.iconOnPlainBackground); - check(tester.widget(find.byType(UnreadCountBadge)).backgroundColor) - .isNotNull().isSameColorAs(swatch); + + final unreadCountBadgeRenderBox = tester.renderObject(find.byType(UnreadCountBadge)); + check(unreadCountBadgeRenderBox).legacyMatcher( + // `paints` isn't a [Matcher] so we wrap it with `equals`; + // awkward but it works + equals(paints..rrect(color: swatch.unreadCountBadgeBackground))); }); testWidgets('muted streams are displayed as faded', (tester) async { diff --git a/test/widgets/unread_count_badge_test.dart b/test/widgets/unread_count_badge_test.dart index 9441b15325..739146050a 100644 --- a/test/widgets/unread_count_badge_test.dart +++ b/test/widgets/unread_count_badge_test.dart @@ -3,10 +3,13 @@ import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/model.dart'; import 'package:zulip/widgets/channel_colors.dart'; import 'package:zulip/widgets/unread_count_badge.dart'; +import '../example_data.dart' as eg; import '../model/binding.dart'; +import '../model/test_store.dart'; import 'test_app.dart'; void main() { @@ -15,16 +18,25 @@ void main() { group('UnreadCountBadge', () { Future prepare(WidgetTester tester, { required Widget child, + Subscription? subscription, }) async { addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + if (subscription != null) { + await store.addStream(ZulipStream.fromSubscription(subscription)); + await store.addSubscription(subscription); + } await tester.pumpWidget(TestZulipApp( + accountId: eg.selfAccount.id, child: child)); await tester.pump(); + await tester.pump(); } testWidgets('smoke test; no crash', (tester) async { await prepare(tester, - child: UnreadCountBadge(count: 1, backgroundColor: null)); + child: const UnreadCountBadge(count: 1, channelIdForBackground: null)); tester.widget(find.text("1")); }); @@ -37,15 +49,19 @@ void main() { testWidgets('default color', (tester) async { await prepare(tester, - child: UnreadCountBadge(count: 1, backgroundColor: null)); + child: UnreadCountBadge(count: 1, channelIdForBackground: null)); check(findBackgroundColor(tester)).isNotNull().isSameColorAs(const Color(0x26666699)); }); testWidgets('stream color', (tester) async { - final swatch = ChannelColorSwatch.light(0xff76ce90); + final subscription = eg.subscription(eg.stream(), color: 0xff76ce90); await prepare(tester, - child: UnreadCountBadge(count: 1, backgroundColor: swatch)); - check(findBackgroundColor(tester)).isNotNull().isSameColorAs(swatch.unreadCountBadgeBackground); + subscription: subscription, + child: UnreadCountBadge( + count: 1, + channelIdForBackground: subscription.streamId)); + check(findBackgroundColor(tester)).isNotNull() + .isSameColorAs(ChannelColorSwatch.light(0xff76ce90).unreadCountBadgeBackground); }); }); });