Skip to content

general chat #1: add support at most places #1297

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

Merged
merged 6 commits into from
Mar 10, 2025
Merged
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
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class InitialSnapshot {

final Uri? serverEmojiDataUrl; // TODO(server-6)

final String? realmEmptyTopicDisplayName; // TODO(server-10)

@JsonKey(readValue: _readUsersIsActiveFallbackTrue)
final List<User> realmUsers;
@JsonKey(readValue: _readUsersIsActiveFallbackFalse)
Expand Down Expand Up @@ -138,6 +140,7 @@ class InitialSnapshot {
required this.realmDefaultExternalAccounts,
required this.maxFileUploadSizeMib,
required this.serverEmojiDataUrl,
required this.realmEmptyTopicDisplayName,
required this.realmUsers,
required this.realmNonActiveUsers,
required this.crossRealmBots,
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/initial_snapshot.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions lib/model/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
}

TopicAutocompleteResult? _testTopic(TopicAutocompleteQuery query, TopicName topic) {
if (query.testTopic(topic)) {
if (query.testTopic(topic, store)) {
return TopicAutocompleteResult(topic: topic);
}
return null;
Expand All @@ -939,10 +939,17 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
class TopicAutocompleteQuery extends AutocompleteQuery {
TopicAutocompleteQuery(super.raw);

bool testTopic(TopicName topic) {
bool testTopic(TopicName topic, PerAccountStore store) {
// TODO(#881): Sort by match relevance, like web does.

// ignore: unnecessary_null_comparison // null topic names soon to be enabled
if (topic.displayName == null) {
return store.realmEmptyTopicDisplayName.toLowerCase()
.contains(raw.toLowerCase());
}
return topic.displayName != raw
&& topic.displayName.toLowerCase().contains(raw.toLowerCase());
// ignore: unnecessary_non_null_assertion // null topic names soon to be enabled
&& topic.displayName!.toLowerCase().contains(raw.toLowerCase());
}

@override
Expand Down
15 changes: 15 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
realmMandatoryTopics: initialSnapshot.realmMandatoryTopics,
realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold,
maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib,
realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName,
realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts,
customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields),
emailAddressVisibility: initialSnapshot.emailAddressVisibility,
Expand Down Expand Up @@ -344,6 +345,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
required this.realmMandatoryTopics,
required this.realmWaitingPeriodThreshold,
required this.maxFileUploadSizeMib,
required String? realmEmptyTopicDisplayName,
required this.realmDefaultExternalAccounts,
required this.customProfileFields,
required this.emailAddressVisibility,
Expand All @@ -362,6 +364,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
assert(realmUrl == connection.realmUrl),
assert(emoji.realmUrl == realmUrl),
_globalStore = globalStore,
_realmEmptyTopicDisplayName = realmEmptyTopicDisplayName,
_emoji = emoji,
_users = users,
_channels = channels,
Expand Down Expand Up @@ -414,6 +417,18 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
/// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold].
final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting
final int maxFileUploadSizeMib; // No event for this.

/// The display name to use for empty topics.
///
/// This should only be accessed when FL >= 334, since topics cannot
/// be empty otherwise.
// TODO(server-10) simplify this
String get realmEmptyTopicDisplayName {
assert(_realmEmptyTopicDisplayName != null); // TODO(log)
Copy link
Member

Choose a reason for hiding this comment

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

This calls for a comment explaining why this condition is supposed to be unreachable — it doesn't seem obvious.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe clearest: a bit of dartdoc saying not to access this unless zulipFeatureLevel is at least such-and-such. That's the idea for why this shouldn't happen, right?

return _realmEmptyTopicDisplayName ?? 'general chat';
}
final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting
Comment on lines +426 to +430
Copy link
Member

Choose a reason for hiding this comment

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

nit: set this group off with blank lines


final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;
List<CustomProfileField> customProfileFields;
/// For docs, please see [InitialSnapshot.emailAddressVisibility].
Expand Down
13 changes: 12 additions & 1 deletion lib/widgets/autocomplete.dart
Original file line number Diff line number Diff line change
Expand Up @@ -415,12 +415,23 @@ class TopicAutocomplete extends AutocompleteField<TopicAutocompleteQuery, TopicA

@override
Widget buildItem(BuildContext context, int index, TopicAutocompleteResult option) {
final Widget child;
// ignore: unnecessary_null_comparison // null topic names soon to be enabled
if (option.topic.displayName == null) {
final store = PerAccountStoreWidget.of(context);
child = Text(store.realmEmptyTopicDisplayName,
style: const TextStyle(fontStyle: FontStyle.italic));
} else {
// ignore: unnecessary_non_null_assertion // null topic names soon to be enabled
child = Text(option.topic.displayName!);
}

return InkWell(
onTap: () {
_onTapOption(context, option);
},
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
child: Text(option.topic.displayName)));
child: child));
}
}
3 changes: 2 additions & 1 deletion lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ class ComposeTopicController extends ComposeController<TopicValidationError> {
}

void setTopic(TopicName newTopic) {
value = TextEditingValue(text: newTopic.displayName);
// ignore: dead_null_aware_expression // null topic names soon to be enabled
value = TextEditingValue(text: newTopic.displayName ?? '');
}
}

Expand Down
5 changes: 4 additions & 1 deletion lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -532,12 +532,15 @@ class _TopicItem extends StatelessWidget {
style: TextStyle(
fontSize: 17,
height: (20 / 17),
// ignore: unnecessary_null_comparison // null topic names soon to be enabled
fontStyle: topic.displayName == null ? FontStyle.italic : null,
// TODO(design) check if this is the right variable
color: designVariables.labelMenuButton,
),
maxLines: 2,
overflow: TextOverflow.ellipsis,
topic.displayName))),
// ignore: dead_null_aware_expression // null topic names soon to be enabled
topic.displayName ?? store.realmEmptyTopicDisplayName))),
const SizedBox(width: 12),
if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign),
// TODO(design) copies the "@" marker color; is there a better color?
Expand Down
16 changes: 12 additions & 4 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,11 @@ class MessageListAppBarTitle extends StatelessWidget {
return Row(
mainAxisSize: MainAxisSize.min,
children: [
Flexible(child: Text(topic.displayName, style: const TextStyle(
Copy link
Collaborator

Choose a reason for hiding this comment

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

msglist: Display realmEmptyTopicDisplayName where empty topic appears

Signed-off-by: Zixuan James Li <[email protected]>

(Same comment about this being an NFC commit.)

// ignore: dead_null_aware_expression // null topic names soon to be enabled
Flexible(child: Text(topic.displayName ?? store.realmEmptyTopicDisplayName, style: TextStyle(
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 have a test for this part too. Should be quick, I think; can go in the "app bar" group.

fontSize: 13,
// ignore: unnecessary_null_comparison // null topic names soon to be enabled
fontStyle: topic.displayName == null ? FontStyle.italic : null,
).merge(weightVariableTextStyle(context)))),
if (icon != null)
Padding(
Expand Down Expand Up @@ -1092,11 +1095,15 @@ class StreamMessageRecipientHeader extends StatelessWidget {
child: Row(
children: [
Flexible(
child: Text(topic.displayName,
// ignore: dead_null_aware_expression // null topic names soon to be enabled
child: Text(topic.displayName ?? store.realmEmptyTopicDisplayName,
// TODO: Give a way to see the whole topic (maybe a
// long-press interaction?)
overflow: TextOverflow.ellipsis,
style: recipientHeaderTextStyle(context))),
style: recipientHeaderTextStyle(context,
// ignore: unnecessary_null_comparison // null topic names soon to be enabled
fontStyle: topic.displayName == null ? FontStyle.italic : null,
))),
const SizedBox(width: 4),
Icon(size: 14, color: designVariables.title.withFadedAlpha(0.5),
// A null [Icon.icon] makes a blank space.
Expand Down Expand Up @@ -1191,12 +1198,13 @@ class DmRecipientHeader extends StatelessWidget {
}
}

TextStyle recipientHeaderTextStyle(BuildContext context) {
TextStyle recipientHeaderTextStyle(BuildContext context, {FontStyle? fontStyle}) {
return TextStyle(
color: DesignVariables.of(context).title,
fontSize: 16,
letterSpacing: proportionalLetterSpacing(context, 0.02, baseFontSize: 16),
height: (18 / 16),
fontStyle: fontStyle,
).merge(weightVariableTextStyle(context, wght: 600));
}

Expand Down
4 changes: 4 additions & 0 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,8 @@ TestGlobalStore globalStore({List<Account> accounts = const []}) {
}
const _globalStore = globalStore;

const String defaultRealmEmptyTopicDisplayName = 'test general chat';

InitialSnapshot initialSnapshot({
String? queueId,
int? lastEventId,
Expand All @@ -906,6 +908,7 @@ InitialSnapshot initialSnapshot({
Map<String, RealmDefaultExternalAccount>? realmDefaultExternalAccounts,
int? maxFileUploadSizeMib,
Uri? serverEmojiDataUrl,
String? realmEmptyTopicDisplayName,
List<User>? realmUsers,
List<User>? realmNonActiveUsers,
List<User>? crossRealmBots,
Expand Down Expand Up @@ -943,6 +946,7 @@ InitialSnapshot initialSnapshot({
maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25,
serverEmojiDataUrl: serverEmojiDataUrl
?? realmUrl.replace(path: '/static/emoji.json'),
realmEmptyTopicDisplayName: realmEmptyTopicDisplayName ?? defaultRealmEmptyTopicDisplayName,
realmUsers: realmUsers ?? [],
realmNonActiveUsers: realmNonActiveUsers ?? [],
crossRealmBots: crossRealmBots ?? [],
Expand Down
3 changes: 2 additions & 1 deletion test/model/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1027,8 +1027,9 @@ void main() {
});

group('TopicAutocompleteQuery.testTopic', () {
final store = eg.store();
void doCheck(String rawQuery, String topic, bool expected) {
final result = TopicAutocompleteQuery(rawQuery).testTopic(eg.t(topic));
final result = TopicAutocompleteQuery(rawQuery).testTopic(eg.t(topic), store);
expected ? check(result).isTrue() : check(result).isFalse();
}

Expand Down
65 changes: 52 additions & 13 deletions test/widgets/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ Future<Finder> setupToComposeInput(WidgetTester tester, {
/// Returns a [Finder] for the topic input's [TextField].
Future<Finder> setupToTopicInput(WidgetTester tester, {
required List<GetStreamTopicsEntry> topics,
String? realmEmptyTopicDisplayName,
}) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
realmEmptyTopicDisplayName: realmEmptyTopicDisplayName));
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUser(eg.selfUser);
final connection = store.connection as FakeApiConnection;
Expand Down Expand Up @@ -392,16 +394,11 @@ void main() {
});

group('TopicAutocomplete', () {
void checkTopicShown(GetStreamTopicsEntry topic, PerAccountStore store, {required bool expected}) {
check(find.text(topic.name.displayName).evaluate().length).equals(expected ? 1 : 0);
}

testWidgets('options appear, disappear, and change correctly', (WidgetTester tester) async {
final topic1 = eg.getStreamTopicsEntry(maxId: 1, name: 'Topic one');
final topic2 = eg.getStreamTopicsEntry(maxId: 2, name: 'Topic two');
final topic3 = eg.getStreamTopicsEntry(maxId: 3, name: 'Topic three');
final topicInputFinder = await setupToTopicInput(tester, topics: [topic1, topic2, topic3]);
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

// Options are filtered correctly for query
// TODO(#226): Remove this extra edit when this bug is fixed.
Expand All @@ -410,24 +407,24 @@ void main() {
await tester.pumpAndSettle();

// "topic three" and "topic two" appear, but not "topic one"
checkTopicShown(topic1, store, expected: false);
checkTopicShown(topic2, store, expected: true);
checkTopicShown(topic3, store, expected: true);
check(find.text('Topic one' )).findsNothing();
check(find.text('Topic two' )).findsOne();
check(find.text('Topic three')).findsOne();

// Finishing autocomplete updates topic box; causes options to disappear
await tester.tap(find.text('Topic three'));
await tester.pumpAndSettle();
check(tester.widget<TextField>(topicInputFinder).controller!.text)
.equals(topic3.name.displayName);
checkTopicShown(topic1, store, expected: false);
checkTopicShown(topic2, store, expected: false);
checkTopicShown(topic3, store, expected: true); // shown in `_TopicInput` once
check(find.text('Topic one' )).findsNothing();
check(find.text('Topic two' )).findsNothing();
check(find.text('Topic three')).findsOne(); // shown in `_TopicInput` once

// Then a new autocomplete intent brings up options again
await tester.enterText(topicInputFinder, 'Topic');
await tester.enterText(topicInputFinder, 'Topic T');
await tester.pumpAndSettle();
checkTopicShown(topic2, store, expected: true);
check(find.text('Topic two')).findsOne();
});

testWidgets('text selection is reset on choosing an option', (tester) async {
Expand Down Expand Up @@ -464,5 +461,47 @@ void main() {

await tester.pump(Duration.zero);
});

testWidgets('display realmEmptyTopicDisplayName for empty topic', (tester) async {
final topic = eg.getStreamTopicsEntry(name: '');
final topicInputFinder = await setupToTopicInput(tester, topics: [topic],
realmEmptyTopicDisplayName: 'some display name');

// TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(topicInputFinder, ' ');
await tester.enterText(topicInputFinder, '');
await tester.pumpAndSettle();

check(find.text('some display name')).findsOne();
}, skip: true); // null topic names soon to be enabled

testWidgets('match realmEmptyTopicDisplayName in autocomplete', (tester) async {
final topic = eg.getStreamTopicsEntry(name: '');
final topicInputFinder = await setupToTopicInput(tester, topics: [topic],
realmEmptyTopicDisplayName: 'general chat');

// TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(topicInputFinder, 'general ch');
await tester.enterText(topicInputFinder, 'general cha');
await tester.pumpAndSettle();

check(find.text('general chat')).findsOne();
}, skip: true); // null topic names soon to be enabled

testWidgets('autocomplete to realmEmptyTopicDisplayName sets topic to empty string', (tester) async {
final topic = eg.getStreamTopicsEntry(name: '');
final topicInputFinder = await setupToTopicInput(tester, topics: [topic],
realmEmptyTopicDisplayName: 'general chat');
final controller = tester.widget<TextField>(topicInputFinder).controller!;

// TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(topicInputFinder, 'general ch');
await tester.enterText(topicInputFinder, 'general cha');
await tester.pumpAndSettle();

await tester.tap(find.text('general chat'));
await tester.pump(Duration.zero);
check(controller.value).text.equals('');
}, skip: true); // null topic names soon to be enabled
});
}
9 changes: 9 additions & 0 deletions test/widgets/inbox_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,15 @@ void main() {
});
});

testWidgets('empty topic', (tester) async {
final channel = eg.stream();
await setupPage(tester,
streams: [channel],
subscriptions: [(eg.subscription(channel))],
unreadMessages: [eg.streamMessage(stream: channel, topic: '')]);
check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();
}, skip: true); // null topic names soon to be enabled

group('topic visibility', () {
final channel = eg.stream();
const topic = 'topic';
Expand Down
Loading