From 38b024c21d7b3170edc1f8f84575db54f779ca51 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 17 Nov 2023 13:22:04 -0500 Subject: [PATCH 01/12] unreads [nfc]: Mention in dartdocs that message-ID lists are sorted --- lib/model/unreads.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index b9938d61bf..06d13fe3ff 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -81,10 +81,10 @@ class Unreads extends ChangeNotifier { // TODO excluded for now; would need to handle nuances around muting etc. // int count; - /// Unread stream messages, as: stream ID → topic → message ID. + /// Unread stream messages, as: stream ID → topic → message IDs (sorted). final Map>> streams; - /// Unread DM messages, as: DM narrow → message ID. + /// Unread DM messages, as: DM narrow → message IDs (sorted). final Map> dms; /// Unread messages with the self-user @-mentioned, directly or by wildcard. From e2b3b97ae88905767b604c2ce1bdc90eafd57703 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Sun, 12 Nov 2023 21:40:57 -0500 Subject: [PATCH 02/12] icons: Add arrow_down and arrow_right custom icons Taken today from the "Icons" page in Figma: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=1651%3A24320&mode=dev https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=1651%3A24321&mode=dev --- assets/icons/ZulipIcons.ttf | Bin 5364 -> 5572 bytes assets/icons/arrow_down.svg | 3 +++ assets/icons/arrow_right.svg | 3 +++ lib/widgets/icons.dart | 28 +++++++++++++++++----------- 4 files changed, 23 insertions(+), 11 deletions(-) create mode 100644 assets/icons/arrow_down.svg create mode 100644 assets/icons/arrow_right.svg diff --git a/assets/icons/ZulipIcons.ttf b/assets/icons/ZulipIcons.ttf index 6bf9215cd3239786572d5d4db859da78f65126d2..46f0735eaf4ce9d53a4986440cbf68dc3ca36fbb 100644 GIT binary patch delta 1272 zcma)5J7`l;82ZUk|h`3Z7M6mTcrx$VbUcP+y{O9|hC+ElJ#Vt(;fFQ2H z!uDfl&mGzHb$9}hy+rNQ*=;+|zdch0)Dx;M<;q^Q^8WQXz`9RdDwJl&b9=p$z`Fem za-o>_#;$%@d`|60Vt0{-cH7SKdx6+cEZ1jtsny@~?c#c_RLLFiPP{!{1KNJjaJ1~r zRArx}nLNRDcG4^7WAlf10ud%ud#aVG`tOU6X8`vN_kZ497zz!V4%o$=8!x)vzgGZRhB0h@^g*No_H;8V&4%Q<=j-dx}ss)|@*rR6OrPZcAgkFRZ zHTqs;xr(8WJ{c1;fdJY~#5AoDG^0#o)0kqAB*i*N8%qk)Ax=TYBwY(0q|TvRjA6CT z;_EQ9E;fO63>=2b64T@!#%ia=p?1J1Ty#V#{jNjLOoDE$>-Nni)9Ak; zGeC^^h>$%WVe+VtBsu3JOCIwPCFgzgk_$e1$VDGBA*gd^lv@N`pQ!FOWeudCEtIT=$_b)3gs>E;KZFpd7@nz|H0|$DpCdu^d_h z|3>-=bI5Zlr7o&xmSc@u3-+k}G|&}z^1m-*hz_LQsfRo&NKn+$(vo_(a!XjY-it$h zQt!-WNTeoLR&HwvUc73X4e>hfa}V~*fDFnII=x!0(ij=5G$w;PjB|< fT9Jl~$e2vZgcPJGbs3eCy{MQ7ne-x*5hO$C1#ikkFS^mA&duE1n6oBC^P-Eo z(@szXLH!3(AQ4`4AzgJ*&`r>V&`m*v18>W|&wBoV&iFjP=lMOq_notM;_!GK0-zlm zFmd|ICI>Dh1=ivUIExA=kio|EGy0ZF6qw~tF-Y)}qUgggrcuLF>|l?U7BS(2zqxOPi~BH=NW*{|ao%~*NnMEA2>oN!yAYtpi=eh~ z(WVP2bkM4cYmlB6Ez;<2M)h!C5ArI?5S_eElOzNBk!Xepqm3>p1`kmeSUpnkrFH|!ZpcsDO!10j-UGgeL{?*reVplY_eDw4~-AbTjQ*>I|oLeGhoJ@ zgz0rg%_--J8L02jUp?DwR_%r;+c@zZ=DN?bpV8GYW{|}a%2;c(A01IY0~<`(B1hrj|cO#&~QTn&ajR>$3I8I&f|N-|uy`W!=Y* + + diff --git a/assets/icons/arrow_right.svg b/assets/icons/arrow_right.svg new file mode 100644 index 0000000000..4cec3ed6af --- /dev/null +++ b/assets/icons/arrow_right.svg @@ -0,0 +1,3 @@ + + + diff --git a/lib/widgets/icons.dart b/lib/widgets/icons.dart index b9553c643d..84cb8a7fc1 100644 --- a/lib/widgets/icons.dart +++ b/lib/widgets/icons.dart @@ -22,38 +22,44 @@ abstract final class ZulipIcons { // // BEGIN GENERATED ICON DATA + /// The Zulip custom icon "arrow_down". + static const IconData arrow_down = IconData(0xf101, fontFamily: "Zulip Icons"); + + /// The Zulip custom icon "arrow_right". + static const IconData arrow_right = IconData(0xf102, fontFamily: "Zulip Icons"); + /// The Zulip custom icon "bot". - static const IconData bot = IconData(0xf101, fontFamily: "Zulip Icons"); + static const IconData bot = IconData(0xf103, fontFamily: "Zulip Icons"); /// The Zulip custom icon "globe". - static const IconData globe = IconData(0xf102, fontFamily: "Zulip Icons"); + static const IconData globe = IconData(0xf104, fontFamily: "Zulip Icons"); /// The Zulip custom icon "group_dm". - static const IconData group_dm = IconData(0xf103, fontFamily: "Zulip Icons"); + static const IconData group_dm = IconData(0xf105, fontFamily: "Zulip Icons"); /// The Zulip custom icon "hash_sign". - static const IconData hash_sign = IconData(0xf104, fontFamily: "Zulip Icons"); + static const IconData hash_sign = IconData(0xf106, fontFamily: "Zulip Icons"); /// The Zulip custom icon "language". - static const IconData language = IconData(0xf105, fontFamily: "Zulip Icons"); + static const IconData language = IconData(0xf107, fontFamily: "Zulip Icons"); /// The Zulip custom icon "lock". - static const IconData lock = IconData(0xf106, fontFamily: "Zulip Icons"); + static const IconData lock = IconData(0xf108, fontFamily: "Zulip Icons"); /// The Zulip custom icon "mute". - static const IconData mute = IconData(0xf107, fontFamily: "Zulip Icons"); + static const IconData mute = IconData(0xf109, fontFamily: "Zulip Icons"); /// The Zulip custom icon "read_receipts". - static const IconData read_receipts = IconData(0xf108, fontFamily: "Zulip Icons"); + static const IconData read_receipts = IconData(0xf10a, fontFamily: "Zulip Icons"); /// The Zulip custom icon "topic". - static const IconData topic = IconData(0xf109, fontFamily: "Zulip Icons"); + static const IconData topic = IconData(0xf10b, fontFamily: "Zulip Icons"); /// The Zulip custom icon "unmute". - static const IconData unmute = IconData(0xf10a, fontFamily: "Zulip Icons"); + static const IconData unmute = IconData(0xf10c, fontFamily: "Zulip Icons"); /// The Zulip custom icon "user". - static const IconData user = IconData(0xf10b, fontFamily: "Zulip Icons"); + static const IconData user = IconData(0xf10d, fontFamily: "Zulip Icons"); // END GENERATED ICON DATA } From aa9a09f7c794a07beaaf9e147345cd8bcbd6d976 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 13 Nov 2023 00:27:59 -0500 Subject: [PATCH 03/12] icons: Update `user` to match Figma "Icons" page Accessed today: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=56%3A11511&mode=dev --- assets/icons/ZulipIcons.ttf | Bin 5572 -> 5556 bytes assets/icons/user.svg | 18 +++--------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/assets/icons/ZulipIcons.ttf b/assets/icons/ZulipIcons.ttf index 46f0735eaf4ce9d53a4986440cbf68dc3ca36fbb..31f1ddd0c57ed36ff34cc6b01d4626eb01813977 100644 GIT binary patch delta 381 zcmX@2y+wP11J9P|Bq;_4b{hr;ZjXsRrpy&yN)ua@1zQ*xn9K|Ei%b5-tgc{S5ZNFhDqwHhyXbXHvU?Sj4bkkN;cZ! zK)#ulpir@(9H)?=wyCMMpb)2=pt7}&xVVlr)71oJkfE;D%F5P1tRQBVBw77qP_*&W z4i%D=6bhBnR}mIg(U*eowEb)p)dYoDIMpqjRUkq-uH3A0Oa?4MN^-2+t`V->Eb>ZV Q1upK+7V4X)2pKQ~0Mw{Z%K!iX delta 374 zcmdm@eMEbL15dI=qAvpjyA1;acf>>=Q|1d^MH5?;1*b4DFqs$R7nl5vSzW=vAaZ8n z4g;16Et`2ZzENeFfq_wpk5NR9(a4U`M2}IN zkCB~COk9ppgpHk#QI65vj@d+yNnMTA2q?tIsHDeeVmG;2P^liKPL@$b%*f0Zs28Y; zvB4_I(lV(8M7-k_V_^{z)HXHM78K%?6I8a=5f|67R%ZKmQQ2Ba$=VG>bZGh6Do83z zim2#IGKNX&tB3$O3buY)j2e8RAnjUaKml=W8zpw;)m9LLOOmYE7}W&%1m#${T_aq% zS>%<3SUJ=!T-=>4)HxXgl&oE?m6d^n(!a3D)ZIFI-x?6l0u;>V0D{YgbbJg DBhOCd diff --git a/assets/icons/user.svg b/assets/icons/user.svg index 4b6ac99e8d..7598523429 100644 --- a/assets/icons/user.svg +++ b/assets/icons/user.svg @@ -1,15 +1,3 @@ - - - - \ No newline at end of file + + + From cc75cc2acae011609917fec13a04c99c17e355e1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 7 Nov 2023 16:57:37 -0500 Subject: [PATCH 04/12] api: Bring stream-color int parsing out to the "crunchy shell" --- lib/api/model/events.dart | 7 ++++++- lib/api/model/model.dart | 11 ++++++++++- lib/api/model/model.g.dart | 2 +- lib/model/store.dart | 2 +- lib/widgets/message_list.dart | 9 +-------- test/api/model/events_checks.dart | 4 ++++ test/api/model/events_test.dart | 11 +++++++++++ test/api/model/model_test.dart | 14 ++++++++++++++ test/example_data.dart | 4 ++-- test/model/store_test.dart | 10 +++++----- 10 files changed, 55 insertions(+), 19 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index dbaf96b8a4..a9d4eaf951 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -363,7 +363,9 @@ class SubscriptionUpdateEvent extends SubscriptionEvent { final value = json['value']; switch (SubscriptionProperty.fromRawString(json['property'] as String)) { case SubscriptionProperty.color: - return value as String; + final str = value as String; + assert(RegExp(r'^#[0-9a-f]{6}$').hasMatch(str)); + return 0xff000000 | int.parse(str.substring(1), radix: 16); case SubscriptionProperty.isMuted: case SubscriptionProperty.inHomeView: case SubscriptionProperty.pinToTop: @@ -397,7 +399,10 @@ class SubscriptionUpdateEvent extends SubscriptionEvent { /// Used in handling of [SubscriptionUpdateEvent]. @JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true) enum SubscriptionProperty { + /// As an int that dart:ui's Color constructor will take: + /// color, + isMuted, inHomeView, pinToTop, diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 23edbb100a..e9276c72a0 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -325,7 +325,16 @@ class Subscription extends ZulipStream { bool isMuted; // final bool? inHomeView; // deprecated; ignore - String color; + /// As an int that dart:ui's Color constructor will take: + /// + @JsonKey(readValue: _readColor) + int color; + + static Object? _readColor(Map 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); + } Subscription({ required super.streamId, diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index ecc8b11d48..2269da8673 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -210,7 +210,7 @@ Subscription _$SubscriptionFromJson(Map json) => Subscription( audibleNotifications: json['audible_notifications'] as bool?, pinToTop: json['pin_to_top'] as bool, isMuted: json['is_muted'] as bool, - color: json['color'] as String, + color: Subscription._readColor(json, 'color') as int, ); Map _$SubscriptionToJson(Subscription instance) => diff --git a/lib/model/store.dart b/lib/model/store.dart index fc8d164808..27187dc5f5 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -323,7 +323,7 @@ class PerAccountStore extends ChangeNotifier { if (subscription == null) return; // TODO(log) switch (event.property) { case SubscriptionProperty.color: - subscription.color = event.value as String; + subscription.color = event.value as int; case SubscriptionProperty.isMuted: subscription.isMuted = event.value as bool; case SubscriptionProperty.inHomeView: diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b2d1dbf1c8..daa68cf86f 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -508,13 +508,6 @@ class _UnreadMarker extends StatelessWidget { } } -Color colorForStream(Subscription? subscription) { - final color = subscription?.color; - if (color == null) return const Color(0x00c2c2c2); - assert(RegExp(r'^#[0-9a-f]{6}$').hasMatch(color)); - return Color(0xff000000 | int.parse(color.substring(1), radix: 16)); -} - class StreamTopicRecipientHeader extends StatelessWidget { const StreamTopicRecipientHeader({super.key, required this.message}); @@ -529,7 +522,7 @@ class StreamTopicRecipientHeader extends StatelessWidget { final topic = message.subject; final subscription = store.subscriptions[message.streamId]; - final streamColor = colorForStream(subscription); + final streamColor = Color(subscription?.color ?? 0x00c2c2c2); final contrastingColor = ThemeData.estimateBrightnessForColor(streamColor) == Brightness.dark ? Colors.white diff --git a/test/api/model/events_checks.dart b/test/api/model/events_checks.dart index 038abadb0a..cffa2b4074 100644 --- a/test/api/model/events_checks.dart +++ b/test/api/model/events_checks.dart @@ -19,6 +19,10 @@ extension SubscriptionRemoveEventChecks on Subject { Subject> get streamIds => has((e) => e.streamIds, 'streamIds'); } +extension SubscriptionUpdateEventChecks on Subject { + Subject get value => has((e) => e.value, 'value'); +} + extension MessageEventChecks on Subject { Subject get message => has((e) => e.message, 'message'); } diff --git a/test/api/model/events_test.dart b/test/api/model/events_test.dart index dd2412bd84..9cb9f55818 100644 --- a/test/api/model/events_test.dart +++ b/test/api/model/events_test.dart @@ -42,6 +42,17 @@ void main() { }) as SubscriptionRemoveEvent).streamIds.jsonEquals([123, 456]); }); + test('subscription/update: convert color correctly', () { + check(Event.fromJson({ + 'id': 1, + 'type': 'subscription', + 'op': 'update', + 'stream_id': 1, + 'property': 'color', + 'value': '#123456', + }) as SubscriptionUpdateEvent).value.equals(0xff123456); + }); + test('message: move flags into message object', () { final message = eg.streamMessage(); MessageEvent mkEvent(List flags) => Event.fromJson({ diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 1bacb090a2..66711b4034 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -102,6 +102,20 @@ void main() { }); }); + group('Subscription', () { + test('converts color to int', () { + Subscription subWithColor(String color) { + return Subscription.fromJson( + deepToJson(eg.subscription(eg.stream())) as Map + ..['color'] = color, + ); + } + check(subWithColor('#e79ab5').color).equals(0xffe79ab5); + check(subWithColor('#ffffff').color).equals(0xffffffff); + check(subWithColor('#000000').color).equals(0xff000000); + }); + }); + group('Message', () { test('no crash on unrecognized flag', () { final m1 = Message.fromJson( diff --git a/test/example_data.dart b/test/example_data.dart index 9b31124609..f82bef8fd4 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -138,7 +138,7 @@ Subscription subscription( bool? audibleNotifications, bool? pinToTop, bool? isMuted, - String? color, + int? color, }) { return Subscription( streamId: stream.streamId, @@ -161,7 +161,7 @@ Subscription subscription( audibleNotifications: audibleNotifications ?? false, pinToTop: pinToTop ?? false, isMuted: isMuted ?? false, - color: color ?? "#FF0000", + color: color ?? 0xFFFF0000, ); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index f1f53714a8..889aca20fb 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -183,18 +183,18 @@ void main() { group('handleEvent for SubscriptionEvent', () { final stream = eg.stream(); - test('SubscriptionProperty.color updates with a string value', () { + test('SubscriptionProperty.color updates with an int value', () { final store = eg.store(initialSnapshot: eg.initialSnapshot( streams: [stream], - subscriptions: [eg.subscription(stream, color: "#FF0000")], + subscriptions: [eg.subscription(stream, color: 0xFFFF0000)], )); - check(store.subscriptions[stream.streamId]!.color).equals('#FF0000'); + check(store.subscriptions[stream.streamId]!.color).equals(0xFFFF0000); store.handleEvent(SubscriptionUpdateEvent(id: 1, streamId: stream.streamId, property: SubscriptionProperty.color, - value: "#FF00FF")); - check(store.subscriptions[stream.streamId]!.color).equals('#FF00FF'); + value: 0xFFFF00FF)); + check(store.subscriptions[stream.streamId]!.color).equals(0xFFFF00FF); }); test('SubscriptionProperty.isMuted updates with a boolean value', () { From eac19710355920e96c356c146524d7019b72267f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 9 Nov 2023 12:01:18 -0500 Subject: [PATCH 05/12] ui [nfc]: Pull out clampLchLightness helper We'll use this for stream headers in the Inbox view, coming up. --- lib/widgets/color.dart | 18 ++++++++++++++++++ lib/widgets/unread_count_badge.dart | 19 ++----------------- 2 files changed, 20 insertions(+), 17 deletions(-) create mode 100644 lib/widgets/color.dart diff --git a/lib/widgets/color.dart b/lib/widgets/color.dart new file mode 100644 index 0000000000..85fd60ba18 --- /dev/null +++ b/lib/widgets/color.dart @@ -0,0 +1,18 @@ +import 'dart:ui'; + +import 'package:flutter_color_models/flutter_color_models.dart'; + +// This function promises to deal with "LCH" lightness, not "LAB" lightness, +// but it's not yet true. We haven't found a Dart libary that can work with LCH: +// +// We use LAB because some quick reading suggests that the "L" axis +// is the same in both representations: +// +// +// TODO try LCH; see linked discussion +Color clampLchLightness(Color color, num lowerLimit, num upperLimit) { + final asLab = LabColor.fromColor(color); + return asLab + .copyWith(lightness: asLab.lightness.clamp(lowerLimit, upperLimit)) + .toColor(); +} diff --git a/lib/widgets/unread_count_badge.dart b/lib/widgets/unread_count_badge.dart index 6a05b06d48..a24108cbc7 100644 --- a/lib/widgets/unread_count_badge.dart +++ b/lib/widgets/unread_count_badge.dart @@ -1,8 +1,8 @@ import 'dart:ui'; -import 'package:flutter_color_models/flutter_color_models.dart'; import 'package:flutter/material.dart'; +import 'color.dart'; import 'text.dart'; /// A widget to display a given number of unreads in a conversation. @@ -37,25 +37,10 @@ class UnreadCountBadge extends StatelessWidget { // Follows `.unread-count` in Vlad's replit: // // - - // The design uses "LCH", not "LAB", but we haven't found a Dart libary - // that can work with LCH: - // - // - // We use LAB because some quick reading suggests that the "L" axis - // is the same in both representations: - // - // and because the design doesn't use the LCH representation except to - // adjust an "L" value. // - // TODO try LCH; see linked discussion // TODO fix bug where our results differ from the replit's (see unit tests) // TODO profiling for expensive computation - final asLab = LabColor.fromColor(baseStreamColor!); - return asLab - .copyWith(lightness: asLab.lightness.clamp(30, 70)) - .toColor() - .withOpacity(0.3); + return clampLchLightness(baseStreamColor!, 30, 70).withOpacity(0.3); } @override From aee4dabcfed397e81e1bdce7796cd79f36b95d33 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 16 Nov 2023 13:53:25 -0500 Subject: [PATCH 06/12] model: Cache stream-color swatch on Subscription object As mentioned in a TODO, I'm not sure it makes sense to deal with package:flutter/painting.dart from within our lib/api/model/ code. We might end up factoring that out somewhere else. Next, we'll have our UnreadCountBadge widget optionally consume one of these swatches. --- lib/api/model/model.dart | 48 +++++++++++++++++++++++++++++--- test/api/model/model_checks.dart | 6 ++++ test/api/model/model_test.dart | 18 ++++++++++++ 3 files changed, 68 insertions(+), 4 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index e9276c72a0..53cadade5a 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -1,3 +1,5 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter/painting.dart'; import 'package:json_annotation/json_annotation.dart'; import 'reaction.dart'; @@ -328,14 +330,29 @@ class Subscription extends ZulipStream { /// As an int that dart:ui's Color constructor will take: /// @JsonKey(readValue: _readColor) - int color; - + int get color => _color; + int _color; + set color(int value) { + _color = value; + _swatch = null; + } static Object? _readColor(Map 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(color); + + @visibleForTesting + @JsonKey(includeToJson: false) + StreamColorSwatch? get debugCachedSwatchValue => _swatch; + Subscription({ required super.streamId, required super.name, @@ -357,8 +374,8 @@ class Subscription extends ZulipStream { required this.audibleNotifications, required this.pinToTop, required this.isMuted, - required this.color, - }); + required int color, + }) : _color = color; factory Subscription.fromJson(Map json) => _$SubscriptionFromJson(json); @@ -367,6 +384,29 @@ class Subscription extends ZulipStream { Map toJson() => _$SubscriptionToJson(this); } +/// A [ColorSwatch] with colors related to a base stream color. +/// +/// 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)); + + Color get base => this[_StreamColorVariant.base]!; + + static Map<_StreamColorVariant, Color> _compute(int base) { + final baseAsColor = Color(base); + + return { + _StreamColorVariant.base: baseAsColor, + }; + } +} + +enum _StreamColorVariant { + base, + // TODO more, like the unread-count badge background color +} + /// As in the get-messages response. /// /// https://zulip.com/api/get-messages#response diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 603712a9f4..1353280b04 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -1,3 +1,5 @@ +import 'dart:ui'; + import 'package:checks/checks.dart'; import 'package:zulip/api/model/model.dart'; @@ -5,6 +7,10 @@ extension ZulipStreamChecks on Subject { Subject get canRemoveSubscribersGroup => has((e) => e.canRemoveSubscribersGroup, 'canRemoveSubscribersGroup'); } +extension StreamColorSwatchChecks on Subject { + Subject get base => has((s) => s.base, 'base'); +} + extension MessageChecks on Subject { Subject get content => has((e) => e.content, 'content'); Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 66711b4034..557acc3f9d 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -1,4 +1,5 @@ import 'dart:convert'; +import 'dart:ui'; import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; @@ -114,6 +115,23 @@ 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('StreamColorSwatch', () { + test('base', () { + check(StreamColorSwatch(0xffffffff)).base.equals(const Color(0xffffffff)); + }); + }); }); group('Message', () { From 1352007d9c3b8c616311d52bf1e436e150d799bf Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 16 Nov 2023 13:48:01 -0500 Subject: [PATCH 07/12] UnreadCountBadge: Consume StreamColorSwatch for stream-colored background --- lib/api/model/model.dart | 14 +++- lib/widgets/recent_dm_conversations.dart | 2 +- lib/widgets/unread_count_badge.dart | 35 +++----- test/api/model/model_checks.dart | 1 + test/api/model/model_test.dart | 62 ++++++++++++++ test/widgets/unread_count_badge_checks.dart | 2 +- test/widgets/unread_count_badge_test.dart | 92 +++++++-------------- 7 files changed, 119 insertions(+), 89 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 53cadade5a..f939b31117 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -2,6 +2,7 @@ import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; import 'package:json_annotation/json_annotation.dart'; +import '../../widgets/color.dart'; import 'reaction.dart'; export 'reaction.dart'; @@ -393,18 +394,29 @@ class StreamColorSwatch extends ColorSwatch<_StreamColorVariant> { Color get base => this[_StreamColorVariant.base]!; + Color get unreadCountBadgeBackground => this[_StreamColorVariant.unreadCountBadgeBackground]!; + static Map<_StreamColorVariant, Color> _compute(int base) { final baseAsColor = Color(base); return { _StreamColorVariant.base: baseAsColor, + + // Follows `.unread-count` in Vlad's replit: + // + // + // + // TODO fix bug where our results differ from the replit's (see unit tests) + _StreamColorVariant.unreadCountBadgeBackground: + clampLchLightness(baseAsColor, 30, 70) + .withOpacity(0.3), }; } } enum _StreamColorVariant { base, - // TODO more, like the unread-count badge background color + unreadCountBadgeBackground, } /// As in the get-messages response. diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index 0122deb999..81df4ee472 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -134,7 +134,7 @@ class RecentDmConversationsItem extends StatelessWidget { const SizedBox(width: 12), unreadCount > 0 ? Padding(padding: const EdgeInsetsDirectional.only(end: 16), - child: UnreadCountBadge(baseStreamColor: null, + child: UnreadCountBadge(backgroundColor: null, count: unreadCount)) : const SizedBox(), ])))); diff --git a/lib/widgets/unread_count_badge.dart b/lib/widgets/unread_count_badge.dart index a24108cbc7..cc654eef37 100644 --- a/lib/widgets/unread_count_badge.dart +++ b/lib/widgets/unread_count_badge.dart @@ -2,7 +2,7 @@ import 'dart:ui'; import 'package:flutter/material.dart'; -import 'color.dart'; +import '../api/model/model.dart'; import 'text.dart'; /// A widget to display a given number of unreads in a conversation. @@ -13,42 +13,33 @@ class UnreadCountBadge extends StatelessWidget { const UnreadCountBadge({ super.key, required this.count, - required this.baseStreamColor, + required this.backgroundColor, this.bold = false, }); final int count; final bool bold; - /// A base stream color, from a stream subscription in user data, or null. + /// The badge's background color. /// - /// If not null, the background will be colored with an appropriate - /// transformation of this. + /// Pass a [StreamColorSwatch] 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? baseStreamColor; - - @visibleForTesting - Color getBackgroundColor() { - if (baseStreamColor == null) { - return const Color.fromRGBO(102, 102, 153, 0.15); - } - - // Follows `.unread-count` in Vlad's replit: - // - // - // - // TODO fix bug where our results differ from the replit's (see unit tests) - // TODO profiling for expensive computation - return clampLchLightness(baseStreamColor!, 30, 70).withOpacity(0.3); - } + final Color? backgroundColor; @override Widget build(BuildContext context) { + final effectiveBackgroundColor = switch (backgroundColor) { + StreamColorSwatch(unreadCountBadgeBackground: var color) => color, + Color() => backgroundColor, + null => const Color.fromRGBO(102, 102, 153, 0.15), + }; + return DecoratedBox( decoration: BoxDecoration( borderRadius: BorderRadius.circular(3), - color: getBackgroundColor(), + color: effectiveBackgroundColor, ), child: Padding( padding: const EdgeInsets.fromLTRB(4, 0, 4, 1), diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 1353280b04..1980467d8e 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -9,6 +9,7 @@ extension ZulipStreamChecks on Subject { extension StreamColorSwatchChecks on Subject { Subject get base => has((s) => s.base, 'base'); + Subject get unreadCountBadgeBackground => has((s) => s.unreadCountBadgeBackground, 'unreadCountBadgeBackground'); } extension MessageChecks on Subject { diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 557acc3f9d..be72fe4631 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -131,6 +131,68 @@ void main() { test('base', () { check(StreamColorSwatch(0xffffffff)).base.equals(const Color(0xffffffff)); }); + + test('unreadCountBadgeBackground', () { + void runCheck(int base, Color expected) { + check(StreamColorSwatch(base)).unreadCountBadgeBackground.equals(expected); + } + + // Check against everything in ZULIP_ASSIGNMENT_COLORS and EXTREME_COLORS + // in . + // On how to extract expected results from the replit, see: + // https://github.com/zulip/zulip-flutter/pull/371#discussion_r1393643523 + + // TODO Fix bug causing our implementation's results to differ from the + // replit's. Where they differ, see comment with what the replit gives. + + // ZULIP_ASSIGNMENT_COLORS + runCheck(0xff76ce90, const Color(0x4d65bd80)); + runCheck(0xfffae589, const Color(0x4dbdab53)); // 0x4dbdaa52 + runCheck(0xffa6c7e5, const Color(0x4d8eafcc)); // 0x4d8fb0cd + runCheck(0xffe79ab5, const Color(0x4de295b0)); // 0x4de194af + runCheck(0xffbfd56f, const Color(0x4d9eb551)); // 0x4d9eb450 + runCheck(0xfff4ae55, const Color(0x4de19d45)); // 0x4de09c44 + runCheck(0xffb0a5fd, const Color(0x4daba0f8)); // 0x4daca2f9 + runCheck(0xffaddfe5, const Color(0x4d83b4b9)); // 0x4d83b4ba + runCheck(0xfff5ce6e, const Color(0x4dcba749)); // 0x4dcaa648 + runCheck(0xffc2726a, const Color(0x4dc2726a)); + runCheck(0xff94c849, const Color(0x4d86ba3c)); // 0x4d86ba3b + runCheck(0xffbd86e5, const Color(0x4dbd86e5)); + runCheck(0xffee7e4a, const Color(0x4dee7e4a)); + runCheck(0xffa6dcbf, const Color(0x4d82b69b)); // 0x4d82b79b + runCheck(0xff95a5fd, const Color(0x4d95a5fd)); + runCheck(0xff53a063, const Color(0x4d53a063)); + runCheck(0xff9987e1, const Color(0x4d9987e1)); + runCheck(0xffe4523d, const Color(0x4de4523d)); + runCheck(0xffc2c2c2, const Color(0x4dababab)); + runCheck(0xff4f8de4, const Color(0x4d4f8de4)); + runCheck(0xffc6a8ad, const Color(0x4dc2a4a9)); // 0x4dc1a4a9 + runCheck(0xffe7cc4d, const Color(0x4dc3ab2a)); // 0x4dc2aa28 + runCheck(0xffc8bebf, const Color(0x4db3a9aa)); + runCheck(0xffa47462, const Color(0x4da47462)); + + // EXTREME_COLORS + runCheck(0xFFFFFFFF, const Color(0x4dababab)); + runCheck(0xFF000000, const Color(0x4d474747)); + runCheck(0xFFD3D3D3, const Color(0x4dababab)); + runCheck(0xFFA9A9A9, const Color(0x4da9a9a9)); + runCheck(0xFF808080, const Color(0x4d808080)); + runCheck(0xFFFFFF00, const Color(0x4dacb300)); // 0x4dacb200 + runCheck(0xFFFF0000, const Color(0x4dff0000)); + runCheck(0xFF008000, const Color(0x4d008000)); + runCheck(0xFF0000FF, const Color(0x4d0000ff)); // 0x4d0902ff + runCheck(0xFFEE82EE, const Color(0x4dee82ee)); + runCheck(0xFFFFA500, const Color(0x4def9800)); // 0x4ded9600 + runCheck(0xFF800080, const Color(0x4d810181)); // 0x4d810281 + runCheck(0xFF00FFFF, const Color(0x4d00c2c3)); // 0x4d00c3c5 + runCheck(0xFFFF00FF, const Color(0x4dff00ff)); + runCheck(0xFF00FF00, const Color(0x4d00cb00)); + runCheck(0xFF800000, const Color(0x4d8d140c)); // 0x4d8b130b + runCheck(0xFF008080, const Color(0x4d008080)); + runCheck(0xFF000080, const Color(0x4d492bae)); // 0x4d4b2eb3 + runCheck(0xFFFFFFE0, const Color(0x4dadad90)); // 0x4dacad90 + runCheck(0xFFFF69B4, const Color(0x4dff69b4)); + }); }); }); diff --git a/test/widgets/unread_count_badge_checks.dart b/test/widgets/unread_count_badge_checks.dart index ffd454c555..dcd3f99d74 100644 --- a/test/widgets/unread_count_badge_checks.dart +++ b/test/widgets/unread_count_badge_checks.dart @@ -6,5 +6,5 @@ import 'package:zulip/widgets/unread_count_badge.dart'; 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.getBackgroundColor(), 'background color'); + Subject get backgroundColor => has((b) => b.backgroundColor, 'backgroundColor'); } diff --git a/test/widgets/unread_count_badge_test.dart b/test/widgets/unread_count_badge_test.dart index e9240e3b8f..848372b976 100644 --- a/test/widgets/unread_count_badge_test.dart +++ b/test/widgets/unread_count_badge_test.dart @@ -1,83 +1,47 @@ import 'package:checks/checks.dart'; -import 'package:flutter/widgets.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/model.dart'; import 'package:zulip/widgets/unread_count_badge.dart'; -import 'unread_count_badge_checks.dart'; - void main() { group('UnreadCountBadge', () { testWidgets('smoke test; no crash', (tester) async { await tester.pumpWidget( const Directionality(textDirection: TextDirection.ltr, - child: UnreadCountBadge(count: 1, baseStreamColor: null))); + child: UnreadCountBadge(count: 1, backgroundColor: null))); tester.widget(find.text("1")); }); - test('colors', () { - void runCheck(Color? baseStreamColor, Color expectedBackgroundColor) { - check(UnreadCountBadge(count: 1, baseStreamColor: baseStreamColor)) - .backgroundColor.equals(expectedBackgroundColor); + group('background', () { + Future prepare(WidgetTester tester, Color? backgroundColor) async { + await tester.pumpWidget( + Directionality(textDirection: TextDirection.ltr, + child: UnreadCountBadge(count: 1, backgroundColor: backgroundColor))); } - runCheck(null, const Color(0x26666699)); - - // Check against everything in ZULIP_ASSIGNMENT_COLORS and EXTREME_COLORS - // in . - // On how to extract expected results from the replit, see: - // https://github.com/zulip/zulip-flutter/pull/371#discussion_r1393643523 - - // TODO Fix bug causing our implementation's results to differ from the - // replit's. Where they differ, see comment with what the replit gives. - - // ZULIP_ASSIGNMENT_COLORS - runCheck(const Color(0xff76ce90), const Color(0x4d65bd80)); - runCheck(const Color(0xfffae589), const Color(0x4dbdab53)); // 0x4dbdaa52 - runCheck(const Color(0xffa6c7e5), const Color(0x4d8eafcc)); // 0x4d8fb0cd - runCheck(const Color(0xffe79ab5), const Color(0x4de295b0)); // 0x4de194af - runCheck(const Color(0xffbfd56f), const Color(0x4d9eb551)); // 0x4d9eb450 - runCheck(const Color(0xfff4ae55), const Color(0x4de19d45)); // 0x4de09c44 - runCheck(const Color(0xffb0a5fd), const Color(0x4daba0f8)); // 0x4daca2f9 - runCheck(const Color(0xffaddfe5), const Color(0x4d83b4b9)); // 0x4d83b4ba - runCheck(const Color(0xfff5ce6e), const Color(0x4dcba749)); // 0x4dcaa648 - runCheck(const Color(0xffc2726a), const Color(0x4dc2726a)); - runCheck(const Color(0xff94c849), const Color(0x4d86ba3c)); // 0x4d86ba3b - runCheck(const Color(0xffbd86e5), const Color(0x4dbd86e5)); - runCheck(const Color(0xffee7e4a), const Color(0x4dee7e4a)); - runCheck(const Color(0xffa6dcbf), const Color(0x4d82b69b)); // 0x4d82b79b - runCheck(const Color(0xff95a5fd), const Color(0x4d95a5fd)); - runCheck(const Color(0xff53a063), const Color(0x4d53a063)); - runCheck(const Color(0xff9987e1), const Color(0x4d9987e1)); - runCheck(const Color(0xffe4523d), const Color(0x4de4523d)); - runCheck(const Color(0xffc2c2c2), const Color(0x4dababab)); - runCheck(const Color(0xff4f8de4), const Color(0x4d4f8de4)); - runCheck(const Color(0xffc6a8ad), const Color(0x4dc2a4a9)); // 0x4dc1a4a9 - runCheck(const Color(0xffe7cc4d), const Color(0x4dc3ab2a)); // 0x4dc2aa28 - runCheck(const Color(0xffc8bebf), const Color(0x4db3a9aa)); - runCheck(const Color(0xffa47462), const Color(0x4da47462)); + Color? findBackgroundColor(WidgetTester tester) { + final widget = tester.widget(find.byType(DecoratedBox)); + final decoration = widget.decoration as BoxDecoration; + return decoration.color; + } - // EXTREME_COLORS - runCheck(const Color(0xFFFFFFFF), const Color(0x4dababab)); - runCheck(const Color(0xFF000000), const Color(0x4d474747)); - runCheck(const Color(0xFFD3D3D3), const Color(0x4dababab)); - runCheck(const Color(0xFFA9A9A9), const Color(0x4da9a9a9)); - runCheck(const Color(0xFF808080), const Color(0x4d808080)); - runCheck(const Color(0xFFFFFF00), const Color(0x4dacb300)); // 0x4dacb200 - runCheck(const Color(0xFFFF0000), const Color(0x4dff0000)); - runCheck(const Color(0xFF008000), const Color(0x4d008000)); - runCheck(const Color(0xFF0000FF), const Color(0x4d0000ff)); // 0x4d0902ff - runCheck(const Color(0xFFEE82EE), const Color(0x4dee82ee)); - runCheck(const Color(0xFFFFA500), const Color(0x4def9800)); // 0x4ded9600 - runCheck(const Color(0xFF800080), const Color(0x4d810181)); // 0x4d810281 - runCheck(const Color(0xFF00FFFF), const Color(0x4d00c2c3)); // 0x4d00c3c5 - runCheck(const Color(0xFFFF00FF), const Color(0x4dff00ff)); - runCheck(const Color(0xFF00FF00), const Color(0x4d00cb00)); - runCheck(const Color(0xFF800000), const Color(0x4d8d140c)); // 0x4d8b130b - runCheck(const Color(0xFF008080), const Color(0x4d008080)); - runCheck(const Color(0xFF000080), const Color(0x4d492bae)); // 0x4d4b2eb3 - runCheck(const Color(0xFFFFFFE0), const Color(0x4dadad90)); // 0x4dacad90 - runCheck(const Color(0xFFFF69B4), const Color(0x4dff69b4)); + testWidgets('default color', (WidgetTester tester) async { + await prepare(tester, null); + check(findBackgroundColor(tester)).equals(const Color(0x26666699)); + }); + + testWidgets('specified color', (WidgetTester tester) async { + await prepare(tester, Colors.pink); + check(findBackgroundColor(tester)).equals(Colors.pink); + }); + + testWidgets('stream color', (WidgetTester tester) async { + final swatch = StreamColorSwatch(0xff76ce90); + await prepare(tester, swatch); + check(findBackgroundColor(tester)).equals(swatch.unreadCountBadgeBackground); + }); }); }); } From a96baf523d70ba1911269d67c589827405f1ebc3 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 16 Nov 2023 15:50:21 -0500 Subject: [PATCH 08/12] model: Add more colors to StreamColorSwatch We'll use these soon, in the Inbox view. --- lib/api/model/model.dart | 54 ++++++++++++++ test/api/model/model_checks.dart | 3 + test/api/model/model_test.dart | 117 +++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index f939b31117..730b28ff89 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -1,5 +1,6 @@ import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; +import 'package:flutter_color_models/flutter_color_models.dart'; import 'package:json_annotation/json_annotation.dart'; import '../../widgets/color.dart'; @@ -396,9 +397,30 @@ class StreamColorSwatch extends ColorSwatch<_StreamColorVariant> { Color get unreadCountBadgeBackground => this[_StreamColorVariant.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]!; + + /// 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]: + /// + Color get iconOnBarBackground => this[_StreamColorVariant.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, @@ -410,6 +432,35 @@ class StreamColorSwatch extends ColorSwatch<_StreamColorVariant> { _StreamColorVariant.unreadCountBadgeBackground: clampLchLightness(baseAsColor, 30, 70) .withOpacity(0.3), + + // Follows `.sidebar-row__icon` in Vlad's replit: + // + // + // TODO fix bug where our results differ from the replit's (see unit tests) + _StreamColorVariant.iconOnPlainBackground: clamped20to75, + + // Follows `.recepeient__icon` in Vlad's replit: + // + // + // + // 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: + // + // + // TODO I think [LabColor.interpolate] doesn't actually do LAB mixing; + // it just calls up to the superclass method [ColorModel.interpolate]: + // + // 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(), }; } } @@ -417,6 +468,9 @@ class StreamColorSwatch extends ColorSwatch<_StreamColorVariant> { enum _StreamColorVariant { base, unreadCountBadgeBackground, + iconOnPlainBackground, + iconOnBarBackground, + barBackground, } /// As in the get-messages response. diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 1980467d8e..eb95e36ef7 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -10,6 +10,9 @@ extension ZulipStreamChecks on Subject { extension StreamColorSwatchChecks on Subject { Subject get base => has((s) => s.base, 'base'); Subject get unreadCountBadgeBackground => has((s) => s.unreadCountBadgeBackground, 'unreadCountBadgeBackground'); + Subject get iconOnPlainBackground => has((s) => s.iconOnPlainBackground, 'iconOnPlainBackground'); + Subject get iconOnBarBackground => has((s) => s.iconOnBarBackground, 'iconOnBarBackground'); + Subject get barBackground => has((s) => s.barBackground, 'barBackground'); } extension MessageChecks on Subject { diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index be72fe4631..6c7b886732 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -193,6 +193,123 @@ void main() { runCheck(0xFFFFFFE0, const Color(0x4dadad90)); // 0x4dacad90 runCheck(0xFFFF69B4, const Color(0x4dff69b4)); }); + + test('iconOnPlainBackground', () { + void runCheck(int base, Color expected) { + check(StreamColorSwatch(base)).iconOnPlainBackground.equals(expected); + } + + // Check against everything in ZULIP_ASSIGNMENT_COLORS + // in . + // (Skipping `streamColors` because there are 100+ of them.) + + // TODO Fix bug causing our implementation's results to differ from the + // replit's. Where they differ, see comment with what the replit gives. + + runCheck(0xff76ce90, const Color(0xff73cb8d)); + runCheck(0xfffae589, const Color(0xffccb95f)); // 0xffcbb85e + runCheck(0xffa6c7e5, const Color(0xff9cbcda)); // 0xff9cbddb + runCheck(0xffe79ab5, const Color(0xffe79ab5)); + runCheck(0xffbfd56f, const Color(0xffacc25d)); + runCheck(0xfff4ae55, const Color(0xfff0ab52)); // 0xffefa951 + runCheck(0xffb0a5fd, const Color(0xffb0a5fd)); + runCheck(0xffaddfe5, const Color(0xff90c1c7)); // 0xff90c2c8 + runCheck(0xfff5ce6e, const Color(0xffd9b456)); // 0xffd8b355 + runCheck(0xffc2726a, const Color(0xffc2726a)); + runCheck(0xff94c849, const Color(0xff94c849)); + runCheck(0xffbd86e5, const Color(0xffbd86e5)); + runCheck(0xffee7e4a, const Color(0xffee7e4a)); + runCheck(0xffa6dcbf, const Color(0xff8fc4a8)); + runCheck(0xff95a5fd, const Color(0xff95a5fd)); + runCheck(0xff53a063, const Color(0xff53a063)); + runCheck(0xff9987e1, const Color(0xff9987e1)); + runCheck(0xffe4523d, const Color(0xffe4523d)); + runCheck(0xffc2c2c2, const Color(0xffb9b9b9)); + runCheck(0xff4f8de4, const Color(0xff4f8de4)); + runCheck(0xffc6a8ad, const Color(0xffc6a8ad)); + runCheck(0xffe7cc4d, const Color(0xffd1b839)); // 0xffd0b737 + runCheck(0xffc8bebf, const Color(0xffc0b6b7)); + runCheck(0xffa47462, const Color(0xffa47462)); + runCheck(0xffacc25d, const Color(0xffacc25d)); + }); + + test('iconOnBarBackground', () { + void runCheck(int base, Color expected) { + check(StreamColorSwatch(base)).iconOnBarBackground.equals(expected); + } + + // Check against everything in ZULIP_ASSIGNMENT_COLORS + // in . + // (Skipping `streamColors` because there are 100+ of them.) + + // TODO Fix bug causing our implementation's results to differ from the + // replit's. Where they differ, see comment with what the replit gives. + + runCheck(0xff76ce90, const Color(0xff46ba69)); + runCheck(0xfffae589, const Color(0xffb49f39)); // 0xffb29d3a + runCheck(0xffa6c7e5, const Color(0xff6f9ec9)); // 0xff6f9fcb + runCheck(0xffe79ab5, const Color(0xffdb6991)); + runCheck(0xffbfd56f, const Color(0xff8ea43e)); + runCheck(0xfff4ae55, const Color(0xffeb901a)); // 0xffea8d19 + runCheck(0xffb0a5fd, const Color(0xff7b69fc)); + runCheck(0xffaddfe5, const Color(0xff67aab2)); // 0xff67acb4 + runCheck(0xfff5ce6e, const Color(0xffc59a2c)); // 0xffc3992d + runCheck(0xffc2726a, const Color(0xffa94e45)); + runCheck(0xff94c849, const Color(0xff74a331)); + runCheck(0xffbd86e5, const Color(0xffa254da)); + runCheck(0xffee7e4a, const Color(0xffe55716)); + runCheck(0xffa6dcbf, const Color(0xff67af89)); + runCheck(0xff95a5fd, const Color(0xff5972fc)); + runCheck(0xff53a063, const Color(0xff3e784a)); + runCheck(0xff9987e1, const Color(0xff6f56d5)); + runCheck(0xffe4523d, const Color(0xffc8311c)); + runCheck(0xffc2c2c2, const Color(0xff9a9a9a)); + runCheck(0xff4f8de4, const Color(0xff216cd5)); + runCheck(0xffc6a8ad, const Color(0xffae838a)); + runCheck(0xffe7cc4d, const Color(0xffa69127)); // 0xffa38f26 + runCheck(0xffc8bebf, const Color(0xffa49597)); + runCheck(0xffa47462, const Color(0xff7f584a)); + runCheck(0xffacc25d, const Color(0xff8ea43e)); + }); + + test('barBackground', () { + void runCheck(int base, Color expected) { + check(StreamColorSwatch(base)).barBackground.equals(expected); + } + + // Check against everything in ZULIP_ASSIGNMENT_COLORS + // in . + // (Skipping `streamColors` because there are 100+ of them.) + + // TODO Fix bug causing our implementation's results to differ from the + // replit's. Where they differ, see comment with what the replit gives. + + runCheck(0xff76ce90, const Color(0xffddefe1)); + runCheck(0xfffae589, const Color(0xfff1ead7)); // 0xfff0ead6 + runCheck(0xffa6c7e5, const Color(0xffe5ebf2)); // 0xffe5ecf2 + runCheck(0xffe79ab5, const Color(0xfff6e4ea)); + runCheck(0xffbfd56f, const Color(0xffe9edd6)); + runCheck(0xfff4ae55, const Color(0xfffbe7d4)); // 0xfffae7d4 + runCheck(0xffb0a5fd, const Color(0xffeae6fa)); + runCheck(0xffaddfe5, const Color(0xffe2edee)); + runCheck(0xfff5ce6e, const Color(0xfff5e9d5)); // 0xfff4e9d5 + runCheck(0xffc2726a, const Color(0xfff0dbd8)); // 0xffefdbd8 + runCheck(0xff94c849, const Color(0xffe5eed3)); // 0xffe4eed3 + runCheck(0xffbd86e5, const Color(0xffeddff5)); + runCheck(0xffee7e4a, const Color(0xfffdded1)); // 0xfffcded1 + runCheck(0xffa6dcbf, const Color(0xffe2ede7)); + runCheck(0xff95a5fd, const Color(0xffe5e6fa)); // 0xffe4e6fa + runCheck(0xff53a063, const Color(0xffd5e5d6)); + runCheck(0xff9987e1, const Color(0xffe5dff4)); + runCheck(0xffe4523d, const Color(0xfffcd6cd)); // 0xfffbd6cd + runCheck(0xffc2c2c2, const Color(0xffebebeb)); + runCheck(0xff4f8de4, const Color(0xffd9e0f5)); // 0xffd8e0f5 + runCheck(0xffc6a8ad, const Color(0xffeee7e8)); + runCheck(0xffe7cc4d, const Color(0xfff4ead0)); // 0xfff3eacf + runCheck(0xffc8bebf, const Color(0xffeceaea)); + runCheck(0xffa47462, const Color(0xffe7dad6)); + runCheck(0xffacc25d, const Color(0xffe9edd6)); + }); }); }); From faf00d90d64e739ca32d51701960fa933e2c2357 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Oct 2023 13:26:59 -0400 Subject: [PATCH 09/12] inbox: Add "Inbox" page Fixes: #117 --- lib/widgets/app.dart | 6 + lib/widgets/inbox.dart | 513 +++++++++++++++++++++++ lib/widgets/recent_dm_conversations.dart | 2 +- test/flutter_checks.dart | 7 + test/model/test_store.dart | 8 + test/widgets/inbox_test.dart | 297 +++++++++++++ 6 files changed, 832 insertions(+), 1 deletion(-) create mode 100644 lib/widgets/inbox.dart create mode 100644 test/widgets/inbox_test.dart diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index a7fe91bee9..351108b80b 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -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 'inbox.dart'; import 'login.dart'; import 'message_list.dart'; import 'page.dart'; @@ -254,6 +255,11 @@ class HomePage extends StatelessWidget { narrow: const AllMessagesNarrow())), child: const Text("All messages")), const SizedBox(height: 16), + ElevatedButton( + onPressed: () => Navigator.push(context, + InboxPage.buildRoute(context: context)), + child: const Text("Inbox")), // TODO(i18n) + const SizedBox(height: 16), ElevatedButton( onPressed: () => Navigator.push(context, RecentDmConversationsPage.buildRoute(context: context)), diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart new file mode 100644 index 0000000000..858d732f11 --- /dev/null +++ b/lib/widgets/inbox.dart @@ -0,0 +1,513 @@ +import 'package:flutter/material.dart'; + +import '../api/model/model.dart'; +import '../model/narrow.dart'; +import '../model/recent_dm_conversations.dart'; +import '../model/unreads.dart'; +import 'icons.dart'; +import 'message_list.dart'; +import 'page.dart'; +import 'sticky_header.dart'; +import 'store.dart'; +import 'text.dart'; +import 'unread_count_badge.dart'; + +class InboxPage extends StatefulWidget { + const InboxPage({super.key}); + + static Route buildRoute({required BuildContext context}) { + return MaterialAccountWidgetRoute(context: context, + page: const InboxPage()); + } + + @override + State createState() => _InboxPageState(); +} + +class _InboxPageState extends State with PerAccountStoreAwareStateMixin { + Unreads? unreadsModel; + RecentDmConversationsView? recentDmConversationsModel; + + get allDmsCollapsed => _allDmsCollapsed; + bool _allDmsCollapsed = false; + set allDmsCollapsed(value) { + setState(() { + _allDmsCollapsed = value; + }); + } + + get collapsedStreamIds => _collapsedStreamIds; + final Set _collapsedStreamIds = {}; + void collapseStream(int streamId) { + setState(() { + _collapsedStreamIds.add(streamId); + }); + } + void uncollapseStream(int streamId) { + setState(() { + _collapsedStreamIds.remove(streamId); + }); + } + + @override + void onNewStore() { + final newStore = PerAccountStoreWidget.of(context); + unreadsModel?.removeListener(_modelChanged); + unreadsModel = newStore.unreads..addListener(_modelChanged); + recentDmConversationsModel?.removeListener(_modelChanged); + recentDmConversationsModel = newStore.recentDmConversationsView + ..addListener(_modelChanged); + } + + @override + void dispose() { + unreadsModel?.removeListener(_modelChanged); + recentDmConversationsModel?.removeListener(_modelChanged); + super.dispose(); + } + + void _modelChanged() { + setState(() { + // Much of the state lives in [unreadsModel] and + // [recentDmConversationsModel]. + // This method was called because one of those just changed. + // + // We also update some state that lives locally: we reset a collapsible + // row's collapsed state when it's cleared of unreads. + // TODO(perf) handle those updates efficiently + collapsedStreamIds.removeWhere((streamId) => + !unreadsModel!.streams.containsKey(streamId)); + if (unreadsModel!.dms.isEmpty) { + allDmsCollapsed = false; + } + }); + } + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + + final streams = store.streams; + final subscriptions = store.subscriptions; + + // TODO(perf) make an incrementally-updated view-model for InboxPage + final sections = <_InboxSectionData>[]; + + // TODO efficiently include DM conversations that aren't recent enough + // to appear in recentDmConversationsView, but still have unreads in + // unreadsModel. + final dmItems = <(DmNarrow, int)>[]; + int allDmsCount = 0; + for (final dmNarrow in recentDmConversationsModel!.sorted) { + final countInNarrow = unreadsModel!.countInDmNarrow(dmNarrow); + if (countInNarrow == 0) { + continue; + } + dmItems.add((dmNarrow, countInNarrow)); + allDmsCount += countInNarrow; + } + if (allDmsCount > 0) { + sections.add(_AllDmsSectionData(allDmsCount, dmItems)); + } + + final sortedUnreadStreams = unreadsModel!.streams.entries + .where((entry) => + streams.containsKey(entry.key) // TODO(log) a bug if missing in streams + // Filter out any straggling unreads in unsubscribed streams. + // There won't normally be any, but it happens with certain infrequent + // state changes, typically for less than a few hundred milliseconds. + // See [Unreads]. + // + // Also, we want to depend on the subscription data for things like + // choosing the stream icon. + && subscriptions.containsKey(entry.key)) + .toList() + ..sort((a, b) { + final streamA = streams[a.key]!; + final streamB = streams[b.key]!; + + // TODO(i18n) something like JS's String.prototype.localeCompare + return streamA.name.toLowerCase().compareTo(streamB.name.toLowerCase()); + }) + ..sort((a, b) { + // TODO "pin" icon on the stream row? dividers in the list? + final aPinned = subscriptions[a.key]!.pinToTop; + final bPinned = subscriptions[b.key]!.pinToTop; + return aPinned == bPinned ? 0 : (aPinned ? -1 : 1); + }); + + for (final MapEntry(key: streamId, value: topics) in sortedUnreadStreams) { + final topicItems = <(String, int, int)>[]; + int countInStream = 0; + for (final MapEntry(key: topic, value: messageIds) in topics.entries) { + final countInTopic = messageIds.length; + topicItems.add((topic, countInTopic, messageIds.last)); + countInStream += countInTopic; + } + if (countInStream == 0) { + continue; + } + topicItems.sort((a, b) { + final (_, _, aLastUnreadId) = a; + final (_, _, bLastUnreadId) = b; + return bLastUnreadId - aLastUnreadId; + }); + sections.add(_StreamSectionData(streamId, countInStream, topicItems)); + } + + // TODO(#346) Filter out muted messages. + // (Eventually let the user toggle that filtering?) + + return Scaffold( + appBar: AppBar(title: const Text('Inbox')), + body: StickyHeaderListView.builder( + itemCount: sections.length, + itemBuilder: (context, index) { + final section = sections[index]; + switch (section) { + case _AllDmsSectionData(): + return _AllDmsSection( + data: section, + collapsed: allDmsCollapsed, + pageState: this, + ); + case _StreamSectionData(:var streamId): + final collapsed = collapsedStreamIds.contains(streamId); + return _StreamSection(data: section, collapsed: collapsed, pageState: this); + } + })); + } +} + +sealed class _InboxSectionData { + const _InboxSectionData(); +} + +class _AllDmsSectionData extends _InboxSectionData { + final int count; + final List<(DmNarrow, int)> items; + + const _AllDmsSectionData(this.count, this.items); +} + +class _StreamSectionData extends _InboxSectionData { + final int streamId; + final int count; + final List<(String, int, int)> items; + + const _StreamSectionData(this.streamId, this.count, this.items); +} + +abstract class _HeaderItem extends StatelessWidget { + final bool collapsed; + final _InboxPageState pageState; + final int count; + + const _HeaderItem({ + required this.collapsed, + required this.pageState, + required this.count, + }); + + String get title; + IconData get icon; + Color get collapsedIconColor; + Color get uncollapsedIconColor; + Color get uncollapsedBackgroundColor; + Color? get unreadCountBadgeBackgroundColor; + + void Function() get onCollapseButtonTap; + void Function() get onRowTap; + + @override + Widget build(BuildContext context) { + return Material( + color: collapsed ? Colors.white : uncollapsedBackgroundColor, + child: InkWell( + // TODO use onRowTap to handle taps that are not on the collapse button. + // Probably we should give the collapse button a 44px or 48px square + // touch target: + // + // But that's in tension with the Figma, which gives these header rows + // 40px min height. + onTap: onCollapseButtonTap, + child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ + Padding(padding: const EdgeInsets.all(10), + child: Icon(size: 20, color: const Color(0x7F1D2E48), + collapsed ? ZulipIcons.arrow_right : ZulipIcons.arrow_down)), + Icon(size: 18, color: collapsed ? collapsedIconColor : uncollapsedIconColor, + icon), + const SizedBox(width: 5), + Expanded(child: Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Text( + style: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 17, + height: (20 / 17), + color: Color(0xFF222222), + ).merge(weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900)), + maxLines: 1, + overflow: TextOverflow.ellipsis, + title))), + const SizedBox(width: 12), + // TODO(#384) for streams, show @-mention indicator when it applies + Padding(padding: const EdgeInsetsDirectional.only(end: 16), + child: UnreadCountBadge(backgroundColor: unreadCountBadgeBackgroundColor, bold: true, + count: count)), + ]))); + } +} + +class _AllDmsHeaderItem extends _HeaderItem { + const _AllDmsHeaderItem({ + required super.collapsed, + required super.pageState, + required super.count, + }); + + @override get title => 'Direct messages'; // TODO(i18n) + @override get icon => ZulipIcons.user; + @override get collapsedIconColor => const Color(0xFF222222); + @override get uncollapsedIconColor => const Color(0xFF222222); + @override get uncollapsedBackgroundColor => const Color(0xFFF3F0E7); + @override get unreadCountBadgeBackgroundColor => null; + + @override get onCollapseButtonTap => () { + pageState.allDmsCollapsed = !collapsed; + }; + @override get onRowTap => onCollapseButtonTap; // TODO open all-DMs narrow? +} + +class _AllDmsSection extends StatelessWidget { + const _AllDmsSection({ + required this.data, + required this.collapsed, + required this.pageState, + }); + + final _AllDmsSectionData data; + final bool collapsed; + final _InboxPageState pageState; + + @override + Widget build(BuildContext context) { + final header = _AllDmsHeaderItem( + count: data.count, + collapsed: collapsed, + pageState: pageState, + ); + return StickyHeaderItem( + header: header, + child: Column(children: [ + header, + if (!collapsed) ...data.items.map((item) { + final (narrow, count) = item; + return _DmItem( + narrow: narrow, + count: count, + allDmsCount: data.count, + pageState: pageState, + ); + }), + ])); + } +} + +class _DmItem extends StatelessWidget { + const _DmItem({ + required this.narrow, + required this.count, + required this.allDmsCount, + required this.pageState + }); + + final DmNarrow narrow; + final int count; + final int allDmsCount; + final _InboxPageState pageState; + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final selfUser = store.users[store.account.userId]!; + + final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem] + [] => selfUser.fullName, + [var otherUserId] => store.users[otherUserId]?.fullName ?? '(unknown user)', + + // TODO(i18n): List formatting, like you can do in JavaScript: + // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu']) + // // 'Chris、Greg、Alya、Shu' + _ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)').join(', '), + }; + + return StickyHeaderItem( + header: _AllDmsHeaderItem( + count: allDmsCount, + collapsed: false, + pageState: pageState, + ), + allowOverflow: true, + child: Material( + color: Colors.white, + child: InkWell( + onTap: () { + Navigator.push(context, + MessageListPage.buildRoute(context: context, narrow: narrow)); + }, + child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 34), + child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ + const SizedBox(width: 63), + Expanded(child: Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Text( + style: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 17, + height: (20 / 17), + color: Color(0xFF222222), + ).merge(weightVariableTextStyle(context)), + maxLines: 2, + overflow: TextOverflow.ellipsis, + title))), + const SizedBox(width: 12), + Padding(padding: const EdgeInsetsDirectional.only(end: 16), + child: UnreadCountBadge(backgroundColor: null, + count: count)), + ]))))); + } +} + +class _StreamHeaderItem extends _HeaderItem { + final Subscription subscription; + + const _StreamHeaderItem({ + required this.subscription, + required super.collapsed, + required super.pageState, + required super.count, + }); + + @override get title => subscription.name; + @override get icon => switch (subscription) { + // TODO these icons aren't quite right yet; see this message and the following: + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1680637 + Subscription(isWebPublic: true) => ZulipIcons.globe, + Subscription(inviteOnly: true) => ZulipIcons.lock, + Subscription() => ZulipIcons.hash_sign, + }; + @override get collapsedIconColor => subscription.colorSwatch().iconOnPlainBackground; + @override get uncollapsedIconColor => subscription.colorSwatch().iconOnBarBackground; + @override get uncollapsedBackgroundColor => + subscription.colorSwatch().barBackground; + @override get unreadCountBadgeBackgroundColor => + subscription.colorSwatch().unreadCountBadgeBackground; + + @override get onCollapseButtonTap => () { + if (collapsed) { + pageState.uncollapseStream(subscription.streamId); + } else { + pageState.collapseStream(subscription.streamId); + } + }; + @override get onRowTap => onCollapseButtonTap; // TODO open stream narrow +} + +class _StreamSection extends StatelessWidget { + const _StreamSection({ + required this.data, + required this.collapsed, + required this.pageState, + }); + + final _StreamSectionData data; + final bool collapsed; + final _InboxPageState pageState; + + @override + Widget build(BuildContext context) { + final subscription = PerAccountStoreWidget.of(context).subscriptions[data.streamId]!; + final header = _StreamHeaderItem( + subscription: subscription, + count: data.count, + collapsed: collapsed, + pageState: pageState, + ); + return StickyHeaderItem( + header: header, + child: Column(children: [ + header, + if (!collapsed) ...data.items.map((item) { + final (topic, count, _) = item; + return _TopicItem( + streamId: data.streamId, + topic: topic, + count: count, + streamCount: data.count, + pageState: pageState, + ); + }), + ])); + } +} + +class _TopicItem extends StatelessWidget { + const _TopicItem({ + required this.streamId, + required this.topic, + required this.count, + required this.streamCount, + required this.pageState, + }); + + final int streamId; + final String topic; + final int count; + final int streamCount; + final _InboxPageState pageState; + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final subscription = store.subscriptions[streamId]!; + + return StickyHeaderItem( + header: _StreamHeaderItem( + subscription: subscription, + count: streamCount, + collapsed: false, + pageState: pageState, + ), + allowOverflow: true, + child: Material( + color: Colors.white, + child: InkWell( + onTap: () { + final narrow = TopicNarrow(streamId, topic); + Navigator.push(context, + MessageListPage.buildRoute(context: context, narrow: narrow)); + }, + child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 34), + child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ + const SizedBox(width: 63), + Expanded(child: Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Text( + style: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 17, + height: (20 / 17), + color: Color(0xFF222222), + ).merge(weightVariableTextStyle(context)), + maxLines: 2, + overflow: TextOverflow.ellipsis, + topic))), + const SizedBox(width: 12), + // TODO(#384) show @-mention indicator when it applies + Padding(padding: const EdgeInsetsDirectional.only(end: 16), + child: UnreadCountBadge(backgroundColor: subscription.colorSwatch(), + count: count)), + ]))))); + } +} diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index 81df4ee472..9ecbb9b0f8 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -86,7 +86,7 @@ class RecentDmConversationsItem extends StatelessWidget { final String title; final Widget avatar; - switch (narrow.otherRecipientIds) { + switch (narrow.otherRecipientIds) { // TODO dedupe with DM items in [InboxPage] case []: title = selfUser.fullName; avatar = AvatarImage(userId: selfUser.userId); diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 6d3dfab777..7e24ed4782 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -22,6 +22,13 @@ extension GlobalKeyChecks> on Subject get currentState => has((k) => k.currentState, 'currentState'); } +extension IconChecks on Subject { + Subject get icon => has((i) => i.icon, 'icon'); + Subject get color => has((i) => i.color, 'color'); + + // TODO others +} + extension RouteChecks on Subject> { Subject get settings => has((r) => r.settings, 'settings'); } diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 740a3b0788..6b1d293b11 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -76,4 +76,12 @@ extension PerAccountStoreTestExtension on PerAccountStore { void addStreams(List streams) { handleEvent(StreamCreateEvent(id: 1, streams: streams)); } + + void addSubscription(Subscription subscription) { + addSubscriptions([subscription]); + } + + void addSubscriptions(List subscriptions) { + handleEvent(SubscriptionAddEvent(id: 1, subscriptions: subscriptions)); + } } diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart new file mode 100644 index 0000000000..61678527dd --- /dev/null +++ b/test/widgets/inbox_test.dart @@ -0,0 +1,297 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/icons.dart'; +import 'package:zulip/widgets/inbox.dart'; +import 'package:zulip/widgets/store.dart'; + +import '../example_data.dart' as eg; +import '../flutter_checks.dart'; +import '../model/binding.dart'; +import '../model/test_store.dart'; + +void main() { + TestZulipBinding.ensureInitialized(); + + late PerAccountStore store; + + Future setupPage(WidgetTester tester, { + List? streams, + List? subscriptions, + List? users, + required List unreadMessages, + NavigatorObserver? navigatorObserver, + }) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + + store + ..addStreams(streams ?? []) + ..addSubscriptions(subscriptions ?? []) + ..addUsers(users ?? [eg.selfUser]); + + for (final message in unreadMessages) { + assert(!message.flags.contains(MessageFlag.read)); + store.handleEvent(MessageEvent(id: 1, message: message)); + } + + await tester.pumpWidget( + GlobalStoreWidget( + child: MaterialApp( + navigatorObservers: [if (navigatorObserver != null) navigatorObserver], + home: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: const InboxPage())))); + + // global store and per-account store get loaded + await tester.pumpAndSettle(); + } + + /// Set up an inbox view with lots of interesting content. + Future setupVarious(WidgetTester tester) async { + final stream1 = eg.stream(streamId: 1); + final sub1 = eg.subscription(stream1); + final stream2 = eg.stream(streamId: 2); + final sub2 = eg.subscription(stream2); + + await setupPage(tester, + streams: [stream1, stream2], + subscriptions: [sub1, sub2], + users: [eg.selfUser, eg.otherUser, eg.thirdUser], + unreadMessages: [ + eg.streamMessage(stream: stream1, topic: 'specific topic', flags: []), + eg.streamMessage(stream: stream2, flags: []), + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []), + eg.dmMessage(from: eg.otherUser, to: [eg.selfUser, eg.thirdUser], flags: []), + ]); + } + + /// Find an all-DMs header element. + // Why "an" all-DMs header element? Because there might be two: one that + // floats at the top of the screen to give the "sticky header" effect, and one + // that scrolls normally, the way it would in a regular [ListView]. + // TODO we'll need to find both and run checks on them, knowing which is which. + Widget? findAllDmsHeaderRow(WidgetTester tester) { + final rowLabel = tester.widgetList( + find.text('Direct messages'), + ).firstOrNull; + if (rowLabel == null) { + return null; + } + + return tester.widget( + find.ancestor( + of: find.byWidget(rowLabel), + matching: find.byType(Row))); + } + + /// For the given stream ID, find a stream header element. + // Why "an" all-DMs header element? Because there might be two: one that + // floats at the top of the screen to give the "sticky header" effect, and one + // that scrolls normally, the way it would in a regular [ListView]. + // TODO we'll need to find both and run checks on them, knowing which is which. + Widget? findStreamHeaderRow(WidgetTester tester, int streamId) { + final stream = store.streams[streamId]!; + final rowLabel = tester.widgetList(find.text(stream.name)).firstOrNull; + if (rowLabel == null) { + return null; + } + + return tester.widget( + find.ancestor( + of: find.byWidget(rowLabel), + matching: find.byType(Row))); + } + + IconData expectedStreamHeaderIcon(int streamId) { + final subscription = store.subscriptions[streamId]!; + return switch (subscription) { + Subscription(isWebPublic: true) => ZulipIcons.globe, + Subscription(inviteOnly: true) => ZulipIcons.lock, + Subscription() => ZulipIcons.hash_sign, + }; + } + + Icon findStreamHeaderIcon(WidgetTester tester, int streamId) { + final expectedIcon = expectedStreamHeaderIcon(streamId); + final headerRow = findStreamHeaderRow(tester, streamId); + check(headerRow).isNotNull(); + + return tester.widget(find.descendant( + of: find.byWidget(headerRow!), + matching: find.byIcon(expectedIcon), + )); + } + + group('InboxPage', () { + testWidgets('page builds; empty', (tester) async { + await setupPage(tester, unreadMessages: []); + }); + + // TODO more checks: ordering, etc. + testWidgets('page builds; not empty', (tester) async { + await setupVarious(tester); + }); + + // TODO test that tapping a conversation row opens the message list + // for the conversation + + group('collapsing', () { + Icon findHeaderCollapseIcon(WidgetTester tester, Widget headerRow) { + return tester.widget( + find.descendant( + of: find.byWidget(headerRow), + matching: find.byWidgetPredicate( + (widget) => widget is Icon + && (widget.icon == ZulipIcons.arrow_down + || widget.icon == ZulipIcons.arrow_right)))); + } + + group('all-DMs section', () { + Future tapCollapseIcon(WidgetTester tester) async { + final headerRow = findAllDmsHeaderRow(tester); + check(headerRow).isNotNull(); + final icon = findHeaderCollapseIcon(tester, headerRow!); + await tester.tap(find.byWidget(icon)); + await tester.pump(); + } + + /// Check that the section appears uncollapsed. + /// + /// For [findSectionContent], pass a [Finder] that will find some of + /// the section's content if it is uncollapsed. The function will + /// check that it finds something. + void checkAppearsUncollapsed( + WidgetTester tester, + Finder findSectionContent, + ) { + final headerRow = findAllDmsHeaderRow(tester); + check(headerRow).isNotNull(); + final icon = findHeaderCollapseIcon(tester, headerRow!); + check(icon).icon.equals(ZulipIcons.arrow_down); + // TODO check bar background color + check(tester.widgetList(findSectionContent)).isNotEmpty(); + } + + /// Check that the section appears collapsed. + /// + /// For [findSectionContent], pass a [Finder] that would find some of + /// the section's content if it were uncollapsed. The function will + /// check that the finder comes up empty. + void checkAppearsCollapsed( + WidgetTester tester, + Finder findSectionContent, + ) { + final headerRow = findAllDmsHeaderRow(tester); + check(headerRow).isNotNull(); + final icon = findHeaderCollapseIcon(tester, headerRow!); + check(icon).icon.equals(ZulipIcons.arrow_right); + // TODO check bar background color + check(tester.widgetList(findSectionContent)).isEmpty(); + } + + testWidgets('appearance', (tester) async { + await setupVarious(tester); + + final headerRow = findAllDmsHeaderRow(tester); + check(headerRow).isNotNull(); + + final findSectionContent = find.text(eg.otherUser.fullName); + + checkAppearsUncollapsed(tester, findSectionContent); + await tapCollapseIcon(tester); + checkAppearsCollapsed(tester, findSectionContent); + await tapCollapseIcon(tester); + checkAppearsUncollapsed(tester, findSectionContent); + }); + + // TODO check it remains collapsed even if you scroll far away and back + + // TODO check that it's always uncollapsed when it appears after being + // absent, even if it was collapsed the last time it was present. + // (Could test multiple triggers for its reappearance: it could + // reappear because a new unread arrived, but with #296 it could also + // reappear because of a change in muted-users state.) + }); + + group('stream section', () { + Future tapCollapseIcon(WidgetTester tester, int streamId) async { + final headerRow = findStreamHeaderRow(tester, streamId); + check(headerRow).isNotNull(); + final icon = findHeaderCollapseIcon(tester, headerRow!); + await tester.tap(find.byWidget(icon)); + await tester.pump(); + } + + /// Check that the section appears uncollapsed. + /// + /// For [findSectionContent], pass a [Finder] that will find some of + /// the section's content if it is uncollapsed. The function will + /// check that it finds something. + void checkAppearsUncollapsed( + WidgetTester tester, + int streamId, + Finder findSectionContent, + ) { + final subscription = store.subscriptions[streamId]!; + final headerRow = findStreamHeaderRow(tester, streamId); + check(headerRow).isNotNull(); + final collapseIcon = findHeaderCollapseIcon(tester, headerRow!); + check(collapseIcon).icon.equals(ZulipIcons.arrow_down); + final streamIcon = findStreamHeaderIcon(tester, streamId); + check(streamIcon).color.equals(subscription.colorSwatch().iconOnBarBackground); + // TODO check bar background color + check(tester.widgetList(findSectionContent)).isNotEmpty(); + } + + /// Check that the section appears collapsed. + /// + /// For [findSectionContent], pass a [Finder] that would find some of + /// the section's content if it were uncollapsed. The function will + /// check that the finder comes up empty. + void checkAppearsCollapsed( + WidgetTester tester, + int streamId, + Finder findSectionContent, + ) { + final subscription = store.subscriptions[streamId]!; + final headerRow = findStreamHeaderRow(tester, streamId); + check(headerRow).isNotNull(); + final collapseIcon = findHeaderCollapseIcon(tester, headerRow!); + check(collapseIcon).icon.equals(ZulipIcons.arrow_right); + final streamIcon = findStreamHeaderIcon(tester, streamId); + check(streamIcon).color.equals(subscription.colorSwatch().iconOnPlainBackground); + // TODO check bar background color + check(tester.widgetList(findSectionContent)).isEmpty(); + } + + testWidgets('appearance', (tester) async { + await setupVarious(tester); + + final headerRow = findStreamHeaderRow(tester, 1); + check(headerRow).isNotNull(); + + final findSectionContent = find.text('specific topic'); + + checkAppearsUncollapsed(tester, 1, findSectionContent); + await tapCollapseIcon(tester, 1); + checkAppearsCollapsed(tester, 1, findSectionContent); + await tapCollapseIcon(tester, 1); + checkAppearsUncollapsed(tester, 1, findSectionContent); + }); + + // TODO check it remains collapsed even if you scroll far away and back + + // TODO check that it's always uncollapsed when it appears after being + // absent, even if it was collapsed the last time it was present. + // (Could test multiple triggers for its reappearance: it could + // reappear because a new unread arrived, but with #346 it could also + // reappear because you unmuted a conversation.) + }); + }); + }); +} From a8b084f7dec0f36e9ed4fd686f4b09bf1784def3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 19 Nov 2023 22:07:39 -0800 Subject: [PATCH 10/12] inbox: Use compareTo rather than subtraction for comparison The subtraction is fine if there isn't overflow, which should be impossible for Zulip message IDs. (They need to fit into 54 bits in order to not confuse the web client.) Still, compareTo feels a bit more explicit about what we mean. --- lib/widgets/inbox.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 858d732f11..3d6f410e25 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -150,7 +150,7 @@ class _InboxPageState extends State with PerAccountStoreAwareStateMix topicItems.sort((a, b) { final (_, _, aLastUnreadId) = a; final (_, _, bLastUnreadId) = b; - return bLastUnreadId - aLastUnreadId; + return bLastUnreadId.compareTo(aLastUnreadId); }); sections.add(_StreamSectionData(streamId, countInStream, topicItems)); } From 0219c16ae568c91185c2b9d22730524d66cca355 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 19 Nov 2023 22:07:39 -0800 Subject: [PATCH 11/12] inbox: Sort once, not twice, as List.sort may not be stable The trick of sorting first by the low-order key and then by the high-order key works great if the sort algorithm leaves elements in their existing relative order when they compare equal, aka if the sort is "stable". But Dart's List.sort disclaims any guarantee of being stable. So this trick doesn't work; the pinned streams and unpinned streams could each get scrambled out of alphabetical order in the course of the second sort. Instead do one sort, with a comparison function that looks at both criteria. --- lib/widgets/inbox.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 3d6f410e25..e10c05f5c5 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -123,17 +123,18 @@ class _InboxPageState extends State with PerAccountStoreAwareStateMix && subscriptions.containsKey(entry.key)) .toList() ..sort((a, b) { + // TODO "pin" icon on the stream row? dividers in the list? + final aPinned = subscriptions[a.key]!.pinToTop; + final bPinned = subscriptions[b.key]!.pinToTop; + if (aPinned != bPinned) { + return aPinned ? -1 : 1; + } + final streamA = streams[a.key]!; final streamB = streams[b.key]!; // TODO(i18n) something like JS's String.prototype.localeCompare return streamA.name.toLowerCase().compareTo(streamB.name.toLowerCase()); - }) - ..sort((a, b) { - // TODO "pin" icon on the stream row? dividers in the list? - final aPinned = subscriptions[a.key]!.pinToTop; - final bPinned = subscriptions[b.key]!.pinToTop; - return aPinned == bPinned ? 0 : (aPinned ? -1 : 1); }); for (final MapEntry(key: streamId, value: topics) in sortedUnreadStreams) { From 98e3527d6c5f8b41f4df8e786f950c9da6c2ca28 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 19 Nov 2023 22:13:27 -0800 Subject: [PATCH 12/12] inbox [nfc]: Get data all from subscription object, vs. stream Since we always have the subscription object around, we might as well use it for the stream properties too, simplifying this code a bit. --- lib/widgets/inbox.dart | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index e10c05f5c5..b5f5dad94b 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -86,8 +86,6 @@ class _InboxPageState extends State with PerAccountStoreAwareStateMix @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - - final streams = store.streams; final subscriptions = store.subscriptions; // TODO(perf) make an incrementally-updated view-model for InboxPage @@ -111,30 +109,26 @@ class _InboxPageState extends State with PerAccountStoreAwareStateMix } final sortedUnreadStreams = unreadsModel!.streams.entries - .where((entry) => - streams.containsKey(entry.key) // TODO(log) a bug if missing in streams - // Filter out any straggling unreads in unsubscribed streams. - // There won't normally be any, but it happens with certain infrequent - // state changes, typically for less than a few hundred milliseconds. - // See [Unreads]. - // - // Also, we want to depend on the subscription data for things like - // choosing the stream icon. - && subscriptions.containsKey(entry.key)) + // Filter out any straggling unreads in unsubscribed streams. + // There won't normally be any, but it happens with certain infrequent + // state changes, typically for less than a few hundred milliseconds. + // See [Unreads]. + // + // Also, we want to depend on the subscription data for things like + // choosing the stream icon. + .where((entry) => subscriptions.containsKey(entry.key)) .toList() ..sort((a, b) { + final subA = subscriptions[a.key]!; + final subB = subscriptions[b.key]!; + // TODO "pin" icon on the stream row? dividers in the list? - final aPinned = subscriptions[a.key]!.pinToTop; - final bPinned = subscriptions[b.key]!.pinToTop; - if (aPinned != bPinned) { - return aPinned ? -1 : 1; + if (subA.pinToTop != subB.pinToTop) { + return subA.pinToTop ? -1 : 1; } - final streamA = streams[a.key]!; - final streamB = streams[b.key]!; - // TODO(i18n) something like JS's String.prototype.localeCompare - return streamA.name.toLowerCase().compareTo(streamB.name.toLowerCase()); + return subA.name.toLowerCase().compareTo(subB.name.toLowerCase()); }); for (final MapEntry(key: streamId, value: topics) in sortedUnreadStreams) {