Skip to content
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

Support setting typing status in more situations #1033

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Oct 31, 2024

A follow-up to #897. This will cover more types of interactions described here.

Specifically, this will track interactions with compose buttons and autocomplete scroll view, and send typing notices.

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:

  https://github.com/flutter/flutter/pull/137381/commits/ bbdeabf74b8e9443f0156b4631d460f022ac598c

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]>
This ensures that the `GetStreamTopicsResult` is self-contained, so
that the caller does not rely on the assumption that a response has
been prepared through `prepareComposeBox`.

This also elevates contentInputFinder for reuse throughout the test.

This changes the way the tests work, because now we use tester.enterText
to set the topic and content, instead of accessing the controllers.

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]>
This further simplifies `ComposeBoxLayout` and free it from passing the
controllers around. This makes it easier as we might add more parameters
to the compose button constructors.

Signed-off-by: Zixuan James Li <[email protected]>
Universally add a Duration.zero delay, so that the result does not
get returned until the microtask completes.  This helps make the tests
more realistic and flexible.

Signed-off-by: Zixuan James Li <[email protected]>
Previously, when the content input field is empty, this would notify
the listeners before the placeholder text got added.  This caused
listeners to send an unintended stoppedComposing notification.

Signed-off-by: Zixuan James Li <[email protected]>
The new _ChannelComposeButtonRow needs to be stateful because changes to
the topic input affects the typing destination.  This is similar to
_StreamContentInput.

We send "typing started" notices as long as the user presses any of the
compose buttons, regardless if they proceed to attach files or not.
This will later apply to other kinds of compose buttons, too.

Signed-off-by: Zixuan James Li <[email protected]>
This gives more vertical space above the compose box.  This will provide
enough space for the autocomplete to appear without overflowing when we
add tests for it.

Signed-off-by: Zixuan James Li <[email protected]>
When selecting from the list of autocompletion, the user is still
considered to be actively composing.

Fixes: zulip#666
@PIG208
Copy link
Member Author

PIG208 commented Oct 31, 2024

An example comparison for the compose box tests before and after 7a29e50:

before after
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant