Skip to content

msglist: Display typing indicators on typing. #790

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 5 commits into from
Jul 30, 2024
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
19 changes: 19 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -483,5 +483,24 @@
"notifSelfUser": "You",
"@notifSelfUser": {
"description": "Display name for the user themself, to show after replying in an Android notification"
},
"onePersonTyping": "{typist} is typing…",
"@onePersonTyping": {
"description": "Text to display when there is one user typing.",
"placeholders": {
"typist": {"type": "String", "example": "Alice"}
}
},
"twoPeopleTyping": "{typist} and {otherTypist} are typing…",
"@twoPeopleTyping": {
"description": "Text to display when there are two users typing.",
"placeholders": {
"typist": {"type": "String", "example": "Alice"},
"otherTypist": {"type": "String", "example": "Bob"}
}
},
"manyPeopleTyping": "Several people are typing…",
"@manyPeopleTyping": {
"description": "Text to display when there are multiple users typing."
}
}
5 changes: 3 additions & 2 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'package:json_annotation/json_annotation.dart';

import '../../model/algorithms.dart';
import 'json.dart';
import 'model.dart';

Expand Down Expand Up @@ -900,7 +901,7 @@ class TypingEvent extends Event {
required this.recipientIds,
required this.streamId,
required this.topic,
});
}) : assert(recipientIds == null || isSortedWithoutDuplicates(recipientIds));

static Object? _readSenderId(Map<Object?, Object?> json, String key) {
return (json['sender'] as Map<String, dynamic>)['user_id'];
Expand All @@ -909,7 +910,7 @@ class TypingEvent extends Event {
static List<int>? _recipientIdsFromJson(Object? json) {
if (json == null) return null;
return (json as List<Object?>).map(
(item) => (item as Map<String, Object?>)['user_id'] as int).toList();
(item) => (item as Map<String, Object?>)['user_id'] as int).toList()..sort();
Copy link
Member

Choose a reason for hiding this comment

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

commit-message nit:

events: Sort recipientIds for TypingEvent.

The prefix we've used for changes here is api:, so let's stay consistent with that.

A command like git log --oneline lib/api/model/events.dart is very handy for looking to see what precedents are for this kind of question.

}

factory TypingEvent.fromJson(Map<String, dynamic> json) {
Expand Down
2 changes: 1 addition & 1 deletion lib/api/route/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Future<InitialSnapshot> registerQueue(ApiConnection connection) {
'notification_settings_null': true,
'bulk_message_deletion': true,
'user_avatar_url_field_optional': false, // TODO(#254): turn on
'stream_typing_notifications': false, // TODO implement
'stream_typing_notifications': true,
Copy link
Member

Choose a reason for hiding this comment

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

This should get its own commit and own commit message, since it's changing what we get from the server (whereas the rest of the commit has a very different character, adding UI).

I think the effect is that it's just causing us to get more TypingEvents, and our code already handles the new kind?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link
Member

Choose a reason for hiding this comment

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

commit-structure nit: put this commit before the main commit, so that the main commit can be accurate in claiming to fix the issue #665

(otherwise, #665 isn't actually fully working as of that commit)

'user_settings_object': true,
},
});
Expand Down
7 changes: 6 additions & 1 deletion lib/model/typing_status.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'dart:async';
import 'package:flutter/foundation.dart';

import '../api/model/events.dart';
import '../log.dart';
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 happens a commit sooner than it should

(so the linter would warn at that commit)

import 'narrow.dart';

/// The model for tracking the typing status organized by narrows.
Expand Down Expand Up @@ -38,6 +39,10 @@ class TypingStatus extends ChangeNotifier {
}

bool _addTypist(SendableNarrow narrow, int typistUserId) {
if (typistUserId == selfUserId) {
Copy link
Member

Choose a reason for hiding this comment

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

commit-message nit:

model: Do not allow adding self as typist.

For files in lib/model/, we normally make the prefix more specific than this — here, typing_status: would be good.

(I'd be tempted to shorten it to typing:, but that's too confusing with the concept of the type system and type checker.)

assert(debugLog('typing status: adding self as typist'));
return false;
}
final narrowTimerMap = _timerMapsByNarrow[narrow] ??= {};
final typistTimer = narrowTimerMap[typistUserId];
final isNewTypist = typistTimer == null;
Expand All @@ -64,7 +69,7 @@ class TypingStatus extends ChangeNotifier {
void handleTypingEvent(TypingEvent event) {
SendableNarrow narrow = switch (event.messageType) {
MessageType.direct => DmNarrow(
allRecipientIds: event.recipientIds!..sort(), selfUserId: selfUserId),
allRecipientIds: event.recipientIds!, selfUserId: selfUserId),
MessageType.stream => TopicNarrow(event.streamId!, event.topic!),
};

Expand Down
68 changes: 65 additions & 3 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import 'dart:math';

import 'package:collection/collection.dart';
import 'package:flutter/material.dart';
import 'package:flutter_color_models/flutter_color_models.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import 'package:intl/intl.dart';

import '../api/model/model.dart';
import '../model/message_list.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../model/typing_status.dart';
import 'action_sheet.dart';
import 'actions.dart';
import 'compose_box.dart';
Expand Down Expand Up @@ -496,17 +498,19 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 2);
return length - 1 - (index - 3);
},
childCount: length + 2,
childCount: length + 3,
(context, i) {
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 2)];
if (i == 2) return TypingStatusWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 3)];
return _buildItem(data, i);
}));

Expand Down Expand Up @@ -609,6 +613,64 @@ class ScrollToBottomButton extends StatelessWidget {
}
}

class TypingStatusWidget extends StatefulWidget {
const TypingStatusWidget({super.key, required this.narrow});

final Narrow narrow;

@override
State<StatefulWidget> createState() => _TypingStatusWidgetState();
}

class _TypingStatusWidgetState extends State<TypingStatusWidget> with PerAccountStoreAwareStateMixin<TypingStatusWidget> {
TypingStatus? model;

@override
void onNewStore() {
model?.removeListener(_modelChanged);
model = PerAccountStoreWidget.of(context).typingStatus
..addListener(_modelChanged);
}

@override
void dispose() {
model?.removeListener(_modelChanged);
super.dispose();
}

void _modelChanged() {
setState(() {
// The actual state lives in [model].
// This method was called because that just changed.
});
}

@override
Widget build(BuildContext context) {
final narrow = widget.narrow;
if (narrow is! SendableNarrow) return const SizedBox();

final store = PerAccountStoreWidget.of(context);
final localizations = ZulipLocalizations.of(context);
final typistIds = model!.typistIdsInNarrow(narrow);
if (typistIds.isEmpty) return const SizedBox();
final text = switch (typistIds.length) {
1 => localizations.onePersonTyping(
store.users[typistIds.first]?.fullName ?? localizations.unknownUserName),
2 => localizations.twoPeopleTyping(
store.users[typistIds.first]?.fullName ?? localizations.unknownUserName,
store.users[typistIds.last]?.fullName ?? localizations.unknownUserName),
_ => localizations.manyPeopleTyping,
};

return Padding(
padding: const EdgeInsetsDirectional.only(start: 16, top: 2),
child: Text(text,
style: const TextStyle(
color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)));
}
}

class MarkAsReadWidget extends StatefulWidget {
const MarkAsReadWidget({super.key, required this.narrow});

Expand Down
7 changes: 7 additions & 0 deletions test/api/model/events_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -241,5 +241,12 @@ void main() {
check(() => TypingEvent.fromJson({
...baseJson, 'message_type': 'stream', 'stream_id': 123})).throws<void>();
});

test('direct type sort recipient ids', () {
check(TypingEvent.fromJson({
...directMessageJson,
'recipients': [4, 10, 8, 2, 1].map((e) => {'user_id': e, 'email': '[email protected]'}).toList(),
})).recipientIds.isNotNull().deepEquals([1, 2, 4, 8, 10]);
});
});
}
22 changes: 7 additions & 15 deletions test/model/typing_status_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -98,21 +98,13 @@ void main() {
checkNotifiedOnce();
});

test('sort dm recipients', () {
prepareModel(selfUserId: 5);
final recipientIds = [5, 4, 10, 8, 2, 1];

final eventUnsorted = TypingEvent(id: 1, op: TypingOp.start,
senderId: 1,
messageType: MessageType.direct,
recipientIds: recipientIds,
streamId: null, topic: null);
// DmNarrow's constructor expects the recipient IDs to be sorted,
// and [model.handleTypingEvent] should handle that.
model.handleTypingEvent(eventUnsorted);
check(model.typistIdsInNarrow(
DmNarrow(allRecipientIds: recipientIds..sort(), selfUserId: 5),
)).single.equals(1);
test('ignore adding self as typist', () {
prepareModel();

model.handleTypingEvent(
eg.typingEvent(groupNarrow, TypingOp.start, eg.selfUser.userId));
checkTypists({});
checkNotNotified();
});
});

Expand Down
Loading