Skip to content

Show user status in UI #1702

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.

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 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion lib/widgets/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,11 @@ class _MentionAutocompleteItem extends StatelessWidget {
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.start,
children: [
labelWidget,
Row(children: [
Flexible(child: labelWidget),
if (option case UserMentionAutocompleteResult(:var userId))
UserStatusEmoji(userId: userId, size: 18,
padding: const EdgeInsetsDirectional.only(start: 5.0))]),
Copy link
Member

Choose a reason for hiding this comment

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

nit: similarly:

Suggested change
padding: const EdgeInsetsDirectional.only(start: 5.0))]),
padding: const EdgeInsetsDirectional.only(start: 5.0)),
]),

if (sublabelWidget != null) sublabelWidget,
])),
Comment on lines 316 to 323
Copy link
Member

Choose a reason for hiding this comment

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

I think this new logic is best included as part of labelWidget. Conceptually it's closely tied to the user's name, which is part of the label.

Probably in fact the cleanest home for this is in the switch (option) above, which is already the place where we inspect the details of option.

]));
Expand Down
2 changes: 2 additions & 0 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1943,6 +1943,8 @@ class SenderRow extends StatelessWidget {
: designVariables.title,
).merge(weightVariableTextStyle(context, wght: 600)),
overflow: TextOverflow.ellipsis)),
UserStatusEmoji(userId: message.senderId, size: 18,
padding: const EdgeInsetsDirectional.only(start: 5.0)),
if (sender?.isBot ?? false) ...[
const SizedBox(width: 5),
Icon(
Expand Down
9 changes: 8 additions & 1 deletion lib/widgets/new_dm_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ class _SelectedUserChip extends StatelessWidget {
fontSize: 16,
height: 16 / 16,
color: designVariables.labelMenuButton)))),
UserStatusEmoji(userId: userId, size: 16,
padding: EdgeInsetsDirectional.only(end: 4)),
])));
}
}
Expand Down Expand Up @@ -415,7 +417,12 @@ class _NewDmUserListItem extends StatelessWidget {
Avatar(userId: userId, size: 32, borderRadius: 3),
SizedBox(width: 8),
Expanded(
child: Text(store.userDisplayName(userId),
child: Text.rich(
TextSpan(
children: [
TextSpan(text: store.userDisplayName(userId)),
UserStatusEmoji.asWidgetSpan(userId: userId, fontSize: 17,
textScaler: MediaQuery.textScalerOf(context))]),
Comment on lines +421 to +425
Copy link
Member

Choose a reason for hiding this comment

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

nit: for list elements, give the last one a trailing comma and newline like the others, because it's not a distinguished "child" or "payload" argument; OTOH can compact this by putting the "children" argument on the first line:

Suggested change
TextSpan(
children: [
TextSpan(text: store.userDisplayName(userId)),
UserStatusEmoji.asWidgetSpan(userId: userId, fontSize: 17,
textScaler: MediaQuery.textScalerOf(context))]),
TextSpan(children: [
TextSpan(text: store.userDisplayName(userId)),
UserStatusEmoji.asWidgetSpan(userId: userId, fontSize: 17,
textScaler: MediaQuery.textScalerOf(context)),
]),

style: TextStyle(
fontSize: 17,
height: 19 / 17,
Expand Down
15 changes: 14 additions & 1 deletion lib/widgets/profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import 'page.dart';
import 'remote_settings.dart';
import 'store.dart';
import 'text.dart';
import 'theme.dart';

class _TextStyles {
static const primaryFieldText = TextStyle(fontSize: 20);
Expand Down Expand Up @@ -47,6 +48,7 @@ class ProfilePage extends StatelessWidget {
if (user == null) {
return const _ProfileErrorPage();
}
final userStatus = store.getUserStatus(userId);
Copy link
Member

Choose a reason for hiding this comment

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

nit: put this next to displayEmail; conceptually they're here for the very same reason


final nameStyle = _TextStyles.primaryFieldText
.merge(weightVariableTextStyle(context, wght: 700));
Expand All @@ -73,17 +75,28 @@ class ProfilePage extends StatelessWidget {
),
// TODO write a test where the user is muted; check this and avatar
TextSpan(text: store.userDisplayName(userId, replaceIfMuted: false)),
UserStatusEmoji.asWidgetSpan(
userId: userId,
fontSize: 20,
Copy link
Member

Choose a reason for hiding this comment

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

nit: this says fontSize: 20 but the presence circle above says fontSize: nameStyle.fontSize!; should say the same thing both places, since the intent is for them to be the same

textScaler: MediaQuery.textScalerOf(context),
neverAnimate: false,
),
]),
textAlign: TextAlign.center,
style: nameStyle),
if (userStatus.text != null)
Text(userStatus.text!,
textAlign: TextAlign.center,
style: TextStyle(fontSize: 18, height: 22 / 18,
color: DesignVariables.of(context).userStatusText)),

if (displayEmail != null)
Comment on lines +91 to 93
Copy link
Member

Choose a reason for hiding this comment

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

Is this blank line intended?

The items just above here feel to me closely related to the items just below, and in particular just as closely related as other items that are grouped next to each other.

Text(displayEmail,
textAlign: TextAlign.center,
style: _TextStyles.primaryFieldText),
Text(roleToLabel(user.role, zulipLocalizations),
textAlign: TextAlign.center,
style: _TextStyles.primaryFieldText),
// TODO(#197) render user status
// TODO(#196) render active status
// TODO(#292) render user local time

Expand Down
21 changes: 18 additions & 3 deletions lib/widgets/recent_dm_conversations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,31 @@ class RecentDmConversationsItem extends StatelessWidget {
const SizedBox(width: 8),
Expanded(child: Padding(
padding: const EdgeInsets.symmetric(vertical: 4),
child: Text(
child: Text.rich(
TextSpan(
children: [
TextSpan(text: title),
...(switch (narrow.otherRecipientIds) {
// self-DM
[] => [UserStatusEmoji.asWidgetSpan(userId: store.selfUserId,
fontSize: 17, textScaler: MediaQuery.textScalerOf(context))],
// 1:1-DM
[final otherUserId] =>
Comment on lines +152 to +160
Copy link
Member

Choose a reason for hiding this comment

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

Let's pull all this out of the return-expression and into a local variable, to reduce the amount of logic that's in the one expression.

In fact, looking at where title comes from: it's a switch on exactly the same three cases as this. So how about making title an InlineSpan that includes this?

[UserStatusEmoji.asWidgetSpan(userId: otherUserId,
fontSize: 17, textScaler: MediaQuery.textScalerOf(context))],
// group-DM - show nothing
[...] => [],
}),
]
),
style: TextStyle(
fontSize: 17,
height: (20 / 17),
// TODO(design) check if this is the right variable
color: designVariables.labelMenuButton,
),
maxLines: 2,
overflow: TextOverflow.ellipsis,
title))),
overflow: TextOverflow.ellipsis))),
const SizedBox(width: 12),
unreadCount > 0
? Padding(padding: const EdgeInsetsDirectional.only(end: 16),
Expand Down
8 changes: 8 additions & 0 deletions lib/widgets/theme.dart
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
subscriptionListHeaderLine: const HSLColor.fromAHSL(0.2, 240, 0.1, 0.5).toColor(),
subscriptionListHeaderText: const HSLColor.fromAHSL(1.0, 240, 0.1, 0.5).toColor(),
unreadCountBadgeTextForChannel: Colors.black.withValues(alpha: 0.9),
userStatusText: const Color(0xff808080),
);

static final dark = DesignVariables._(
Expand Down Expand Up @@ -309,6 +310,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
// TODO(design-dark) need proper dark-theme color (this is ad hoc)
subscriptionListHeaderText: const HSLColor.fromAHSL(1.0, 240, 0.1, 0.75).toColor(),
unreadCountBadgeTextForChannel: Colors.white.withValues(alpha: 0.9),
// TODO(design-dark) unchanged in dark theme?
userStatusText: const Color(0xff808080),
);

DesignVariables._({
Expand Down Expand Up @@ -388,6 +391,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
required this.subscriptionListHeaderLine,
required this.subscriptionListHeaderText,
required this.unreadCountBadgeTextForChannel,
required this.userStatusText,
});

/// The [DesignVariables] from the context's active theme.
Expand Down Expand Up @@ -480,6 +484,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
final Color subscriptionListHeaderLine;
final Color subscriptionListHeaderText;
final Color unreadCountBadgeTextForChannel;
final Color userStatusText; // In Figma, but unnamed.

@override
DesignVariables copyWith({
Expand Down Expand Up @@ -559,6 +564,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
Color? subscriptionListHeaderLine,
Color? subscriptionListHeaderText,
Color? unreadCountBadgeTextForChannel,
Color? userStatusText,
}) {
return DesignVariables._(
background: background ?? this.background,
Expand Down Expand Up @@ -637,6 +643,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
subscriptionListHeaderLine: subscriptionListHeaderLine ?? this.subscriptionListHeaderLine,
subscriptionListHeaderText: subscriptionListHeaderText ?? this.subscriptionListHeaderText,
unreadCountBadgeTextForChannel: unreadCountBadgeTextForChannel ?? this.unreadCountBadgeTextForChannel,
userStatusText: userStatusText ?? this.userStatusText,
);
}

Expand Down Expand Up @@ -722,6 +729,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
subscriptionListHeaderLine: Color.lerp(subscriptionListHeaderLine, other.subscriptionListHeaderLine, t)!,
subscriptionListHeaderText: Color.lerp(subscriptionListHeaderText, other.subscriptionListHeaderText, t)!,
unreadCountBadgeTextForChannel: Color.lerp(unreadCountBadgeTextForChannel, other.unreadCountBadgeTextForChannel, t)!,
userStatusText: Color.lerp(userStatusText, other.userStatusText, t)!,
);
}
}
Expand Down
51 changes: 48 additions & 3 deletions test/widgets/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:zulip/api/model/model.dart';
import 'package:zulip/api/route/messages.dart';
import 'package:zulip/api/route/channels.dart';
import 'package:zulip/api/route/realm.dart';
import 'package:zulip/basic.dart';
import 'package:zulip/model/compose.dart';
import 'package:zulip/model/emoji.dart';
import 'package:zulip/model/localizations.dart';
Expand All @@ -15,6 +16,7 @@ import 'package:zulip/model/store.dart';
import 'package:zulip/model/typing_status.dart';
import 'package:zulip/widgets/compose_box.dart';
import 'package:zulip/widgets/content.dart';
import 'package:zulip/widgets/emoji.dart';
import 'package:zulip/widgets/message_list.dart';

import '../api/fake_api.dart';
Expand All @@ -23,6 +25,7 @@ import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/test_store.dart';
import '../test_images.dart';
import 'message_list_test.dart';
import 'test_app.dart';

/// Simulates loading a [MessageListPage] and tapping to focus the compose input.
Expand All @@ -36,6 +39,7 @@ import 'test_app.dart';
/// before the end of the test.
Future<Finder> setupToComposeInput(WidgetTester tester, {
List<User> users = const [],
List<(int userId, UserStatusChange change)>? userStatuses,
Narrow? narrow,
}) async {
assert(narrow is ChannelNarrow? || narrow is SendableNarrow?);
Expand All @@ -47,6 +51,7 @@ Future<Finder> setupToComposeInput(WidgetTester tester, {
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUsers([eg.selfUser, eg.otherUser]);
await store.addUsers(users);
await store.changeUserStatuses(userStatuses ?? []);
final connection = store.connection as FakeApiConnection;

narrow ??= DmNarrow(
Expand Down Expand Up @@ -152,9 +157,24 @@ void main() {
Finder findAvatarImage(int userId) =>
find.byWidgetPredicate((widget) => widget is AvatarImage && widget.userId == userId);

void checkUserShown(User user, {required bool expected}) {
check(find.text(user.fullName)).findsExactly(expected ? 1 : 0);
check(findAvatarImage(user.userId)).findsExactly(expected ? 1 : 0);
void checkUserShown(User user, {required bool expected, bool withStatusEmoji = false}) {
assert(expected || !withStatusEmoji);

final nameFinder = find.text(user.fullName);
check(nameFinder).findsExactly(expected ? 1 : 0);

final avatarFinder = findAvatarImage(user.userId);
check(avatarFinder).findsExactly(expected ? 1 : 0);

final statusEmojiFinder = findStatusEmoji(UnicodeEmojiWidget);
if (withStatusEmoji) {
checkUserStatusEmoji(statusEmojiFinder, isAnimated: false);
}
final rowFinder = find.ancestor(of: nameFinder,
matching: find.ancestor(of: avatarFinder,
matching: find.ancestor(of: statusEmojiFinder,
matching: find.byType(Row))));
check(rowFinder).findsExactly(expected && withStatusEmoji ? 1 : 0);
}

testWidgets('user options appear, disappear, and change correctly', (tester) async {
Expand Down Expand Up @@ -202,6 +222,31 @@ void main() {
debugNetworkImageHttpClientProvider = null;
});

testWidgets('status emoji is set -> emoji is displayed', (tester) async {
final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png');
final user2 = eg.user(userId: 2, fullName: 'User Two', avatarUrl: 'user2.png');
final composeInputFinder = await setupToComposeInput(tester,
users: [user1, user2], userStatuses: [
(
user1.userId,
UserStatusChange(
text: OptionSome('Busy'),
emoji: OptionSome(StatusEmoji(emojiName: 'working_on_it',
emojiCode: '1f6e0', reactionType: ReactionType.unicodeEmoji)))
),
]);

// // TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(composeInputFinder, 'hello @u');
await tester.enterText(composeInputFinder, 'hello @');
await tester.pumpAndSettle(); // async computation; options appear

checkUserShown(user1, expected: true, withStatusEmoji: true);
checkUserShown(user2, expected: true, withStatusEmoji: false);

debugNetworkImageHttpClientProvider = null;
});

void checkWildcardShown(WildcardMentionOption wildcard, {required bool expected}) {
check(find.text(wildcard.canonicalString)).findsExactly(expected ? 1 : 0);
}
Expand Down
Loading