From 56964b144a4105bfbdecfb2c7bc1f12d50854519 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 21 Nov 2023 13:11:20 -0500 Subject: [PATCH 1/6] content [nfc]: Move tryParseEmojiCodeToUnicode out to toplevel, to export --- lib/model/content.dart | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/lib/model/content.dart b/lib/model/content.dart index 99637fcc4b..0200bcfb61 100644 --- a/lib/model/content.dart +++ b/lib/model/content.dart @@ -495,6 +495,27 @@ class ImageEmojiNode extends EmojiNode { //////////////////////////////////////////////////////////////// +// Ported from https://github.com/zulip/zulip-mobile/blob/c979530d6804db33310ed7d14a4ac62017432944/src/emoji/data.js#L108-L112 +// +// Which was in turn ported from https://github.com/zulip/zulip/blob/63c9296d5339517450f79f176dc02d77b08020c8/zerver/models.py#L3235-L3242 +// and that describes the encoding as follows: +// +// > * For Unicode emoji, [emoji_code is] a dash-separated hex encoding of +// > the sequence of Unicode codepoints that define this emoji in the +// > Unicode specification. For examples, see "non_qualified" or +// > "unified" in the following data, with "non_qualified" taking +// > precedence when both present: +// > https://raw.githubusercontent.com/iamcal/emoji-data/master/emoji_pretty.json +String? tryParseEmojiCodeToUnicode(String code) { + try { + return String.fromCharCodes(code.split('-').map((hex) => int.parse(hex, radix: 16))); + } on FormatException { // thrown by `int.parse` + return null; + } on ArgumentError { // thrown by `String.fromCharCodes` + return null; + } +} + /// What sort of nodes a [_ZulipContentParser] is currently expecting to find. enum _ParserContext { /// The parser is currently looking for block nodes. @@ -525,27 +546,6 @@ class _ZulipContentParser { static final _emojiClassRegexp = RegExp(r"^emoji(-[0-9a-f]+)*$"); - // Ported from https://github.com/zulip/zulip-mobile/blob/c979530d6804db33310ed7d14a4ac62017432944/src/emoji/data.js#L108-L112 - // - // Which was in turn ported from https://github.com/zulip/zulip/blob/63c9296d5339517450f79f176dc02d77b08020c8/zerver/models.py#L3235-L3242 - // and that describes the encoding as follows: - // - // > * For Unicode emoji, [emoji_code is] a dash-separated hex encoding of - // > the sequence of Unicode codepoints that define this emoji in the - // > Unicode specification. For examples, see "non_qualified" or - // > "unified" in the following data, with "non_qualified" taking - // > precedence when both present: - // > https://raw.githubusercontent.com/iamcal/emoji-data/master/emoji_pretty.json - String? tryParseEmojiCodeToUnicode(String code) { - try { - return String.fromCharCodes(code.split('-').map((hex) => int.parse(hex, radix: 16))); - } on FormatException { // thrown by `int.parse` - return null; - } on ArgumentError { // thrown by `String.fromCharCodes` - return null; - } - } - InlineContentNode parseInlineContent(dom.Node node) { assert(_debugParserContext == _ParserContext.inline); final debugHtmlNode = kDebugMode ? node : null; From 6354f28719b3078f5b7520f05c4e13ed9a8a59dc Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 30 Nov 2023 02:16:24 -0500 Subject: [PATCH 2/6] test: Add userSettings default to eg.initialSnapshot --- test/example_data.dart | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/example_data.dart b/test/example_data.dart index 42db6b9947..60fde919c7 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -450,7 +450,11 @@ InitialSnapshot initialSnapshot({ subscriptions: subscriptions ?? [], // TODO add subscriptions to default unreadMsgs: unreadMsgs ?? _unreadMsgs(), streams: streams ?? [], // TODO add streams to default - userSettings: userSettings, // TODO add userSettings to default + userSettings: userSettings ?? UserSettings( + twentyFourHourTime: false, + displayEmojiReactionUsers: true, + emojiset: Emojiset.google, + ), realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {}, maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, realmUsers: realmUsers ?? [], From 6337a98a76dd9da333cec682fa28719b2241ef47 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 14 Aug 2023 17:47:38 -0500 Subject: [PATCH 3/6] msglist: Show message reactions! And also support: - removing a reaction you've already made, and - joining in on existing reactions that other people have made. As is our habit with the message list, this aims to be faithful to the web app, as accessed today. That should be a good baseline to make mobile-specific adjustments from. (In particular I think we may want larger touch targets.) Unlike the web app, we use a font instead of a sprite sheet to render Unicode emoji. This means that we, unlike web, have to account for text-layout algorithms, and things like font metrics. So if Unicode emoji appear noticeably differently from web, that probably has something to do with it. Fixes: #121 Fixes-partly: #125 --- lib/widgets/emoji_reaction.dart | 368 ++++++++++++++++++++++++++ lib/widgets/message_list.dart | 3 + test/widgets/emoji_reaction_test.dart | 225 ++++++++++++++++ 3 files changed, 596 insertions(+) create mode 100644 lib/widgets/emoji_reaction.dart create mode 100644 test/widgets/emoji_reaction_test.dart diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart new file mode 100644 index 0000000000..dd58ba26ec --- /dev/null +++ b/lib/widgets/emoji_reaction.dart @@ -0,0 +1,368 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter/material.dart'; + +import '../api/model/initial_snapshot.dart'; +import '../api/model/model.dart'; +import '../api/route/messages.dart'; +import '../model/content.dart'; +import 'content.dart'; +import 'store.dart'; +import 'text.dart'; + +class ReactionChipsList extends StatelessWidget { + const ReactionChipsList({ + super.key, + required this.messageId, + required this.reactions, + }); + + final int messageId; + final Reactions reactions; + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final displayEmojiReactionUsers = store.userSettings?.displayEmojiReactionUsers ?? false; + final showNames = displayEmojiReactionUsers && reactions.total <= 3; + + return Wrap(spacing: 4, runSpacing: 4, crossAxisAlignment: WrapCrossAlignment.center, + children: reactions.aggregated.map((reactionVotes) => ReactionChip( + showName: showNames, + messageId: messageId, reactionWithVotes: reactionVotes), + ).toList()); + } +} + +final _textColorSelected = const HSLColor.fromAHSL(1, 210, 0.20, 0.20).toColor(); +final _textColorUnselected = const HSLColor.fromAHSL(1, 210, 0.20, 0.25).toColor(); + +const _backgroundColorSelected = Colors.white; +// TODO shadow effect, following web, which uses `box-shadow: inset`: +// https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow#inset +// Needs Flutter support for something like that: +// https://github.com/flutter/flutter/issues/18636 +// https://github.com/flutter/flutter/issues/52999 +// Until then use a solid color; a much-lightened version of the shadow color. +// Also adapt by making [_borderColorUnselected] more transparent, so we'll +// want to check that against web when implementing the shadow. +final _backgroundColorUnselected = const HSLColor.fromAHSL(0.15, 210, 0.50, 0.875).toColor(); + +final _borderColorSelected = Colors.black.withOpacity(0.40); +// TODO see TODO on [_backgroundColorUnselected] about shadow effect +final _borderColorUnselected = Colors.black.withOpacity(0.06); + +class ReactionChip extends StatelessWidget { + final bool showName; + final int messageId; + final ReactionWithVotes reactionWithVotes; + + const ReactionChip({ + super.key, + required this.showName, + required this.messageId, + required this.reactionWithVotes, + }); + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + + final reactionType = reactionWithVotes.reactionType; + final emojiCode = reactionWithVotes.emojiCode; + final emojiName = reactionWithVotes.emojiName; + final userIds = reactionWithVotes.userIds; + + final emojiset = store.userSettings?.emojiset ?? Emojiset.google; + + final selfUserId = store.account.userId; + final selfVoted = userIds.contains(selfUserId); + final label = showName + // TODO(i18n): List formatting, like you can do in JavaScript: + // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu']) + // // 'Chris、Greg、Alya、Shu' + ? userIds.map((id) { + return id == selfUserId + ? 'You' + : store.users[id]?.fullName ?? '(unknown user)'; // TODO(i18n) + }).join(', ') + : userIds.length.toString(); + + final borderColor = selfVoted ? _borderColorSelected : _borderColorUnselected; + final labelColor = selfVoted ? _textColorSelected : _textColorUnselected; + final backgroundColor = selfVoted ? _backgroundColorSelected : _backgroundColorUnselected; + final splashColor = selfVoted ? _backgroundColorUnselected : _backgroundColorSelected; + final highlightColor = splashColor.withOpacity(0.5); + + final borderSide = BorderSide(color: borderColor, width: 1); + final shape = StadiumBorder(side: borderSide); + + final Widget emoji; + if (emojiset == Emojiset.text) { + emoji = _TextEmoji(emojiName: emojiName, selected: selfVoted); + } else { + switch (reactionType) { + case ReactionType.unicodeEmoji: + emoji = _UnicodeEmoji( + emojiCode: emojiCode, + emojiName: emojiName, + selected: selfVoted, + ); + case ReactionType.realmEmoji: + case ReactionType.zulipExtraEmoji: + emoji = _ImageEmoji( + emojiCode: emojiCode, + emojiName: emojiName, + selected: selfVoted, + ); + } + } + + return Tooltip( + // TODO(#434): Semantics with eg "Reaction: ; you and N others: " + excludeFromSemantics: true, + message: emojiName, + child: Material( + color: backgroundColor, + shape: shape, + child: InkWell( + customBorder: shape, + splashColor: splashColor, + highlightColor: highlightColor, + onTap: () { + (selfVoted ? removeReaction : addReaction).call(store.connection, + messageId: messageId, + reactionType: reactionType, + emojiCode: emojiCode, + emojiName: emojiName, + ); + }, + child: Padding( + // 1px of this padding accounts for the border, which Flutter + // just paints without changing size. + padding: const EdgeInsetsDirectional.fromSTEB(4, 2, 5, 2), + child: LayoutBuilder( + builder: (context, constraints) { + final maxRowWidth = constraints.maxWidth; + // To give text emojis some room so they need fewer line breaks + // when the label is long. + // TODO(#433) This is a bit overzealous. The shorter width + // won't be necessary when the text emoji is very short, or + // in the near-universal case of small, square emoji (i.e. + // Unicode and image emoji). But it's not simple to recognize + // those cases here: we don't know at this point whether we'll + // be showing a text emoji, because we use that for various + // error conditions (including when an image fails to load, + // which we learn about especially late). + final maxLabelWidth = (maxRowWidth - 6) * 0.75; // 6 is padding + + return Row( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.center, + children: [ + // So text-emoji chips are at least as tall as square-emoji + // ones (probably a good thing). + SizedBox(height: _squareEmojiScalerClamped(context).scale(_squareEmojiSize)), + Flexible( // [Flexible] to let text emojis expand if they can + child: Padding(padding: const EdgeInsets.symmetric(horizontal: 3, vertical: 1), + child: emoji)), + // Added vertical: 1 to give some space when the label is + // taller than the emoji (e.g. because it needs multiple lines) + Padding(padding: const EdgeInsets.symmetric(horizontal: 3, vertical: 1), + child: Container( + constraints: BoxConstraints(maxWidth: maxLabelWidth), + child: Text( + textWidthBasis: TextWidthBasis.longestLine, + textScaler: _labelTextScalerClamped(context), + style: TextStyle( + fontFamily: 'Source Sans 3', + fontSize: (14 * 0.90), + height: 13 / (14 * 0.90), + color: labelColor, + ).merge(selfVoted + ? weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900) + : weightVariableTextStyle(context)), + label), + )), + ]); + }))))); + } +} + +/// The size of a square emoji (Unicode or image). +/// +/// Should be scaled by [_emojiTextScalerClamped]. +const _squareEmojiSize = 17.0; + +/// A font size that, with Noto Color Emoji and our line-height config, +/// causes a Unicode emoji to occupy a [_squareEmojiSize] square in the layout. +/// +/// Determined experimentally: +/// +// TODO(#404) Actually bundle Noto Color Emoji with the app. Some Android +// phones use Noto Color Emoji automatically, and some don't; e.g., Samsung +// has its own emoji font: +// +const _notoColorEmojiTextSize = 14.5; + +/// A [TextScaler] that limits Unicode and image emojis' max scale factor, +/// to leave space for the label. +/// +/// This should scale [_squareEmojiSize] for Unicode and image emojis. +// TODO(a11y) clamp higher? +TextScaler _squareEmojiScalerClamped(BuildContext context) => + MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 2); + +/// A [TextScaler] that limits text emojis' max scale factor, +/// to minimize the need for line breaks. +// TODO(a11y) clamp higher? +TextScaler _textEmojiScalerClamped(BuildContext context) => + MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 1.5); + +/// A [TextScaler] that limits the label's max scale factor, +/// to minimize the need for line breaks. +// TODO(a11y) clamp higher? +TextScaler _labelTextScalerClamped(BuildContext context) => + MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 2); + +class _UnicodeEmoji extends StatelessWidget { + const _UnicodeEmoji({ + required this.emojiCode, + required this.emojiName, + required this.selected, + }); + + final String emojiCode; + final String emojiName; + final bool selected; + + @override + Widget build(BuildContext context) { + final parsed = tryParseEmojiCodeToUnicode(emojiCode); + if (parsed == null) { // TODO(log) + return _TextEmoji(emojiName: emojiName, selected: selected); + } + + switch (defaultTargetPlatform) { + case TargetPlatform.android: + case TargetPlatform.fuchsia: + case TargetPlatform.linux: + case TargetPlatform.windows: + return Text( + textScaler: _squareEmojiScalerClamped(context), + style: const TextStyle(fontSize: _notoColorEmojiTextSize), + strutStyle: const StrutStyle(fontSize: _notoColorEmojiTextSize, forceStrutHeight: true), + parsed); + case TargetPlatform.iOS: + case TargetPlatform.macOS: + // We expect the font "Apple Color Emoji" to be used. There are some + // surprises in how Flutter ends up rendering emojis in this font: + // - With a font size of 17px, the emoji visually seems to be about 17px + // square. (Unlike on Android, with Noto Color Emoji, where a 14.5px font + // size gives an emoji that looks 17px square.) See: + // + // - The emoji doesn't fill the space taken by the [Text] in the layout. + // There's whitespace above, below, and on the right. See: + // + // + // That extra space would be problematic, except we've used a [Stack] to + // make the [Text] "positioned" so the space doesn't add margins around the + // visible part. Key points that enable the [Stack] workaround: + // - The emoji seems approximately vertically centered (this is + // accomplished with help from a [StrutStyle]; see below). + // - There seems to be approximately no space on its left. + final boxSize = _squareEmojiScalerClamped(context).scale(_squareEmojiSize); + return Stack(alignment: Alignment.centerLeft, clipBehavior: Clip.none, children: [ + SizedBox(height: boxSize, width: boxSize), + PositionedDirectional(start: 0, child: Text( + textScaler: _squareEmojiScalerClamped(context), + style: const TextStyle(fontSize: _squareEmojiSize), + strutStyle: const StrutStyle(fontSize: _squareEmojiSize, forceStrutHeight: true), + parsed)), + ]); + } + } +} + +class _ImageEmoji extends StatelessWidget { + const _ImageEmoji({ + required this.emojiCode, + required this.emojiName, + required this.selected, + }); + + final String emojiCode; + final String emojiName; + final bool selected; + + Widget get _textFallback => _TextEmoji(emojiName: emojiName, selected: selected); + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + + // Some people really dislike animated emoji. + final doNotAnimate = + // From reading code, this doesn't actually get set on iOS: + // https://github.com/zulip/zulip-flutter/pull/410#discussion_r1408522293 + MediaQuery.disableAnimationsOf(context) + || (defaultTargetPlatform == TargetPlatform.iOS + // TODO(upstream) On iOS 17+ (new in 2023), there's a more closely + // relevant setting than "reduce motion". It's called "auto-play + // animated images", and we should file an issue to expose it. + // See GitHub comment linked above. + && WidgetsBinding.instance.platformDispatcher.accessibilityFeatures.reduceMotion); + + final String src; + switch (emojiCode) { + case 'zulip': // the single "zulip extra emoji" + src = '/static/generated/emoji/images/emoji/unicode/zulip.png'; + default: + final item = store.realmEmoji[emojiCode]; + if (item == null) { + return _textFallback; + } + src = doNotAnimate && item.stillUrl != null ? item.stillUrl! : item.sourceUrl; + } + final parsedSrc = Uri.tryParse(src); + if (parsedSrc == null) { // TODO(log) + return _textFallback; + } + final resolved = store.account.realmUrl.resolveUri(parsedSrc); + + // Unicode and text emoji get scaled; it would look weird if image emoji didn't. + final size = _squareEmojiScalerClamped(context).scale(_squareEmojiSize); + + return RealmContentNetworkImage( + resolved, + width: size, + height: size, + errorBuilder: (context, _, __) => _textFallback, + ); + } +} + +class _TextEmoji extends StatelessWidget { + const _TextEmoji({required this.emojiName, required this.selected}); + + final String emojiName; + final bool selected; + + @override + Widget build(BuildContext context) { + return Text( + textAlign: TextAlign.end, + textScaler: _textEmojiScalerClamped(context), + style: TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 14 * 0.8, + height: 1, // to be denser when we have to wrap + color: selected ? _textColorSelected : _textColorUnselected, + ).merge(selected + ? weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900) + : weightVariableTextStyle(context)), + // Encourage line breaks before "_" (common in these), but try not + // to leave a colon alone on a line. See: + // + ':\ufeff${emojiName.replaceAll('_', '\u200b_')}\ufeff:'); + } +} diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 0504e69789..254b55ac21 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -16,6 +16,7 @@ import 'action_sheet.dart'; import 'compose_box.dart'; import 'content.dart'; import 'dialog.dart'; +import 'emoji_reaction.dart'; import 'icons.dart'; import 'page.dart'; import 'profile.dart'; @@ -775,6 +776,8 @@ class MessageWithPossibleSender extends StatelessWidget { const SizedBox(height: 4), ], MessageContent(message: message, content: item.content), + if ((message.reactions?.total ?? 0) > 0) + ReactionChipsList(messageId: message.id, reactions: message.reactions!) ])), Container( width: 80, diff --git a/test/widgets/emoji_reaction_test.dart b/test/widgets/emoji_reaction_test.dart new file mode 100644 index 0000000000..27c9b92b0c --- /dev/null +++ b/test/widgets/emoji_reaction_test.dart @@ -0,0 +1,225 @@ +import 'dart:io' as io; +import 'dart:io'; + +import 'package:flutter/foundation.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/initial_snapshot.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/emoji_reaction.dart'; +import 'package:zulip/widgets/store.dart'; + +import '../example_data.dart' as eg; +import '../model/binding.dart'; +import '../model/test_store.dart'; +import '../test_images.dart'; + +void main() { + TestZulipBinding.ensureInitialized(); + + group('ReactionChipsList', () { + late PerAccountStore store; + + Future prepare() async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + + store.addUser(eg.selfUser); + + // TODO do this more centrally, or put in reusable helper + final Future font = rootBundle.load('assets/Source_Sans_3/SourceSans3VF-Upright.otf'); + final fontLoader = FontLoader('Source Sans 3')..addFont(font); + await fontLoader.load(); + } + + // From trying the options on an iPhone 13 Pro running iOS 16.6.1: + const textScaleFactors = [ + 0.8235, // smallest + 1, + 1.3529, // largest without using the "Larger Accessibility Sizes" setting + 3.1176, // largest + ]; + + Future setupChipsInBox(WidgetTester tester, { + required List reactions, + double? width, + TextDirection? textDirection, + }) async { + final message = eg.streamMessage(reactions: reactions); + + await tester.pumpWidget( + MaterialApp( + home: Directionality( + textDirection: textDirection ?? TextDirection.ltr, + child: GlobalStoreWidget( + child: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: Center( + child: ColoredBox( + color: Colors.white, + child: SizedBox( + width: width ?? 245.0, // (seen in context on an iPhone 13 Pro) + child: ReactionChipsList( + messageId: message.id, + reactions: message.reactions!, + ))))))))); + + // global store, per-account store + await tester.pumpAndSettle(); + } + + // Smoke tests under various conditions. + for (final displayEmojiReactionUsers in [true, false]) { + for (final emojiset in [Emojiset.text, Emojiset.google]) { + for (final textDirection in TextDirection.values) { + for (final textScaleFactor in textScaleFactors) { + Future runSmokeTest( + String description, + List reactions, { + required List users, + required Map realmEmoji, + }) async { + final descriptionDetails = [ + displayEmojiReactionUsers ? 'show names when few' : 'no names', + emojiset.name, + textDirection.name, + 'text scale: $textScaleFactor', + ].join(' / '); + testWidgets('smoke ($description): $descriptionDetails', (tester) async { + // Skip iOS. We're not covering the iOS code, for now, because it + // contains a workaround for layout issues that we think will only + // reproduce on actual iOS, and not in this test environment: + // + // If those layout issues get fixed and we want to cover + // TargetPlatform.iOS, remember that we suspect the Apple Color + // Emoji font only works on Apple platforms, so for any tests + // aimed at iOS, we should only run them on macOS. + // TODO Could do an on-device integration test, which would let us + // cover iOS before a layout fix lands upstream: + // + debugDefaultTargetPlatformOverride = TargetPlatform.android; + + tester.platformDispatcher.textScaleFactorTestValue = textScaleFactor; + addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); + + await prepare(); + + store + ..addUsers(users) + ..handleEvent(RealmEmojiUpdateEvent(id: 1, + realmEmoji: realmEmoji)) + ..handleEvent(UserSettingsUpdateEvent(id: 1, + property: UserSettingName.displayEmojiReactionUsers, + value: displayEmojiReactionUsers)) + ..handleEvent(UserSettingsUpdateEvent(id: 1, + property: UserSettingName.emojiset, + value: emojiset)); + + // This does mean that all image emoji will look the same… + // shrug, at least for now. + final httpClient = FakeImageHttpClient(); + debugNetworkImageHttpClientProvider = () => httpClient; + httpClient.request.response + ..statusCode = HttpStatus.ok + ..content = kSolidBlueAvatar; + + await setupChipsInBox(tester, textDirection: textDirection, reactions: reactions); + + // TODO(upstream) Do these in an addTearDown, once we can: + // https://github.com/flutter/flutter/issues/123189 + debugDefaultTargetPlatformOverride = null; + debugNetworkImageHttpClientProvider = null; + }, + // The Android code for Unicode emojis can't be exercised faithfully + // on a Mac, because Noto Color Emoji can't work there: + // + // So, skip on macOS. + skip: io.Platform.isMacOS); + } + + // Base JSON for various unicode emoji reactions. Just missing user_id. + final u1 = {'emoji_name': '+1', 'emoji_code': '1f44d', 'reaction_type': 'unicode_emoji'}; + final u2 = {'emoji_name': 'family_man_man_girl_boy', 'emoji_code': '1f468-200d-1f468-200d-1f467-200d-1f466', 'reaction_type': 'unicode_emoji'}; + final u3 = {'emoji_name': 'smile', 'emoji_code': '1f642', 'reaction_type': 'unicode_emoji'}; + final u4 = {'emoji_name': 'tada', 'emoji_code': '1f389', 'reaction_type': 'unicode_emoji'}; + final u5 = {'emoji_name': 'exploding_head', 'emoji_code': '1f92f', 'reaction_type': 'unicode_emoji'}; + + // Base JSON for various realm-emoji reactions. Just missing user_id. + final i1 = {'emoji_name': 'twocents', 'emoji_code': '181', 'reaction_type': 'realm_emoji'}; + final i2 = {'emoji_name': 'threecents', 'emoji_code': '182', 'reaction_type': 'realm_emoji'}; + + // Base JSON for the one "Zulip extra emoji" reaction. Just missing user_id. + final z1 = {'emoji_name': 'zulip', 'emoji_code': 'zulip', 'reaction_type': 'zulip_extra_emoji'}; + + final user1 = eg.user(fullName: 'abc'); + final user2 = eg.user(fullName: 'Long Name With Many Words In It'); + final user3 = eg.user(fullName: 'longnamelongnamelongnamelongname'); + final user4 = eg.user(); + final user5 = eg.user(); + + final users = [user1, user2, user3, user4, user5]; + + final realmEmoji = { + '181': RealmEmojiItem(id: '181', name: 'twocents', authorId: 7, + deactivated: false, sourceUrl: '/foo/2', stillUrl: null), + '182': RealmEmojiItem(id: '182', name: 'threecents', authorId: 7, + deactivated: false, sourceUrl: '/foo/3', stillUrl: null), + }; + + runSmokeTest('same reaction, different users, with one unknown user', [ + Reaction.fromJson({ ...u1, 'user_id': user1.userId}), + Reaction.fromJson({ ...u1, 'user_id': user2.userId}), + // unknown user; shouldn't crash (name should show as "(unknown user)") + Reaction.fromJson({ ...u1, 'user_id': eg.user().userId}), + ], users: users, realmEmoji: realmEmoji); + + runSmokeTest('same user on different reactions', [ + Reaction.fromJson({ ...u1, 'user_id': user2.userId}), + Reaction.fromJson({ ...u2, 'user_id': user2.userId}), + Reaction.fromJson({ ...u3, 'user_id': user2.userId}), + ], users: users, realmEmoji: realmEmoji); + + runSmokeTest('self user', [ + Reaction.fromJson({ ...i1, 'user_id': eg.selfUser.userId}), + Reaction.fromJson({ ...i2, 'user_id': user1.userId}), + ], users: users, realmEmoji: realmEmoji); + + runSmokeTest('different [ReactionType]s', [ + Reaction.fromJson({ ...u1, 'user_id': user1.userId}), + Reaction.fromJson({ ...i1, 'user_id': user2.userId}), + Reaction.fromJson({ ...z1, 'user_id': user3.userId}), + ], users: users, realmEmoji: realmEmoji); + + runSmokeTest('many, varied', [ + Reaction.fromJson({ ...u1, 'user_id': user1.userId}), + Reaction.fromJson({ ...u1, 'user_id': user2.userId}), + Reaction.fromJson({ ...u2, 'user_id': user2.userId}), + Reaction.fromJson({ ...u3, 'user_id': user3.userId}), + Reaction.fromJson({ ...u4, 'user_id': user4.userId}), + Reaction.fromJson({ ...u5, 'user_id': user4.userId}), + Reaction.fromJson({ ...u5, 'user_id': user5.userId}), + Reaction.fromJson({ ...i1, 'user_id': user5.userId}), + Reaction.fromJson({ ...z1, 'user_id': user5.userId}), + Reaction.fromJson({ ...u5, 'user_id': eg.selfUser.userId}), + Reaction.fromJson({ ...i1, 'user_id': eg.selfUser.userId}), + Reaction.fromJson({ ...z1, 'user_id': eg.selfUser.userId}), + ], users: users, realmEmoji: realmEmoji); + } + } + } + } + }); + + // TODO more tests: + // - Tapping a chip does the right thing + // - When an image emoji fails to load, falls back to :text_emoji: + // - Label text correctly chooses names or number + // - When a user isn't found, says "(unknown user)" + // - More about layout? (not just that it's error-free) + // - Non-animated image emoji is selected when intended +} From 8529e1449d9caed2f26b6dc943790043f52b0f02 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 28 Nov 2023 22:24:07 -0500 Subject: [PATCH 4/6] action_sheet test: Remove unnecessary binding setup These calls only need to happen once, at the top of `main`: https://github.com/zulip/zulip-flutter/pull/410#discussion_r1408418545 --- test/widgets/action_sheet_test.dart | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index cc8bedb3a9..4cde45185a 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -69,6 +69,7 @@ Future setupToMessageActionSheet(WidgetTester tester, { void main() { TestZulipBinding.ensureInitialized(); + TestWidgetsFlutterBinding.ensureInitialized(); void prepareRawContentResponseSuccess(PerAccountStore store, { required Message message, @@ -91,12 +92,7 @@ void main() { } group('ShareButton', () { - // Tests should call setupMockSharePlus. - setUp(() async { - TestZulipBinding.ensureInitialized(); - TestWidgetsFlutterBinding.ensureInitialized(); - }); - + // Tests should call this. MockSharePlus setupMockSharePlus() { final mock = MockSharePlus(); TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( @@ -304,8 +300,6 @@ void main() { group('CopyButton', () { setUp(() async { - TestZulipBinding.ensureInitialized(); - TestWidgetsFlutterBinding.ensureInitialized(); TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( SystemChannels.platform, MockClipboard().handleMethodCall, From 12029ee91c96090d6c169e818171472c6512e131 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 28 Nov 2023 22:25:11 -0500 Subject: [PATCH 5/6] action_sheet test: Remove unnecessary teardowns This teardown is already added in setupToMessageActionSheet, which is called near the top of every test. --- test/widgets/action_sheet_test.dart | 8 -------- 1 file changed, 8 deletions(-) diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 4cde45185a..2aaf817826 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -102,10 +102,6 @@ void main() { return mock; } - tearDown(() async { - testBinding.reset(); - }); - Future tapShareButton(WidgetTester tester) async { await tester.ensureVisible(find.byIcon(Icons.adaptive.share, skipOffstage: false)); await tester.tap(find.byIcon(Icons.adaptive.share)); @@ -306,10 +302,6 @@ void main() { ); }); - tearDown(() async { - testBinding.reset(); - }); - Future tapCopyButton(WidgetTester tester) async { await tester.ensureVisible(find.byIcon(Icons.copy, skipOffstage: false)); await tester.tap(find.byIcon(Icons.copy)); From 8f43bc6b05d87f5b4d5ad3d9ce3acfd4d5b5a7e1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 22 Nov 2023 15:43:25 -0500 Subject: [PATCH 6/6] msglist: Support adding a thumbs-up reaction Fixes: #125 --- lib/widgets/action_sheet.dart | 55 +++++++++++++++++++++++++++++ test/widgets/action_sheet_test.dart | 51 ++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 29ac03abdb..a35cb08f3b 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -17,15 +17,27 @@ import 'store.dart'; /// /// Must have a [MessageListPage] ancestor. void showMessageActionSheet({required BuildContext context, required Message message}) { + final store = PerAccountStoreWidget.of(context); + // The UI that's conditioned on this won't live-update during this appearance // of the action sheet (we avoid calling composeBoxControllerOf in a build // method; see its doc). But currently it will be constant through the life of // any message list, so that's fine. final isComposeBoxOffered = MessageListPage.composeBoxControllerOf(context) != null; + + final selfUserId = store.account.userId; + final hasThumbsUpReactionVote = message.reactions + ?.aggregated.any((reactionWithVotes) => + reactionWithVotes.reactionType == ReactionType.unicodeEmoji + && reactionWithVotes.emojiCode == '1f44d' + && reactionWithVotes.userIds.contains(selfUserId)) + ?? false; + showDraggableScrollableModalBottomSheet( context: context, builder: (BuildContext _) { return Column(children: [ + if (!hasThumbsUpReactionVote) AddThumbsUpButton(message: message, messageListContext: context), ShareButton(message: message, messageListContext: context), if (isComposeBoxOffered) QuoteAndReplyButton( message: message, @@ -60,6 +72,49 @@ abstract class MessageActionSheetMenuItemButton extends StatelessWidget { } } +// This button is very temporary, to complete #125 before we have a way to +// choose an arbitrary reaction (#388). So, skipping i18n. +class AddThumbsUpButton extends MessageActionSheetMenuItemButton { + AddThumbsUpButton({ + super.key, + required super.message, + required super.messageListContext, + }); + + @override get icon => Icons.add_reaction_outlined; + + @override + String label(ZulipLocalizations zulipLocalizations) { + return 'React with 👍'; // TODO(i18n) skip translation for now + } + + @override get onPressed => (BuildContext context) async { + Navigator.of(context).pop(); + String? errorMessage; + try { + await addReaction(PerAccountStoreWidget.of(messageListContext).connection, + messageId: message.id, + reactionType: ReactionType.unicodeEmoji, + emojiCode: '1f44d', + emojiName: '+1', + ); + } catch (e) { + if (!messageListContext.mounted) return; + + switch (e) { + case ZulipApiException(): + errorMessage = e.message; + // TODO specific messages for common errors, like network errors + // (support with reusable code) + default: + } + + await showErrorDialog(context: context, + title: 'Adding reaction failed', message: errorMessage); + } + }; +} + class ShareButton extends MessageActionSheetMenuItemButton { ShareButton({ super.key, diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 2aaf817826..992dbf87cf 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -3,6 +3,7 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/compose.dart'; @@ -19,6 +20,7 @@ import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; import '../model/test_store.dart'; +import '../stdlib_checks.dart'; import '../test_clipboard.dart'; import '../test_share_plus.dart'; import 'compose_box_checks.dart'; @@ -91,6 +93,54 @@ void main() { (store.connection as FakeApiConnection).prepare(httpStatus: 400, json: fakeResponseJson); } + group('AddThumbsUpButton', () { + Future tapButton(WidgetTester tester) async { + await tester.ensureVisible(find.byIcon(Icons.add_reaction_outlined, skipOffstage: false)); + await tester.tap(find.byIcon(Icons.add_reaction_outlined)); + await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e + } + + testWidgets('success', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + + final connection = store.connection as FakeApiConnection; + connection.prepare(json: {}); + await tapButton(tester); + await tester.pump(Duration.zero); + + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/${message.id}/reactions') + ..bodyFields.deepEquals({ + 'reaction_type': 'unicode_emoji', + 'emoji_code': '1f44d', + 'emoji_name': '+1', + }); + }); + + testWidgets('request has an error', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + + final connection = store.connection as FakeApiConnection; + + connection.prepare(httpStatus: 400, json: { + 'code': 'BAD_REQUEST', + 'msg': 'Invalid message(s)', + 'result': 'error', + }); + await tapButton(tester); + await tester.pump(Duration.zero); // error arrives; error dialog shows + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Adding reaction failed', + expectedMessage: 'Invalid message(s)'))); + }); + }); + group('ShareButton', () { // Tests should call this. MockSharePlus setupMockSharePlus() { @@ -169,6 +219,7 @@ void main() { /// /// Checks that there is a quote-and-reply button. Future tapQuoteAndReplyButton(WidgetTester tester) async { + await tester.ensureVisible(find.byIcon(Icons.format_quote_outlined, skipOffstage: false)); final quoteAndReplyButton = findQuoteAndReplyButton(tester); check(quoteAndReplyButton).isNotNull(); await tester.tap(find.byWidget(quoteAndReplyButton!));