Skip to content

Commit

Permalink
ui: Provide [StreamColorSwatch]es via DesignVariables, not api/model/
Browse files Browse the repository at this point in the history
Instead of putting them in Subscription model objects.

This helps toward zulip#95 by bundling this with our other
ThemeExtensions that will switch together between light and dark
variants when the theme changes. It's also just nicer to separate
this very UI-focused code out of api/model/ (zulip#393).

Not marking [nfc] just because of differences in caching logic,
although I don't expect a user-visible perf effect.

Related: zulip#95
Fixes: zulip#393
  • Loading branch information
chrisbobbe committed Jun 18, 2024
1 parent f024144 commit acb9654
Show file tree
Hide file tree
Showing 12 changed files with 267 additions and 51 deletions.
24 changes: 3 additions & 21 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import 'package:flutter/foundation.dart';
import 'package:json_annotation/json_annotation.dart';

import '../../widgets/stream_colors.dart';
import 'events.dart';
import 'initial_snapshot.dart';
import 'reaction.dart';
Expand Down Expand Up @@ -403,29 +401,13 @@ class Subscription extends ZulipStream {
/// As an int that dart:ui's Color constructor will take:
/// <https://api.flutter.dev/flutter/dart-ui/Color/Color.html>
@JsonKey(readValue: _readColor)
int get color => _color;
int _color;
set color(int value) {
_color = value;
_swatch = null;
}
int color;
static Object? _readColor(Map<dynamic, dynamic> json, String key) {
final str = (json[key] as String);
assert(RegExp(r'^#[0-9a-f]{6}$').hasMatch(str));
return 0xff000000 | int.parse(str.substring(1), radix: 16);
}

StreamColorSwatch? _swatch;
/// A [StreamColorSwatch] 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.light(color);

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

Subscription({
required super.streamId,
required super.name,
Expand All @@ -447,8 +429,8 @@ class Subscription extends ZulipStream {
required this.audibleNotifications,
required this.pinToTop,
required this.isMuted,
required int color,
}) : _color = color;
required this.color,
});

factory Subscription.fromJson(Map<String, dynamic> json) =>
_$SubscriptionFromJson(json);
Expand Down
14 changes: 9 additions & 5 deletions lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import 'page.dart';
import 'sticky_header.dart';
import 'store.dart';
import 'text.dart';
import 'theme.dart';
import 'unread_count_badge.dart';

class InboxPage extends StatefulWidget {
Expand Down Expand Up @@ -423,12 +424,14 @@ class _StreamHeaderItem extends _HeaderItem {

@override String get title => subscription.name;
@override IconData get icon => iconDataForStream(subscription);
@override Color collapsedIconColor(context) => subscription.colorSwatch().iconOnPlainBackground;
@override Color uncollapsedIconColor(context) => subscription.colorSwatch().iconOnBarBackground;
@override Color collapsedIconColor(context) =>
colorSwatchFor(context, subscription).iconOnPlainBackground;
@override Color uncollapsedIconColor(context) =>
colorSwatchFor(context, subscription).iconOnBarBackground;
@override Color uncollapsedBackgroundColor(context) =>
subscription.colorSwatch().barBackground;
colorSwatchFor(context, subscription).barBackground;
@override Color? unreadCountBadgeBackgroundColor(context) =>
subscription.colorSwatch().unreadCountBadgeBackground;
colorSwatchFor(context, subscription).unreadCountBadgeBackground;

@override Future<void> onCollapseButtonTap() async {
await super.onCollapseButtonTap();
Expand Down Expand Up @@ -525,7 +528,8 @@ class _TopicItem extends StatelessWidget {
const SizedBox(width: 12),
if (hasMention) const _AtMentionMarker(),
Padding(padding: const EdgeInsetsDirectional.only(end: 16),
child: UnreadCountBadge(backgroundColor: subscription.colorSwatch(),
child: UnreadCountBadge(
backgroundColor: colorSwatchFor(context, subscription),
count: count)),
]))));
}
Expand Down
8 changes: 5 additions & 3 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import 'profile.dart';
import 'sticky_header.dart';
import 'store.dart';
import 'text.dart';
import 'theme.dart';

class MessageListPage extends StatefulWidget {
const MessageListPage({super.key, required this.narrow});
Expand Down Expand Up @@ -67,8 +68,9 @@ class _MessageListPageState extends State<MessageListPage> {
case StreamNarrow(:final streamId):
case TopicNarrow(:final streamId):
final subscription = store.subscriptions[streamId];
appBarBackgroundColor = subscription?.colorSwatch().barBackground
?? _kUnsubscribedStreamRecipientHeaderColor;
appBarBackgroundColor = subscription != null
? colorSwatchFor(context, subscription).barBackground
: _kUnsubscribedStreamRecipientHeaderColor;
// All recipient headers will match this color; remove distracting line
// (but are recipient headers even needed for topic narrows?)
removeAppBarBottomBorder = true;
Expand Down Expand Up @@ -655,7 +657,7 @@ class StreamMessageRecipientHeader extends StatelessWidget {
final Color backgroundColor;
final Color iconColor;
if (subscription != null) {
final swatch = subscription.colorSwatch();
final swatch = colorSwatchFor(context, subscription);
backgroundColor = swatch.barBackground;
iconColor = swatch.iconOnBarBackground;
} else {
Expand Down
95 changes: 95 additions & 0 deletions lib/widgets/stream_colors.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,101 @@ import 'package:flutter_color_models/flutter_color_models.dart';
import '../api/model/model.dart';
import 'color.dart';

/// A caching factory for [StreamColorSwatch]es, from [subscription.color] bases.
///
/// See subclasses:
/// [StreamColorSwatchesLight]
/// [StreamColorSwatchesDark]
abstract class StreamColorSwatches {
/// Gives the cached [StreamColorSwatch] for a [subscription.color].
///
/// For caching details, see subclasses.
StreamColorSwatch forBaseColor(int base);

/// Gives a [StreamColorSwatches], lerped to [other] at [t].
///
/// If [this] and [other] are [identical], returns [this].
///
/// Else returns an instance whose [forBaseColor] will call
/// [this.forBaseColor] and [other.forBaseColor]
/// and return [StreamColorSwatch.lerp]'s result on those.
/// This computation is cached on the instance
/// in order to save work building [t]'s animation frame when there are
/// multiple UI elements using the same [subscription.color].
StreamColorSwatches lerp(StreamColorSwatches? other, double t) {
// This short-circuit helps when [a] and [b]
// are both [StreamColorSwatchesLight.instance]
// or both [StreamColorSwatchesDark.instance].
if (identical(this, other)) {
return this;
}

return _StreamColorSwatchesLerped(this, other, t);
}
}

/// The [StreamColorSwatches] for the light theme.
class StreamColorSwatchesLight extends StreamColorSwatches {
// No public constructor; always use [instance]. Empirically,
// [StreamColorSwatches.lerp] is called even when the theme has not changed,
// and the short-circuit on [identical] cuts redundant work when that happens.
static StreamColorSwatchesLight get instance =>
(_instance ??= StreamColorSwatchesLight._());
static StreamColorSwatchesLight? _instance;

StreamColorSwatchesLight._();

final Map<int, StreamColorSwatch> _cache = {};

/// Gives the cached [StreamColorSwatch.light] for a [subscription.color].
///
/// When this is first called, the result is added to a global cache
/// keyed by [subscription.color].
/// Subsequent calls return the value from the cache.
@override
StreamColorSwatch forBaseColor(int base) =>
_cache[base] ??= StreamColorSwatch.light(base);
}

/// The [StreamColorSwatches] for the dark theme.
class StreamColorSwatchesDark extends StreamColorSwatches {
// No public constructor, like [StreamColorSwatchesLight].
static StreamColorSwatchesDark get instance =>
(_instance ??= StreamColorSwatchesDark._());
static StreamColorSwatchesDark? _instance;

StreamColorSwatchesDark._();

final Map<int, StreamColorSwatch> _cache = {};

/// Like [StreamColorSwatchesLight.forBaseColor],
/// but using [StreamColorSwatch.dark].
@override
StreamColorSwatch forBaseColor(int base) =>
_cache[base] ??= StreamColorSwatch.dark(base);
}

class _StreamColorSwatchesLerped extends StreamColorSwatches {
_StreamColorSwatchesLerped(this.a, this.b, this.t);

final StreamColorSwatches a;
final StreamColorSwatches? b;
final double t;

final Map<int, StreamColorSwatch> _cache = {};

/// Gives the cached [StreamColorSwatch] for a [subscription.color].
///
/// Caching is per-instance, not global, in order to save work building
/// [t]'s animation frame when there are multiple UI elements using the same
/// [subscription.color].
@override
StreamColorSwatch forBaseColor(int base) =>
_cache[base]
??= StreamColorSwatch.lerp(a.forBaseColor(base), b?.forBaseColor(base), t)!;
}


/// A [ColorSwatch] with colors related to a base stream color.
///
/// Use this in UI code for colors related to [Subscription.color],
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 @@ -9,6 +9,7 @@ import 'message_list.dart';
import 'page.dart';
import 'store.dart';
import 'text.dart';
import 'theme.dart';
import 'unread_count_badge.dart';

/// Scrollable listing of subscribed streams.
Expand Down Expand Up @@ -199,7 +200,7 @@ class SubscriptionItem extends StatelessWidget {

@override
Widget build(BuildContext context) {
final swatch = subscription.colorSwatch();
final swatch = colorSwatchFor(context, subscription);
final hasUnreads = (unreadCount > 0);
return Material(
// TODO(#95) need dark-theme color
Expand Down
20 changes: 19 additions & 1 deletion lib/widgets/theme.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import 'package:flutter/material.dart';

import '../api/model/model.dart';
import 'content.dart';
import 'stream_colors.dart';
import 'text.dart';

ThemeData zulipThemeData(BuildContext context) {
Expand Down Expand Up @@ -78,14 +80,16 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
bgTopBar = const Color(0xfff5f5f5),
borderBar = const Color(0x33000000),
icon = const Color(0xff666699),
title = const Color(0xff1a1a1a);
title = const Color(0xff1a1a1a),
streamColorSwatches = StreamColorSwatchesLight.instance;

DesignVariables._({
required this.bgMain,
required this.bgTopBar,
required this.borderBar,
required this.icon,
required this.title,
required this.streamColorSwatches,
});

/// The [DesignVariables] from the context's active theme.
Expand All @@ -104,20 +108,25 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
final Color icon;
final Color title;

// Not exactly from the Figma design, but from Vlad anyway.
final StreamColorSwatches streamColorSwatches;

@override
DesignVariables copyWith({
Color? bgMain,
Color? bgTopBar,
Color? borderBar,
Color? icon,
Color? title,
StreamColorSwatches? streamColorSwatches,
}) {
return DesignVariables._(
bgMain: bgMain ?? this.bgMain,
bgTopBar: bgTopBar ?? this.bgTopBar,
borderBar: borderBar ?? this.borderBar,
icon: icon ?? this.icon,
title: title ?? this.title,
streamColorSwatches: streamColorSwatches ?? this.streamColorSwatches,
);
}

Expand All @@ -132,6 +141,15 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
borderBar: Color.lerp(borderBar, other?.borderBar, t)!,
icon: Color.lerp(icon, other?.icon, t)!,
title: Color.lerp(title, other?.title, t)!,
streamColorSwatches: streamColorSwatches.lerp(other?.streamColorSwatches, t),
);
}
}

/// The theme-appropriate [StreamColorSwatch] based on [subscription.color].
///
/// For how this value is cached, see [StreamColorSwatches.forBaseColor].
StreamColorSwatch colorSwatchFor(BuildContext context, Subscription subscription) {
return DesignVariables.of(context)
.streamColorSwatches.forBaseColor(subscription.color);
}
13 changes: 0 additions & 13 deletions test/api/model/model_test.dart
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import 'dart:convert';
import 'dart:ui';

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

import '../../example_data.dart' as eg;
import '../../stdlib_checks.dart';
import '../../widgets/stream_colors_checks.dart';
import 'model_checks.dart';

void main() {
Expand Down Expand Up @@ -117,17 +115,6 @@ void main() {
check(subWithColor('#ffffff').color).equals(0xffffffff);
check(subWithColor('#000000').color).equals(0xff000000);
});

test('colorSwatch caching', () {
final sub = eg.subscription(eg.stream(), color: 0xffffffff);
check(sub.debugCachedSwatchValue).isNull();
sub.colorSwatch();
check(sub.debugCachedSwatchValue).isNotNull().base.equals(const Color(0xffffffff));
sub.color = 0xffff0000;
check(sub.debugCachedSwatchValue).isNull();
sub.colorSwatch();
check(sub.debugCachedSwatchValue).isNotNull().base.equals(const Color(0xffff0000));
});
});

group('Message', () {
Expand Down
8 changes: 5 additions & 3 deletions test/widgets/inbox_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,10 @@ 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(
StreamColorSwatch.light(subscription.color).iconOnBarBackground);
check(streamHeaderBackgroundColor(tester, streamId))
.isNotNull().equals(subscription.colorSwatch().barBackground);
.isNotNull().equals(StreamColorSwatch.light(subscription.color).barBackground);
check(tester.widgetList(findSectionContent)).isNotEmpty();
}

Expand All @@ -445,7 +446,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(
StreamColorSwatch.light(subscription.color).iconOnPlainBackground);
check(streamHeaderBackgroundColor(tester, streamId))
.isNotNull().equals(Colors.white);
check(tester.widgetList(findSectionContent)).isEmpty();
Expand Down
5 changes: 3 additions & 2 deletions test/widgets/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import 'package:zulip/widgets/content.dart';
import 'package:zulip/widgets/icons.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/store.dart';
import 'package:zulip/widgets/stream_colors.dart';
import 'package:zulip/widgets/theme.dart';

import '../api/fake_api.dart';
Expand Down Expand Up @@ -277,7 +278,7 @@ void main() {

testWidgets('color of recipient header background', (tester) async {
final subscription = eg.subscription(stream, color: Colors.red.value);
final swatch = subscription.colorSwatch();
final swatch = StreamColorSwatch.light(subscription.color);
await setupMessageListPage(tester,
messages: [eg.streamMessage(stream: subscription)],
subscriptions: [subscription]);
Expand All @@ -292,7 +293,7 @@ void main() {
testWidgets('color of stream icon', (tester) async {
final stream = eg.stream(isWebPublic: true);
final subscription = eg.subscription(stream, color: Colors.red.value);
final swatch = subscription.colorSwatch();
final swatch = StreamColorSwatch.light(subscription.color);
await setupMessageListPage(tester,
messages: [eg.streamMessage(stream: subscription)],
subscriptions: [subscription]);
Expand Down
Loading

0 comments on commit acb9654

Please sign in to comment.