-
Notifications
You must be signed in to change notification settings - Fork 307
Support setting typing status #897
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
Conversation
8cf2d81
to
1927d23
Compare
27c6bd6
to
ecfd833
Compare
Switched to a different implementation for the compose box updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
lib/model/binding.dart
Outdated
/// Certain environments use some variants of the core library [Stopwatch], | ||
/// such as [ClockStopwatch]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be OK and helpful to say what environments those are? I think it's the test environment, right?
@@ -106,6 +106,12 @@ abstract class ZulipBinding { | |||
/// This wraps [url_launcher.closeInAppWebView]. | |||
Future<void> closeInAppWebView(); | |||
|
|||
/// Provides access to a new stopwatch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix link; binding: Add stopwatch.
Commit-message nit: I think there's a missing word or two in here:
[…] We need to for testability of stopwatches with fake async. […]
lib/model/typing_status.dart
Outdated
import 'package:flutter/foundation.dart'; | ||
import 'package:flutter/widgets.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this file doesn't involve widgets, best to avoid importing widgets.dart.
In a quick test, foundation.dart worked for me.
lib/model/typing_status.dart
Outdated
@@ -84,3 +87,138 @@ class TypingStatus extends ChangeNotifier { | |||
} | |||
} | |||
} | |||
|
|||
/// Manages updates to the user's typing status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
/// Sends the self-user's typing-status updates.
or similar, to be clearer that this doesn't involve reading updates from the server? (There's a different subsystem for that, TypingStatus
.)
lib/model/typing_status.dart
Outdated
/// See the web implementation: | ||
/// https://github.com/zulip/zulip/blob/52a9846cdf4abfbe937a94559690d508e95f4065/web/shared/src/typing_status.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also link to https://zulip.readthedocs.io/en/latest/subsystems/typing-indicators.html
test/model/typing_status_test.dart
Outdated
assert(timeElapsed <= model.typingStartedWaitPeriod + waitInterval); | ||
// typingStartedWaitPeriod < t <= typingStartedWaitPeriod + waitInterval * 1: | ||
// The "still typing" requests are still throttled, because it hasn't | ||
// been a full typingStartedWaitedPeriod since the last time we sent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spelling of "typingStartedWaitedPeriod"
test/model/typing_status_test.dart
Outdated
assert(timeElapsed <= model.typingStartedWaitPeriod + waitInterval); | ||
// typingStartedWaitPeriod < t <= typingStartedWaitPeriod + waitInterval * 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment doesn't correspond to the assert
exactly; do we want another assert
too?
assert(timeElapsed > model.typingStartedWaitPeriod);
test/widgets/message_list_test.dart
Outdated
addTearDown(testBinding.reset); | ||
streams ??= subscriptions ??= [eg.subscription(eg.stream(streamId: eg.defaultStreamMessageStreamId))]; | ||
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( | ||
streams: streams, subscriptions: subscriptions, unreadMsgs: unreadMsgs)); | ||
store = await testBinding.globalStore.perAccount(eg.selfAccount.id); | ||
connection = store.connection as FakeApiConnection; | ||
TypingNotifier.debugEnable = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this go right next to its teardown, like the others?
TypingNotifier.debugEnable = false;
addTearDown(TypingNotifier.debugReset);
lib/widgets/compose_box.dart
Outdated
required this.controller, | ||
required this.focusNode, | ||
required this.hintText, | ||
}); | ||
|
||
final Narrow narrow; | ||
final SendableNarrow Function() getTypingNotificationNarrow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be simplified to final SendableNarrow destination;
?
Then the _FixedDestinationContentInput
caller could just do
@@ -510,7 +510,7 @@ class _FixedDestinationContentInput extends StatelessWidget {
Widget build(BuildContext context) {
return _ContentInput(
narrow: narrow,
- getTypingNotificationNarrow: () => narrow,
+ destination: narrow,
controller: controller,
focusNode: focusNode,
hintText: _hintText(context));
. The _StreamContentInputState
caller could do
@@ -430,10 +430,11 @@ class _StreamContentInputState extends State<_StreamContentInput> {
final zulipLocalizations = ZulipLocalizations.of(context);
final streamName = store.streams[widget.narrow.streamId]?.name
?? zulipLocalizations.composeBoxUnknownChannelName;
+ final destination = TopicNarrow(
+ widget.narrow.streamId, widget.topicController.textNormalized);
return _ContentInput(
narrow: widget.narrow,
- getTypingNotificationNarrow: () => TopicNarrow(
- widget.narrow.streamId, widget.topicController.textNormalized),
+ destination: destination,
controller: widget.controller,
focusNode: widget.focusNode,
hintText: zulipLocalizations.composeBoxChannelContentHint(streamName, _topicTextNormalized));
—or maybe put the destination as a private field that gets mutated along with _topicTextNormalized
, which it's computed from. I see that _topicTextNormalized
is already being passed to _ContentInput
(for its hintText
), so _ContentInput
already has to update on keystrokes in the topic input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when the user changes the topic input, it's a cue that they've stopped composing for the old topic. How do we make sure "typing stopped" notices get sent for the old topic in that case? Is it enough that we listen for the content input being blurred, because you always blur that when you go to change the topic input? Or do we need to listen to the topic input too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, editing topic input always blurs content input, sending a "typing stopped" notice.
lib/widgets/compose_box.dart
Outdated
} | ||
|
||
void _contentChanged() { | ||
if (_prevText == widget.controller.text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about performance with this comparison, especially when the text is really long, but I don't have strong intuition on it. We expect this to be called on every keystroke and every movement of the selection handles, right? What if we just checked the length of the text? I'd assume .length
is efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find a version(*) of String.==
if you have your IDE navigate to _StringBase
(it's in bin/cache/pkg/sky_engine/lib/_internal/vm/lib/string_patch.dart
in your Flutter tree).
@pragma("vm:exact-result-type", bool)
bool operator ==(Object other) {
if (identical(this, other)) {
return true;
}
if (other is String && this.length == other.length) {
final len = this.length;
for (int i = 0; i < len; i++) {
if (this.codeUnitAt(i) != other.codeUnitAt(i)) {
return false;
}
}
return true;
}
return false;
}
As you can see there, it has a fast path if the strings are identical, and another if they're different lengths, returning true and false respectively. So the linear-time comparison happens only if the new string is a different instance yet has the same length.
I think this is probably fine. If we discover it's a performance issue, the fix might be to only check identical
, and if not identical then assume the content was actually edited.
(*) I'm not sure that's the actual version that gets run, because the two concrete implementations _OneByteString
and _TwoByteString
override it like so:
@pragma("vm:recognized", "asm-intrinsic")
@pragma("vm:exact-result-type", bool)
// Intrinsic is more efficient than an inlined body even for the small
// strings.
@pragma('vm:never-inline')
bool operator ==(Object other) {
return super == other;
}
The asm-intrinsic
thing sounds like the Dart VM may replace calls to that method with calls to an implementation written in assembly. I'd expect the assembly implementation to have all the same algorithmic optimizations as the pure-Dart version, though — it'll just have better constant factors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK, interesting!
So the linear-time comparison happens only if the new string is a different instance yet has the same length.
Yeah, this does seem rare here.
8eb0d4d
to
f01ad9b
Compare
LGTM, but how about also calling widget.contentController.clear(); —and our content-input listener responds to that by calling widget.contentController.clear();
try {
// TODO(#720) clear content input only on success response;
// while waiting, put input(s) and send button into a disabled
// "working on it" state (letting input text be selected for copying). and we want to send the notice as soon as it applies, i.e., as soon as the send button is tapped. (As a consequence, I guess we'll end up making a redundant |
Marking for Greg's review. Thanks for the revision! edit: ah, one nit I almost forgot about: I see that edit edit: And the PR description and one of the commits should get a |
0ae829f
to
31d8093
Compare
Created #1033 as a follow-up, and left the app lifecycle change here with the main implementation. Thanks for the review! This is ready now. |
0a9c6aa
to
744e468
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! Comments below.
lib/model/typing_status.dart
Outdated
// We just started typing to this destination, so notify the server. | ||
_currentDestination = destination; | ||
_startOrExtendIdleTimer(); | ||
return _actuallyPingServer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump 🙂
lib/widgets/compose_box.dart
Outdated
@@ -370,6 +452,8 @@ class _StreamContentInputState extends State<_StreamContentInput> { | |||
?? zulipLocalizations.composeBoxUnknownChannelName; | |||
return _ContentInput( | |||
narrow: widget.narrow, | |||
destination: TopicNarrow( | |||
widget.narrow.streamId, widget.topicController.textNormalized), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for this to not use _topicTextNormalized
?
Compare the hintText
value a few lines below, and also my previous comment #897 (comment) .
In general if you're doing something that's different from what a review comment asked for, please be sure to flag it, and say why — that helps simplify things for the reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. It was unintentional that I used widget.topicController.textNormalized
here.
lib/widgets/compose_box.dart
Outdated
case AppLifecycleState.detached: | ||
// > The application defaults to this state before it initializes, and | ||
// > can be in this state (applicable on Android, iOS, and web) after | ||
// > all views have been detached. | ||
// At the point, the compose box might not exist, so it is irrelevant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, "might not" isn't a satisfying basis for deciding what to do here. What if it did exist?
I think in fact the compose box can't exist here, because none of the UI can.
But then this all sounds like a reason why we should conclude the user isn't composing a message. How could they be, with no compose box? So this should go in the first group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I believe that's also what I concluded when we talked about this briefly in our call on Thursday.)
test/widgets/compose_box_test.dart
Outdated
if (topic != null) { | ||
// The topic input is currently only available to ChannelNarrow. | ||
narrow as ChannelNarrow; | ||
connection.prepare(body: | ||
jsonEncode(GetStreamTopicsResult(topics: [eg.getStreamTopicsEntry()]).toJson())); | ||
await tester.enterText(topicInputFinder, topic); | ||
check(connection.takeRequests()).single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. So the test cases that cause this request to happen are only the handful that do something to the topic input? I guess you were pointing at that in the previous comment #897 (comment) , but I didn't see the implication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that it's only two existing test cases (plus a couple of new ones) that turn out to actually need this functionality, and that it can just as well be done after the rest of the setup is complete instead of mixed into the middle of the other setup, it'd be best to do it separately. That way this prep helper doesn't grow into an omnibus do-everything, and it stays more straightforward to see what each of the tests is doing.
For the topic input, because there are these three steps involved — prepare request, enter text, consume request — it'd make sense to have a helper for those. Then the test cases that need it can call that helper, after calling prepareComposeBox
.
For the content input, the step is trivial, so test cases should just do that step directly to keep things straightforward.
test/widgets/compose_box_test.dart
Outdated
final controllerKey = await prepareComposeBox(tester, | ||
narrow: ChannelNarrow(eg.stream().streamId), | ||
topic: 'some topic', content: 'see image: '); | ||
final composeBoxController = controllerKey.currentState!; | ||
// (When we check that the send button looks disabled, it should be because | ||
// the file is uploading, not a pre-existing reason.) | ||
composeBoxController.topicController!.value = const TextEditingValue(text: 'some topic'); | ||
composeBoxController.contentController.value = const TextEditingValue(text: 'see image: '); | ||
await tester.pump(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment no longer makes sense — it was here to explain why we're setting the topic and content.
test/widgets/compose_box_test.dart
Outdated
testWidgets('for content input, unfocusing sends a "typing stopped" notice ' | ||
'and refocusing sends a "typing started" notice', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refocusing sends a "typing started" notice
Is this desirable? It seems undesirable to me.
I think I see why it happens in the current implementation. It's not a big deal and we don't have to fix it before merge. But as I said in the previous thread:
#897 (comment)
the test should focus on testing behaviors we actually want. Moreover if we do have a test that checks behavior we don't want, it should be very clearly labeled as such — otherwise, the test becomes an extra barrier in the way of us fixing the behavior.
lib/widgets/compose_box.dart
Outdated
// Losing focus usually indicates that the user has navigated away or | ||
// clicked on other UI elements. On Android, this does not get triggered | ||
// unless the user navigates away from the page or focuses on another input. | ||
// See: https://github.com/zulip/zulip-flutter/pull/897#discussion_r1818188643 | ||
final store = PerAccountStoreWidget.of(context); | ||
store.typingNotifier.stoppedComposing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Losing focus usually indicates that the user has navigated away or | |
// clicked on other UI elements. On Android, this does not get triggered | |
// unless the user navigates away from the page or focuses on another input. | |
// See: https://github.com/zulip/zulip-flutter/pull/897#discussion_r1818188643 | |
final store = PerAccountStoreWidget.of(context); | |
store.typingNotifier.stoppedComposing(); | |
final store = PerAccountStoreWidget.of(context); | |
store.typingNotifier.stoppedComposing(); |
The "On Android" part doesn't seem relevant to understanding this code; it's not part of why we wrote this code the way we did. (If hiding the keyboard did unfocus the input, we'd be glad and would still want to say the user has stopped typing.) So it can be left out.
I'm also not sure that comment is accurate — it tries to give a complete list of the things that will cause unfocus, and I'm not sure there aren't other things we haven't thought of that also do so. (What about opening the bottom sheet; does that unfocus the input? I suspect it does.)
Then the "Losing focus" sentence seems like it's just explaining the meaning of focus.
Thanks for the review! I have updated the PR addressing the comments. Note that I mentioned a minor issue in this discussion on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! This all looks good modulo one nit below, and one commit message that looks like it's meant to be updated before merge:
b0f8c35 fix link; binding: Add stopwatch.
So please go ahead and make that update along with fixing the nit, and then I think it'll be time to merge this.
test/widgets/compose_box_test.dart
Outdated
@@ -54,6 +57,21 @@ void main() { | |||
return controllerKey; | |||
} | |||
|
|||
Future<void> checkEnterTopic(WidgetTester tester, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Future<void> checkEnterTopic(WidgetTester tester, { | |
Future<void> enterTopic(WidgetTester tester, { |
The purpose of this function is to enter a topic, as setup for whatever the calling test is going to do next, rather than to check anything.
(There is a check but it's there because we need to consume the request in order to leave things clean for the rest of the test; the check is useful for validating that it was a get-stream-topics request, so that if something unexpected happens instead of that, we aren't just confusing things by consuming a stray request.)
So because the check is incidental and not the point of the function, the name should reflect what the function is about, to let the reader at a call site focus their attention on the checks that the test is meant to focus on.
The stopwatch gets replaced with a testable stopwatch from clock/clock in tests. We need it for the testability of stopwatches with fake async. The "normal" stopwatch gets used when not testing. This implementation was inspired by: flutter/flutter@bbdeabf Signed-off-by: Zixuan James Li <[email protected]>
The debugEnable setup is borrowed from debugEnableRegisterNotificationToken. This acts as a hook to temporarily disable typing notices in later widget tests. The documentation and implementation are based on the web app: https://github.com/zulip/zulip/blob/52a9846cdf4abfbe937a94559690d508e95f4065/web/shared/src/typing_status.ts We do not expose Futures with keystroke/stopComposing. Because the caller shouldn't need to wait for the underlying API request to finish. Perhaps one nice thing we can do is defining a TypingNotifier mixin and add that to both PerAccountStore and TypingNotifier, similar to ChannelStore. But TypingNotifier is not complicated enough for that to be worthwhile. Signed-off-by: Zixuan James Li <[email protected]>
to avoid confusion with notifications for mobile apps. Signed-off-by: Zixuan James Li <[email protected]>
Once the compose box widget supports sending typing notices, TypingNotifier will send some extra requests whenever something is entered. We disable it preemptively to avoid tests being broken from not expecting those requests. Signed-off-by: Zixuan James Li <[email protected]>
Reduce some noise for a later change that implements typing notices. Signed-off-by: Zixuan James Li <[email protected]>
The `connection.takeRequests` call is not helpful when there are some unrelated requests that do not need to be checked. Signed-off-by: Zixuan James Li <[email protected]>
A request looking up topics within a channel is sent whenever the topic input gets edited. Previously, the response to this request was prepared in a general helper. We move it to a more focused helper to improve reusability, and to simplify the general helper, because only two test cases currently need this feature. This also includes a brief check that consumes the reqeust, confirming that the request is indeed the expected one. Signed-off-by: Zixuan James Li <[email protected]>
For a compose box with topic input, the field `destination` contains a mutable state computed from the topic text. This implementation is similar to hintText's to still keep everything up-to-date. When testing typing activities that do not end with a "typing stopped" notice, we need to wait for the idle timer to expire so that the test does not end with pending timers. Signed-off-by: Zixuan James Li <[email protected]>
This is a bonus feature that covers some cases that FocusNode doesn't cover. We send a "typing stopped" notice when the app loses focus or becomes invisible. An example of WidgetsBindingObserver can be found here: https://api.flutter.dev/flutter/widgets/WidgetsBindingObserver-class.html The cited text comes from the AppLifecycleState documentation: https://github.com/flutter/engine/blob/a65f1d59edc618ae81e2e8ed78d59fb729291afa/lib/ui/platform_dispatcher.dart#L1856-L1991 The link is not included in code because the code themselves are references to their documentation already. Signed-off-by: Zixuan James Li <[email protected]>
Thanks for the review! Updated the PR. |
Thanks! Looks good; merging. |
Enable keyboard dismissal by dragging the message list on both iOS and Android. This provides a consistent experience across platforms while preserving Android's built-in keyboard close button. Previously we used ScrollViewKeyboardDismissBehavior.manual on Android which didn't work effectively for dismissing the keyboard. Now users on both platforms can dismiss the keyboard by dragging the message list, matching iOS's existing behavior. The change is simple - we now use ScrollViewKeyboardDismissBehavior.onDrag unconditionally instead of switching based on platform. Please refer - [comment - 897](zulip#897 (comment)) and [Topic discussion](https://chat.zulip.org/#narrow/channel/48-mobile/topic/Dismiss.20keyboard.20and.20shift.20focus.20when.20tap.20background)
Enable keyboard dismissal by dragging the message list on both iOS and Android. This provides a consistent experience across platforms while preserving Android's built-in keyboard close button. Previously we used ScrollViewKeyboardDismissBehavior.manual on Android which didn't work effectively for dismissing the keyboard. Now users on both platforms can dismiss the keyboard by dragging the message list, matching iOS's existing behavior. The change is simple - we now use ScrollViewKeyboardDismissBehavior.onDrag unconditionally instead of switching based on platform. Please refer - [comment - 897](zulip#897 (comment)) and [Topic discussion](https://chat.zulip.org/#narrow/channel/48-mobile/topic/Dismiss.20keyboard.20and.20shift.20focus.20when.20tap.20background)
Enable keyboard dismissal by dragging the message list on both iOS and Android. This provides a consistent experience across platforms while preserving Android's built-in keyboard close button. Previously we used ScrollViewKeyboardDismissBehavior.manual on Android which didn't work effectively for dismissing the keyboard. Now users on both platforms can dismiss the keyboard by dragging the message list, matching iOS's existing behavior. The change is simple - we now use ScrollViewKeyboardDismissBehavior.onDrag unconditionally instead of switching based on platform. Please refer - [comment - 897](zulip#897 (comment)) and [Topic discussion](https://chat.zulip.org/#narrow/channel/48-mobile/topic/Dismiss.20keyboard.20and.20shift.20focus.20when.20tap.20background)
Follow-up to and stacked on top of #888
Fixes: #666