Skip to content

model [nfc]: Remove StreamColorSwatch; just use ColorSwatch<StreamColorVariant> #642

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

Closed
wants to merge 4 commits into from
Closed
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
128 changes: 61 additions & 67 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -416,16 +416,17 @@ class Subscription extends ZulipStream {
return 0xff000000 | int.parse(str.substring(1), radix: 16);
}

StreamColorSwatch? _swatch;
/// A [StreamColorSwatch] for the subscription, memoized.
ColorSwatch<StreamColor>? _swatch;
/// A [ColorSwatch<StreamColorVariant>] for the subscription, memoized.
// TODO I'm not sure this is the right home for this; it seems like we might
// instead have chosen to put it in more UI-centered code, like in a custom
// material [ColorScheme] class or something. But it works for now.
StreamColorSwatch colorSwatch() => _swatch ??= StreamColorSwatch(color);
ColorSwatch<StreamColor> colorSwatch() =>
_swatch ??= streamColorSwatch(color);

@visibleForTesting
@JsonKey(includeToJson: false)
StreamColorSwatch? get debugCachedSwatchValue => _swatch;
ColorSwatch<StreamColor>? get debugCachedSwatchValue => _swatch;

Subscription({
required super.streamId,
Expand Down Expand Up @@ -462,86 +463,79 @@ class Subscription extends ZulipStream {
///
/// Use this in UI code for colors related to [Subscription.color],
/// such as the background of an unread count badge.
class StreamColorSwatch extends ColorSwatch<_StreamColorVariant> {
StreamColorSwatch(int base) : super(base, _compute(base));
ColorSwatch<StreamColor> streamColorSwatch(int base) {
final baseAsColor = Color(base);

final clamped20to75 = clampLchLightness(baseAsColor, 20, 75);
final clamped20to75AsHsl = HSLColor.fromColor(clamped20to75);

final map = {
StreamColor.base: baseAsColor,

// Follows `.unread-count` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-sidebar#script.js>
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1624484>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
StreamColor.unreadCountBadgeBackground:
clampLchLightness(baseAsColor, 30, 70)
.withOpacity(0.3),

// Follows `.sidebar-row__icon` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
StreamColor.iconOnPlainBackground: clamped20to75,

// Follows `.recepeient__icon` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1624484>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
StreamColor.iconOnBarBackground:
clamped20to75AsHsl
.withLightness(clamped20to75AsHsl.lightness - 0.12)
.toColor(),

// Follows `.recepient` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
//
// TODO I think [LabColor.interpolate] doesn't actually do LAB mixing;
// it just calls up to the superclass method [ColorModel.interpolate]:
// <https://pub.dev/documentation/flutter_color_models/latest/flutter_color_models/ColorModel/interpolate.html>
// which does ordinary RGB mixing. Investigate and send a PR?
// TODO fix bug where our results differ from the replit's (see unit tests)
StreamColor.barBackground:
LabColor.fromColor(const Color(0xfff9f9f9))
.interpolate(LabColor.fromColor(clamped20to75), 0.22)
.toColor(),
};

return ColorSwatch<StreamColor>(base, map);
}

Color get base => this[_StreamColorVariant.base]!;
enum StreamColor {
/// The [Subscription.color] int that the swatch is based on.
base,

Color get unreadCountBadgeBackground => this[_StreamColorVariant.unreadCountBadgeBackground]!;
unreadCountBadgeBackground,

/// The stream icon on a plain-colored surface, such as white.
///
/// For the icon on a [barBackground]-colored surface,
/// use [iconOnBarBackground] instead.
Color get iconOnPlainBackground => this[_StreamColorVariant.iconOnPlainBackground]!;
iconOnPlainBackground,

/// The stream icon on a [barBackground]-colored surface.
///
/// For the icon on a plain surface, use [iconOnPlainBackground] instead.
/// This color is chosen to enhance contrast with [barBackground]:
/// <https://github.com/zulip/zulip/pull/27485>
Color get iconOnBarBackground => this[_StreamColorVariant.iconOnBarBackground]!;
iconOnBarBackground,

/// The background color of a bar representing a stream, like a recipient bar.
///
/// Use this in the message list, the "Inbox" view, and the "Streams" view.
Color get barBackground => this[_StreamColorVariant.barBackground]!;

static Map<_StreamColorVariant, Color> _compute(int base) {
final baseAsColor = Color(base);

final clamped20to75 = clampLchLightness(baseAsColor, 20, 75);
final clamped20to75AsHsl = HSLColor.fromColor(clamped20to75);

return {
_StreamColorVariant.base: baseAsColor,

// Follows `.unread-count` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-sidebar#script.js>
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1624484>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
_StreamColorVariant.unreadCountBadgeBackground:
clampLchLightness(baseAsColor, 30, 70)
.withOpacity(0.3),

// Follows `.sidebar-row__icon` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
_StreamColorVariant.iconOnPlainBackground: clamped20to75,

// Follows `.recepeient__icon` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1624484>
//
// TODO fix bug where our results differ from the replit's (see unit tests)
_StreamColorVariant.iconOnBarBackground:
clamped20to75AsHsl
.withLightness(clamped20to75AsHsl.lightness - 0.12)
.toColor(),

// Follows `.recepient` in Vlad's replit:
// <https://replit.com/@VladKorobov/zulip-topic-feed-colors#script.js>
//
// TODO I think [LabColor.interpolate] doesn't actually do LAB mixing;
// it just calls up to the superclass method [ColorModel.interpolate]:
// <https://pub.dev/documentation/flutter_color_models/latest/flutter_color_models/ColorModel/interpolate.html>
// which does ordinary RGB mixing. Investigate and send a PR?
// TODO fix bug where our results differ from the replit's (see unit tests)
_StreamColorVariant.barBackground:
LabColor.fromColor(const Color(0xfff9f9f9))
.interpolate(LabColor.fromColor(clamped20to75), 0.22)
.toColor(),
};
}
}

enum _StreamColorVariant {
base,
unreadCountBadgeBackground,
iconOnPlainBackground,
iconOnBarBackground,
barBackground,
}

Expand Down
10 changes: 6 additions & 4 deletions lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -410,12 +410,14 @@ class _StreamHeaderItem extends _HeaderItem {

@override get title => subscription.name;
@override get icon => iconDataForStream(subscription);
@override get collapsedIconColor => subscription.colorSwatch().iconOnPlainBackground;
@override get uncollapsedIconColor => subscription.colorSwatch().iconOnBarBackground;
@override get collapsedIconColor =>
subscription.colorSwatch()[StreamColor.iconOnPlainBackground]!;
@override get uncollapsedIconColor =>
subscription.colorSwatch()[StreamColor.iconOnBarBackground]!;
@override get uncollapsedBackgroundColor =>
subscription.colorSwatch().barBackground;
subscription.colorSwatch()[StreamColor.barBackground]!;
@override get unreadCountBadgeBackgroundColor =>
subscription.colorSwatch().unreadCountBadgeBackground;
subscription.colorSwatch()[StreamColor.unreadCountBadgeBackground]!;

@override get onCollapseButtonTap => () async {
await super.onCollapseButtonTap();
Expand Down
9 changes: 5 additions & 4 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ class _MessageListPageState extends State<MessageListPage> {

case StreamNarrow(:final streamId):
case TopicNarrow(:final streamId):
backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground
backgroundColor =
store.subscriptions[streamId]?.colorSwatch()[StreamColor.barBackground]!
?? _kUnsubscribedStreamRecipientHeaderColor;
// All recipient headers will match this color; remove distracting line
// (but are recipient headers even needed for topic narrows?)
Expand Down Expand Up @@ -655,12 +656,12 @@ class StreamMessageRecipientHeader extends StatelessWidget {
final Color iconColor;
if (subscription != null) {
final swatch = subscription.colorSwatch();
backgroundColor = swatch.barBackground;
backgroundColor = swatch[StreamColor.barBackground]!;
contrastingColor =
(ThemeData.estimateBrightnessForColor(swatch.barBackground) == Brightness.dark)
(ThemeData.estimateBrightnessForColor(backgroundColor) == Brightness.dark)
? Colors.white
: Colors.black;
iconColor = swatch.iconOnBarBackground;
iconColor = swatch[StreamColor.iconOnBarBackground]!;
} else {
backgroundColor = _kUnsubscribedStreamRecipientHeaderColor;
contrastingColor = Colors.black;
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/subscription_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ class SubscriptionItem extends StatelessWidget {
const SizedBox(width: 16),
Padding(
padding: const EdgeInsets.symmetric(vertical: 11),
child: Icon(size: 18, color: swatch.iconOnPlainBackground,
child: Icon(size: 18, color: swatch[StreamColor.iconOnPlainBackground]!,
iconDataForStream(subscription))),
const SizedBox(width: 5),
Expanded(
Expand Down
8 changes: 5 additions & 3 deletions lib/widgets/unread_count_badge.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,18 @@ class UnreadCountBadge extends StatelessWidget {

/// The badge's background color.
///
/// Pass a [StreamColorSwatch] if this badge represents messages in one
/// specific stream. The appropriate color from the swatch will be used.
/// Pass a [ColorSwatch<StreamColorVariant>] 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;

@override
Widget build(BuildContext context) {
final backgroundColor = this.backgroundColor;
final effectiveBackgroundColor = switch (backgroundColor) {
StreamColorSwatch(unreadCountBadgeBackground: var color) => color,
ColorSwatch<StreamColor>() =>
backgroundColor[StreamColor.unreadCountBadgeBackground]!,
Color() => backgroundColor,
null => const Color.fromRGBO(102, 102, 153, 0.15),
};
Expand Down
10 changes: 0 additions & 10 deletions test/api/model/model_checks.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import 'dart:ui';

import 'package:checks/checks.dart';
import 'package:zulip/api/model/model.dart';

Expand Down Expand Up @@ -29,14 +27,6 @@ extension ZulipStreamChecks on Subject<ZulipStream> {
Subject<int?> get canRemoveSubscribersGroup => has((e) => e.canRemoveSubscribersGroup, 'canRemoveSubscribersGroup');
}

extension StreamColorSwatchChecks on Subject<StreamColorSwatch> {
Subject<Color> get base => has((s) => s.base, 'base');
Subject<Color> get unreadCountBadgeBackground => has((s) => s.unreadCountBadgeBackground, 'unreadCountBadgeBackground');
Subject<Color> get iconOnPlainBackground => has((s) => s.iconOnPlainBackground, 'iconOnPlainBackground');
Subject<Color> get iconOnBarBackground => has((s) => s.iconOnBarBackground, 'iconOnBarBackground');
Subject<Color> get barBackground => has((s) => s.barBackground, 'barBackground');
}

extension MessageChecks on Subject<Message> {
Subject<int> get id => has((e) => e.id, 'id');
Subject<String> get content => has((e) => e.content, 'content');
Expand Down
24 changes: 16 additions & 8 deletions test/api/model/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:checks/checks.dart';
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/model.dart';

import '../../flutter_checks.dart';
import '../../example_data.dart' as eg;
import '../../stdlib_checks.dart';
import 'model_checks.dart';
Expand Down Expand Up @@ -120,21 +121,25 @@ void main() {
final sub = eg.subscription(eg.stream(), color: 0xffffffff);
check(sub.debugCachedSwatchValue).isNull();
sub.colorSwatch();
check(sub.debugCachedSwatchValue).isNotNull().base.equals(const Color(0xffffffff));
check(sub.debugCachedSwatchValue).isNotNull()
[StreamColor.base].equals(const Color(0xffffffff));
sub.color = 0xffff0000;
check(sub.debugCachedSwatchValue).isNull();
sub.colorSwatch();
check(sub.debugCachedSwatchValue).isNotNull().base.equals(const Color(0xffff0000));
check(sub.debugCachedSwatchValue).isNotNull()
[StreamColor.base].equals(const Color(0xffff0000));
});

group('StreamColorSwatch', () {
group('streamColorSwatch', () {
test('base', () {
check(StreamColorSwatch(0xffffffff)).base.equals(const Color(0xffffffff));
check(streamColorSwatch(0xffffffff))[StreamColor.base]
.equals(const Color(0xffffffff));
});

test('unreadCountBadgeBackground', () {
void runCheck(int base, Color expected) {
check(StreamColorSwatch(base)).unreadCountBadgeBackground.equals(expected);
check(streamColorSwatch(base))
[StreamColor.unreadCountBadgeBackground].equals(expected);
}

// Check against everything in ZULIP_ASSIGNMENT_COLORS and EXTREME_COLORS
Expand Down Expand Up @@ -196,7 +201,8 @@ void main() {

test('iconOnPlainBackground', () {
void runCheck(int base, Color expected) {
check(StreamColorSwatch(base)).iconOnPlainBackground.equals(expected);
check(streamColorSwatch(base))
[StreamColor.iconOnPlainBackground].equals(expected);
}

// Check against everything in ZULIP_ASSIGNMENT_COLORS
Expand Down Expand Up @@ -237,7 +243,8 @@ void main() {

test('iconOnBarBackground', () {
void runCheck(int base, Color expected) {
check(StreamColorSwatch(base)).iconOnBarBackground.equals(expected);
check(streamColorSwatch(base))
[StreamColor.iconOnBarBackground].equals(expected);
}

// Check against everything in ZULIP_ASSIGNMENT_COLORS
Expand Down Expand Up @@ -278,7 +285,8 @@ void main() {

test('barBackground', () {
void runCheck(int base, Color expected) {
check(StreamColorSwatch(base)).barBackground.equals(expected);
check(streamColorSwatch(base))
[StreamColor.barBackground].equals(expected);
}

// Check against everything in ZULIP_ASSIGNMENT_COLORS
Expand Down
4 changes: 4 additions & 0 deletions test/flutter_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import 'package:checks/checks.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

extension ColorSwatchChecks<T> on Subject<ColorSwatch<T>> {
Subject<Color?> operator [](T index) => has((x) => x[index], '[$index]');
}

extension RectChecks on Subject<Rect> {
Subject<double> get top => has((d) => d.top, 'top');
Subject<double> get bottom => has((d) => d.bottom, 'bottom');
Expand Down
6 changes: 4 additions & 2 deletions test/widgets/inbox_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,8 @@ void main() {
final collapseIcon = findHeaderCollapseIcon(tester, headerRow!);
check(collapseIcon).icon.equals(ZulipIcons.arrow_down);
final streamIcon = findStreamHeaderIcon(tester, streamId);
check(streamIcon).color.equals(subscription.colorSwatch().iconOnBarBackground);
check(streamIcon).color
.equals(subscription.colorSwatch()[StreamColor.iconOnBarBackground]!);
// TODO check bar background color
check(tester.widgetList(findSectionContent)).isNotEmpty();
}
Expand All @@ -432,7 +433,8 @@ void main() {
final collapseIcon = findHeaderCollapseIcon(tester, headerRow!);
check(collapseIcon).icon.equals(ZulipIcons.arrow_right);
final streamIcon = findStreamHeaderIcon(tester, streamId);
check(streamIcon).color.equals(subscription.colorSwatch().iconOnPlainBackground);
check(streamIcon).color
.equals(subscription.colorSwatch()[StreamColor.iconOnPlainBackground]!);
// TODO check bar background color
check(tester.widgetList(findSectionContent)).isEmpty();
}
Expand Down
4 changes: 2 additions & 2 deletions test/widgets/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ void main() {
find.descendant(
of: find.byType(StreamMessageRecipientHeader),
matching: find.byType(ColoredBox),
))).color.equals(swatch.barBackground);
))).color.equals(swatch[StreamColor.barBackground]!);
});

testWidgets('color of stream icon', (tester) async {
Expand All @@ -294,7 +294,7 @@ void main() {
subscriptions: [subscription]);
await tester.pump();
check(tester.widget<Icon>(find.byIcon(ZulipIcons.globe)))
.color.equals(swatch.iconOnBarBackground);
.color.equals(swatch[StreamColor.iconOnBarBackground]!);
});

testWidgets('normal streams show hash icon', (tester) async {
Expand Down
2 changes: 1 addition & 1 deletion test/widgets/subscription_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ void main() {
], unreadMsgs: unreadMsgs);
check(getItemCount()).equals(1);
check(tester.widget<Icon>(find.byType(Icon)).color)
.equals(swatch.iconOnPlainBackground);
.equals(swatch[StreamColor.iconOnPlainBackground]!);
check(tester.widget<UnreadCountBadge>(find.byType(UnreadCountBadge)).backgroundColor)
.equals(swatch);
});
Expand Down
Loading
Loading