Skip to content

fix(emoji): Adjust picker layout for plain-text-emojis setting #1587 #1759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(emoji): Adjust picker layout for plain-text-emojis setting #1587 #1759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions lib/widgets/autocomplete.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ class _EmojiAutocompleteItem extends StatelessWidget {
ImageEmojiWidget(size: _size, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>
UnicodeEmojiWidget(size: _size, emojiDisplay: emojiDisplay),
TextEmojiDisplay() => null, // The text is already shown separately.
TextEmojiDisplay() => null,
};

final label = candidate.aliases.isEmpty
Expand All @@ -395,11 +395,12 @@ class _EmojiAutocompleteItem extends StatelessWidget {
return Padding(
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
child: Row(children: [
if (glyph != null) ...[
if (glyph != null)
Padding(padding: const EdgeInsets.all(6),
child: glyph),
const SizedBox(width: 6),
],
child: glyph)
else
const SizedBox(height: 36.0, width: 0),
const SizedBox(width: 6),
Expanded(
child: Text(
style: TextStyle(fontSize: 17, height: 18 / 17,
Expand Down
92 changes: 48 additions & 44 deletions lib/widgets/emoji_reaction.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,49 +597,53 @@ class EmojiPickerListEntry extends StatelessWidget {
Navigator.pop(pageContext, emoji);
}

@override
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
final designVariables = DesignVariables.of(context);

// TODO deduplicate this logic with [_EmojiAutocompleteItem]
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
final Widget? glyph = switch (emojiDisplay) {
ImageEmojiDisplay() =>
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>
UnicodeEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
TextEmojiDisplay() => null, // The text is already shown separately.
};

final label = emoji.aliases.isEmpty
? emoji.emojiName
: [emoji.emojiName, ...emoji.aliases].join(", "); // TODO(#1080)

return InkWell(
onTap: _onPressed,
splashFactory: NoSplash.splashFactory,
overlayColor: WidgetStateColor.resolveWith((states) =>
states.any((e) => e == WidgetState.pressed)
? designVariables.contextMenuItemBg.withFadedAlpha(0.20)
: Colors.transparent),
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 8),
child: Row(spacing: 4, children: [
if (glyph != null)
Padding(
padding: const EdgeInsets.all(10),
child: glyph),
Flexible(child: Text(label,
maxLines: 2,
overflow: TextOverflow.ellipsis,
style: TextStyle(
fontSize: 17,
height: 18 / 17,
color: designVariables.textMessage)))
]),
));
}
@override
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
final designVariables = DesignVariables.of(context);

// TODO deduplicate this logic with [_EmojiAutocompleteItem]
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
final Widget? glyph = switch (emojiDisplay) {
ImageEmojiDisplay() =>
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>
UnicodeEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
TextEmojiDisplay() => null,// The text is already shown separately.
};

final label = emoji.aliases.isEmpty
? emoji.emojiName
: [emoji.emojiName, ...emoji.aliases].join(", ");// TODO(#1080)

return InkWell(
onTap: _onPressed,
splashFactory: NoSplash.splashFactory,
overlayColor: WidgetStateColor.resolveWith((states) =>
states.any((e) => e == WidgetState.pressed)
? designVariables.contextMenuItemBg.withFadedAlpha(0.20)
: Colors.transparent),
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 8),
child: Row(spacing: 4, children: [
if (glyph != null)
Padding(
padding: const EdgeInsets.symmetric(vertical: 10.0),
child: glyph)
else
SizedBox(
height: _emojiSize + 8.0 + 8.0,
),
Flexible(child: Text(label,
maxLines: 2,
overflow: TextOverflow.ellipsis,
style: TextStyle(
fontSize: 17,
height: 18 / 17,
color: designVariables.textMessage)))
]),
));
}
}

/// Opens a bottom sheet showing who reacted to the message.
Expand Down Expand Up @@ -1084,4 +1088,4 @@ class ViewReactionsUserItem extends StatelessWidget {
store.userDisplayName(userId))),
])));
}
}
}
36 changes: 36 additions & 0 deletions test/widgets/autocomplete_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,42 @@ void main() {
expected ? displaySubject.findsOne(): displaySubject.findsNothing();
}

testWidgets('text emoji autocomplete item must have correct height', (tester) async {
// Regression test for #1587
final composeInputFinder = await setupToComposeInput(tester);

await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: {
'1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'an_image_emoji'),
}));
await tester.pump();

// TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(composeInputFinder, ':an_imag');
await tester.enterText(composeInputFinder, ':an_image');
await tester.pumpAndSettle();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can this just be an await tester.pump()? What do the other tests do?


final imageItemFinder = find.widgetWithText(InkWell, 'an_image_emoji');
check(imageItemFinder).findsOne();
final imageItemHeight = tester.getSize(imageItemFinder).height;

// TODO(#226): Remove this extra edit when this bug is fixed.
await store.handleEvent(UserSettingsUpdateEvent(id: 1,
property: UserSettingName.emojiset,
value: Emojiset.text));

await tester.enterText(composeInputFinder, ':an_imag');
await tester.enterText(composeInputFinder, ':an_image');
await tester.pumpAndSettle();

final textItemFinder = find.widgetWithText(InkWell, 'an_image_emoji');
check(textItemFinder).findsOne();
final textItemHeight = tester.getSize(textItemFinder).height;

check(textItemHeight).isCloseTo(imageItemHeight, 1.0);

debugNetworkImageHttpClientProvider = null;
});

testWidgets('show, update, choose', (tester) async {
final composeInputFinder = await setupToComposeInput(tester);
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
Expand Down
26 changes: 26 additions & 0 deletions test/widgets/emoji_reaction_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,32 @@ void main() {
debugNetworkImageHttpClientProvider = null;
});

testWidgets('text emoji list items must have correct height', (tester) async {
// Regression test for #1587.
final message = eg.streamMessage();
await setupEmojiPicker(tester, message: message, narrow: TopicNarrow.ofMessage(message));

// Find a list entry and get its height with the default image-based emoji.
final listEntryFinder = find.byWidgetPredicate(
(w) => w is EmojiPickerListEntry && w.emoji.emojiName == 'zzz');
await tester.ensureVisible(listEntryFinder);
final imageRenderBox = tester.renderObject<RenderBox>(listEntryFinder);
final imageHeight = imageRenderBox.size.height;

// Change the setting to text-only emoji.
await store.handleEvent(UserSettingsUpdateEvent(id: 1,
property: UserSettingName.emojiset,
value: Emojiset.text));
await tester.pumpAndSettle();

// Find the same list entry again and check that its height has not changed.
await tester.ensureVisible(listEntryFinder);
final textRenderBox = tester.renderObject<RenderBox>(listEntryFinder);
check(textRenderBox.size.height).isCloseTo(imageHeight, 1.0);

debugNetworkImageHttpClientProvider = null;
});

testWidgets('with bottom view padding', (tester) async {
await prepare(tester, viewPadding: FakeViewPadding(bottom: 10));

Expand Down