Skip to content

autocomplete: Implement new design for @-mention autocomplete items #995

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

Merged
merged 7 commits into from
Mar 4, 2025
Merged

autocomplete: Implement new design for @-mention autocomplete items #995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
57871ee
user [nfc]: Move getDisplayEmailFor method to lib/model/store.dart
fombalang Oct 15, 2024
dd900b5
autocomplete test [nfc]: Fix brittle avatar image finder
fombalang Nov 12, 2024
995fdc2
autocomplete test [nfc]: Remove unused store from helper
PIG208 Feb 28, 2025
1aca726
autocomplete test [nfc]: Use findExactly for checks
PIG208 Feb 28, 2025
61b64cd
autocomplete: Implement new design for @-mention autocomplete items
fombalang Oct 9, 2024
f86a893
theme: Fix wrong variable in contextMenuItemText color field
fombalang Nov 9, 2024
2841a13
autocomplete: Update emoji-results style to follow design
gnprice Nov 26, 2024
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
27 changes: 27 additions & 0 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,33 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
return byDate.difference(dateJoined).inDays >= realmWaitingPeriodThreshold;
}

/// The given user's real email address, if known, for displaying in the UI.
///
/// Returns null if self-user isn't able to see [user]'s real email address.
String? userDisplayEmail(User user) {
Copy link
Member

Choose a reason for hiding this comment

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

Logically this belongs on UserStore, but I guess it's in the same boat as hasPassedWaitingPeriod — we'll first need to arrange a good way for UserStore to access the realm settings.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps like Unreads that rely on a reference to a ChannelStoreImpl, we can split out realm settings for UserStore. Probably a good refactor project as a follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, or I'd organize it slightly differently to match the existing way UserStore works: see #1327 (comment) .

(Small correction: Unreads takes a ChannelStore, not a ChannelStoreImpl. Generally the only references to the "impl" classes are in PerAccountStore.)

if (zulipFeatureLevel >= 163) { // TODO(server-7)
// A non-null value means self-user has access to [user]'s real email,
// while a null value means it doesn't have access to the email.
// Search for "delivery_email" in https://zulip.com/api/register-queue.
return user.deliveryEmail;
} else {
if (user.deliveryEmail != null) {
// A non-null value means self-user has access to [user]'s real email,
// while a null value doesn't necessarily mean it doesn't have access
// to the email, ....
return user.deliveryEmail;
} else if (emailAddressVisibility == EmailAddressVisibility.everyone) {
// ... we have to also check for [PerAccountStore.emailAddressVisibility].
// See:
// * https://github.com/zulip/zulip-mobile/pull/5515#discussion_r997731727
// * https://chat.zulip.org/#narrow/stream/378-api-design/topic/email.20address.20visibility/near/1296133
return user.email;
} else {
return null;
}
}
}

////////////////////////////////
// Streams, topics, and stuff about them.

Expand Down
82 changes: 65 additions & 17 deletions lib/widgets/autocomplete.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import '../model/autocomplete.dart';
import '../model/compose.dart';
import '../model/narrow.dart';
import 'compose_box.dart';
import 'text.dart';
import 'theme.dart';

abstract class AutocompleteField<QueryT extends AutocompleteQuery, ResultT extends AutocompleteResult> extends StatefulWidget {
const AutocompleteField({
Expand Down Expand Up @@ -218,6 +220,8 @@ class ComposeAutocomplete extends AutocompleteField<ComposeAutocompleteQuery, Co

@override
Widget buildItem(BuildContext context, int index, ComposeAutocompleteResult option) {
final designVariables = DesignVariables.of(context);

final child = switch (option) {
MentionAutocompleteResult() => _MentionAutocompleteItem(
option: option, narrow: narrow),
Expand All @@ -227,6 +231,9 @@ class ComposeAutocomplete extends AutocompleteField<ComposeAutocompleteQuery, Co
onTap: () {
_onTapOption(context, option);
},
highlightColor: designVariables.editorButtonPressedBg,
splashFactory: NoSplash.splashFactory,
borderRadius: BorderRadius.circular(5),
child: child);
}
}
Expand All @@ -237,14 +244,14 @@ class _MentionAutocompleteItem extends StatelessWidget {
final MentionAutocompleteResult option;
final Narrow narrow;

Widget wildcardLabel(WildcardMentionOption wildcardOption, {
String wildcardSublabel(WildcardMentionOption wildcardOption, {
required BuildContext context,
required PerAccountStore store,
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't necessarily add this from scratch, but no need to cut it — it's an optimization, saving a duplicate PerAccountStoreWidget.of, and it's very cheap in terms of the code it requires.

}) {
final isDmNarrow = narrow is DmNarrow;
final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9)
final localizations = ZulipLocalizations.of(context);
final description = switch (wildcardOption) {
return switch (wildcardOption) {
WildcardMentionOption.all || WildcardMentionOption.everyone => isDmNarrow
? localizations.wildcardMentionAllDmDescription
: isChannelWildcardAvailable
Expand All @@ -256,32 +263,61 @@ class _MentionAutocompleteItem extends StatelessWidget {
: localizations.wildcardMentionStreamDescription,
WildcardMentionOption.topic => localizations.wildcardMentionTopicDescription,
};
return Text.rich(TextSpan(text: '${wildcardOption.canonicalString} ', children: [
TextSpan(text: description, style: TextStyle(fontSize: 12,
color: DefaultTextStyle.of(context).style.color?.withValues(alpha: 0.8)))]));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: don't add blank line

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

Widget avatar;
Widget label;
String label;
String? sublabel;
switch (option) {
case UserMentionAutocompleteResult(:var userId):
final user = store.getUser(userId)!; // must exist because UserMentionAutocompleteResult
avatar = Avatar(userId: userId, size: 32, borderRadius: 3); // web uses 21px
label = Text(user.fullName);
avatar = Avatar(userId: userId, size: 36, borderRadius: 4);
label = user.fullName;
sublabel = store.userDisplayEmail(user);
case WildcardMentionAutocompleteResult(:var wildcardOption):
avatar = const Icon(ZulipIcons.three_person, size: 29); // web uses 19px
label = wildcardLabel(wildcardOption, context: context, store: store);
avatar = SizedBox.square(dimension: 36,
child: const Icon(ZulipIcons.three_person, size: 24));
label = wildcardOption.canonicalString;
sublabel = wildcardSublabel(wildcardOption, context: context, store: store);
}

final labelWidget = Text(
label,
style: TextStyle(
fontSize: 18,
height: 20 / 18,
color: designVariables.contextMenuItemLabel,
).merge(weightVariableTextStyle(context,
wght: sublabel == null ? 500 : 600)),
Comment on lines +295 to +296
Copy link
Member

Choose a reason for hiding this comment

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

Huh interesting, subtle.

overflow: TextOverflow.ellipsis,
maxLines: 1);

final sublabelWidget = sublabel == null ? null : Text(
sublabel,
style: TextStyle(
fontSize: 14,
height: 16 / 14,
color: designVariables.contextMenuItemMeta),
overflow: TextOverflow.ellipsis,
maxLines: 1);

return Padding(
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
padding: const EdgeInsetsDirectional.fromSTEB(4, 4, 8, 4),
child: Row(children: [
avatar,
const SizedBox(width: 8),
label,
const SizedBox(width: 6),
Expanded(child: Column(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.start,
children: [
labelWidget,
if (sublabelWidget != null) sublabelWidget,
])),
]));
}
}
Expand All @@ -291,12 +327,13 @@ class _EmojiAutocompleteItem extends StatelessWidget {

final EmojiAutocompleteResult option;

static const _size = 32.0;
static const _notoColorEmojiTextSize = 25.7;
static const _size = 24.0;
static const _notoColorEmojiTextSize = 19.3;

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

// TODO deduplicate this logic with [EmojiPickerListEntry]
Expand All @@ -315,15 +352,26 @@ class _EmojiAutocompleteItem extends StatelessWidget {
? candidate.emojiName
: [candidate.emojiName, ...candidate.aliases].join(", "); // TODO(#1080)

// TODO(design): emoji autocomplete results
// There's no design in Figma for emoji autocomplete results.
// Instead we adapt the design for the emoji picker to the
// context of autocomplete results as exemplified by _MentionAutocompleteItem.
// That means: emoji size, text size, text line-height, and font weight
// from emoji picker; text color (for contrast with background) and
// outer padding from _MentionAutocompleteItem; padding around emoji glyph
// to bring it to same size as avatar in _MentionAutocompleteItem.
return Padding(
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
child: Row(children: [
if (glyph != null) ...[
glyph,
const SizedBox(width: 8),
Padding(padding: const EdgeInsets.all(6),
child: glyph),
const SizedBox(width: 6),
],
Expanded(
child: Text(
style: TextStyle(fontSize: 17, height: 18 / 17,
color: designVariables.contextMenuItemLabel),
maxLines: 2,
overflow: TextOverflow.ellipsis,
label)),
Expand Down
30 changes: 1 addition & 29 deletions lib/widgets/profile.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ import 'dart:convert';

import 'package:flutter/material.dart';

import '../api/model/initial_snapshot.dart';
import '../api/model/model.dart';
import '../generated/l10n/zulip_localizations.dart';
import '../model/content.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import 'app_bar.dart';
import 'content.dart';
import 'message_list.dart';
Expand Down Expand Up @@ -36,32 +34,6 @@ class ProfilePage extends StatelessWidget {
page: ProfilePage(userId: userId));
}

/// The given user's real email address, if known, for displaying in the UI.
///
/// Returns null if self-user isn't able to see [user]'s real email address.
String? _getDisplayEmailFor(User user, {required PerAccountStore store}) {
if (store.zulipFeatureLevel >= 163) { // TODO(server-7)
// A non-null value means self-user has access to [user]'s real email,
// while a null value means it doesn't have access to the email.
// Search for "delivery_email" in https://zulip.com/api/register-queue.
return user.deliveryEmail;
} else {
if (user.deliveryEmail != null) {
// A non-null value means self-user has access to [user]'s real email,
// while a null value doesn't necessarily mean it doesn't have access
// to the email, ....
return user.deliveryEmail;
} else if (store.emailAddressVisibility == EmailAddressVisibility.everyone) {
// ... we have to also check for [PerAccountStore.emailAddressVisibility].
// See:
// * https://github.com/zulip/zulip-mobile/pull/5515#discussion_r997731727
// * https://chat.zulip.org/#narrow/stream/378-api-design/topic/email.20address.20visibility/near/1296133
return user.email;
} else {
return null;
}
}
}

@override
Widget build(BuildContext context) {
Expand All @@ -72,7 +44,7 @@ class ProfilePage extends StatelessWidget {
return const _ProfileErrorPage();
}

final displayEmail = _getDisplayEmailFor(user, store: store);
final displayEmail = store.userDisplayEmail(user);
final items = [
Center(
child: Avatar(userId: userId, size: 200, borderRadius: 200 / 8)),
Expand Down
18 changes: 16 additions & 2 deletions lib/widgets/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
composeBoxBg: const Color(0xffffffff),
contextMenuCancelText: const Color(0xff222222),
contextMenuItemBg: const Color(0xff6159e1),
contextMenuItemLabel: const Color(0xff242631),
contextMenuItemMeta: const Color(0xff626573),
contextMenuItemText: const Color(0xff381da7),
editorButtonPressedBg: Colors.black.withValues(alpha: 0.06),
foreground: const Color(0xff000000),
Expand Down Expand Up @@ -184,6 +186,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
composeBoxBg: const Color(0xff0f0f0f),
contextMenuCancelText: const Color(0xffffffff).withValues(alpha: 0.75),
contextMenuItemBg: const Color(0xff7977fe),
contextMenuItemLabel: const Color(0xffdfe1e8),
contextMenuItemMeta: const Color(0xff9194a3),
contextMenuItemText: const Color(0xff9398fd),
editorButtonPressedBg: Colors.white.withValues(alpha: 0.06),
foreground: const Color(0xffffffff),
Expand Down Expand Up @@ -240,6 +244,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
required this.composeBoxBg,
required this.contextMenuCancelText,
required this.contextMenuItemBg,
required this.contextMenuItemLabel,
required this.contextMenuItemMeta,
required this.contextMenuItemText,
required this.editorButtonPressedBg,
required this.foreground,
Expand Down Expand Up @@ -297,6 +303,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
final Color composeBoxBg;
final Color contextMenuCancelText;
final Color contextMenuItemBg;
final Color contextMenuItemLabel;
final Color contextMenuItemMeta;
final Color contextMenuItemText;
final Color editorButtonPressedBg;
final Color foreground;
Expand Down Expand Up @@ -349,6 +357,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
Color? composeBoxBg,
Color? contextMenuCancelText,
Color? contextMenuItemBg,
Color? contextMenuItemLabel,
Color? contextMenuItemMeta,
Color? contextMenuItemText,
Color? editorButtonPressedBg,
Color? foreground,
Expand Down Expand Up @@ -396,7 +406,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
composeBoxBg: composeBoxBg ?? this.composeBoxBg,
contextMenuCancelText: contextMenuCancelText ?? this.contextMenuCancelText,
contextMenuItemBg: contextMenuItemBg ?? this.contextMenuItemBg,
contextMenuItemText: contextMenuItemText ?? this.contextMenuItemBg,
contextMenuItemLabel: contextMenuItemLabel ?? this.contextMenuItemLabel,
contextMenuItemMeta: contextMenuItemMeta ?? this.contextMenuItemMeta,
contextMenuItemText: contextMenuItemText ?? this.contextMenuItemText,
editorButtonPressedBg: editorButtonPressedBg ?? this.editorButtonPressedBg,
foreground: foreground ?? this.foreground,
icon: icon ?? this.icon,
Expand Down Expand Up @@ -450,7 +462,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
composeBoxBg: Color.lerp(composeBoxBg, other.composeBoxBg, t)!,
contextMenuCancelText: Color.lerp(contextMenuCancelText, other.contextMenuCancelText, t)!,
contextMenuItemBg: Color.lerp(contextMenuItemBg, other.contextMenuItemBg, t)!,
contextMenuItemText: Color.lerp(contextMenuItemText, other.contextMenuItemBg, t)!,
contextMenuItemLabel: Color.lerp(contextMenuItemLabel, other.contextMenuItemLabel, t)!,
contextMenuItemMeta: Color.lerp(contextMenuItemMeta, other.contextMenuItemMeta, t)!,
contextMenuItemText: Color.lerp(contextMenuItemText, other.contextMenuItemText, t)!,
Copy link
Member

Choose a reason for hiding this comment

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

nit:

design_variables: Fix wrong variable in contextMenuItemText color field

Similarly, see git log --oneline lib/widgets/theme.dart for previous examples of commit summary lines for commits touching this file. You'll see there's no design_variables, but there are several with theme. A good prefix here would be theme:.

In general anything with underscores in it looks like an identifier, not a piece of plain English, so it should only be used if it's actually referring to an identifier with that name. There's no identifier design_variables, so that term shouldn't appear.

editorButtonPressedBg: Color.lerp(editorButtonPressedBg, other.editorButtonPressedBg, t)!,
foreground: Color.lerp(foreground, other.foreground, t)!,
icon: Color.lerp(icon, other.icon, t)!,
Expand Down
Loading