Add typing indicators for streams.

[chdinesh1089]

Fixes #11152

This is almost done. Things left to be done are

• Fix and add few frontend tests.
• Document the new arguments stream_id and topic for api/v1/typing in zerver/openapi/zulip.yaml.
• Send typing notification only if the topic already exists as we want to show typing notification only in a topic narrow.

Sending a request to streams with 0 or 1 subscriber(only sender) won't throw any error. Do we want to return any error for that?

[chdinesh1089]

@showell I see this in the issue page.

This is a non-trivial feature, and you'll want to discuss performance on #backend.

Could you do a review and tell if I should open a thread for this in #backend

[showell]

I would prefer to just have a separate action function for stream notifications, so that you don't need the optional parameters and the conditional logic. Two simple functions are better than one complex one, and we're not really sharing any useful logic here.

I would also consider just having a new type (no pun intended) for the event with the value stream_typing, so that the documentation piece is easier here (again, no Union types would be needed).

[chdinesh1089]

Yes, having a different type would make it simpler and separating the function will make it seem less complex. Will make the required changes.

(Thought I'd make the changes and ask for another review soon but not I'm getting a chance to work on this as most of my time is going into academics. Leaving an update since it's been 2weeks..:)

[timabbott]

Yeah, I think the separate function makes sense, with a bit of preparatory refactoring to avoid duplicating code.

[chdinesh1089]

Separated the function. There wasn't much duplicate code.

Also added type stream_typing.

[chdinesh1089]

Sorry for the delay. Haven't been getting time due to academics. Our teachers are tending to give more assignments as everything is online 😬

Made the changes based on reviews. I'll try to do the other work i.e adding frontend tests and documenting in the weekend.

[chdinesh1089]

I haven't added/modified tests here as it's intended to test typing_status.js to which this PR makes no changes. But we make use of this module to send start/stop requests from client which only extends the possible values for status.current_recipient to also be an object consisting stream_id and topic. since we only check if current recipient is equal to previous recipient, adding tests to include recipient format of object:stream_id, topic will be redundant and won't add much value I think.

Will add few tests to also include that format for current_recipient if you think it's necessary.

[chdinesh1089]

Do we want to show "several people are typing..." only for streams or for PMs too if more than 3 users are typing? (The PR shows it for both)

[chdinesh1089]

You can ignore the below. Was not much familiar with API docs and tests, and ended up generating a wrong curl example. Fixed and added examples for python too. Didn't add any js example for stream typing as this params.to.length in zulip-js causes an error because there's no params.to in stream typing case.

---

How do I fix the API test failure? I'm unable to figure out a solution. Here's the issue:
We now added to extra params(stream_id, topic that could be sent to json/typing. The test_curl_examples is failing as it's generating an example consisting of all three parameters which gives an error according to the actual code but the test is asserting it with a success message.

Tried adding this in zulip.yaml, but it still didn't work.

        "400":
          description: Bad request.
          content:
            application/json:
              schema:
                allOf:
                  - $ref: "#/components/schemas/JsonError"
                  - example:
                      {
                        "code": "BAD_REQUEST",
                        "msg": "Bad arguments. Should have 'to' or both 'stream_id' and 'topic'.",
                        "result": "error",
                      }

I think we should still have this. will push it once we figure out a fix for the failure.

[chdinesh1089]

Added this space(and below) intentionally to make it look good in API docs with each of start, stop separated as a different chunk.

[chdinesh1089]

A write up as requested in #19200 (comment)

For the typing notifications one, I'd really appreciate your doing a bit of a writeup on the strategy for how you managed the issues around potential for duplicated code, since that's the thing I've been stuck on in thinking about that PR.

Sending start/stop notifications

This primarily relies on static/shared/js/typing_status.js for the timing part i.e when (or how often) to send start/stop. typing_status.update(worker, new_recipient), the only function used outside of its module, is called (from static/js/typing.js) every time we might want to send a notification and it takes care of the rest. The worker argument is expected to look as follows:

    const worker = {
        get_current_time,
        notify_server_start,
        notify_server_stop,
    };

notify_server_start(to) (and _stop) are just functions that send relevant requests taking in the recipient as to.

The only part that changes when we integrate stream typing notifications is, the new_recipient(and to) argument. Previously, it used an array of user IDs. Now, it can also be an object({stream_id, topic}). There's no problem with typing_status.update to support the new possibility for new_recipient. So, we do not need any functional changes in typing_status.

But the insides of notify_server_start/stop would require a few if/elses, So I've extracted those into smaller functions. I think that'll be clear on looking at typing.js.

(All this is done in the first commit)

Displaying typing... notifications for the receiver

The flow of this so far looks as follows:

1. server_events_dispatch.js calls typing_events.display_notification(event) upon receiving a typing notification
2. display_notification stores the typists into typing_data.typist_dct using typing_data.add_typists(key, sender_id). This data is later used to actually display typists based on narrow where the key in typing_data.add_typists indicates the narrow. (for e.g. it is a string of sorted user ids for group pms)
3. Actually display typing notifications using typing_events.render_notifications_for_narrow(). This also takes care of hiding notifications. This function also has some calls in narrow.js to make sure these notifications are displayed correctly when a narrow is changed.
4. Start timer to hide the notification hide_notification which also takes care of removing typists from typing_data.typist_dct using typing_data.remove_typists(key, sender_id).

The flow is the same for stream typing notifications too but now we want to store {stream_id, topic} along with typists in typing_data.typist_dct but that wouldn't be cleaner with two possible forms of key value when we rely on the key format at a few other places for fetching typists. So I added pm_typists_dict, and stream_typists_dict and removed the earlier one. The add_typist(key, typist) now becomes add_typist(key, typist, message_type) to separately store the typists into their respective dicts. Similarly for remove_typist too. The message_type is sent using event.message_type in typing_events.js.

typist_data.js also has functions for proper generation of keys for each type of message: get_pms_key, get_topic_key. These are used in a bunch of places to get items from the two dicts. I recommend checking their references.

typist_data.js also has inbound_timer_dict which keeps track of timer ids of setTimeouts to remove typists. Using this for both stream and private notifications wouldn't do any harm and keep things simple. Moreover, the wrapper functions clearing these receive the key. So, writing if/else here by creating two objects each for stream and private will only complicate things.

[zulipbot]

Heads up @chdinesh1089, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

[pdurbin]

@chdinesh1089 hi! I'd love to see typing indicators for streams and it seems like this PR of yours gets us there! Thanks! ❤️

I noticed a related topic in chat and asked about this PR there. It sounds like the next step is to rebase. I just thought I'd pass that along. Have a great weekend! 🏖️

[brijsiyag]

In the event that @chdinesh1089 has no intention of working on it, I would be interested in taking over. Thank you!!

[pdurbin]

@brijsiyag great! Locally I rebased main as of b21d93c into this branch and the merge conflicts don't look too bad. There were two auto merges and one test file that needs to be fixed manually:

Auto-merging web/src/typing.js
Auto-merging web/tests/typing_status.test.js
CONFLICT (content): Merge conflict in web/tests/typing_status.test.js

Here are the two places where typing_status.test.js has conflicts, in only a few lines:

I hope this helps either you or @chdinesh1089. ❤️

I haven't set up a dev environment yet, but maybe I can help with testing or something. 😅

Update (after setting up a dev environment): Here's how I resolved the conflicts above:

diff --git a/web/tests/typing_status.test.js b/web/tests/typing_status.test.js
index 9fea40a7f4..8e073f5b69 100644
--- a/web/tests/typing_status.test.js
+++ b/web/tests/typing_status.test.js
@@ -6,6 +6,7 @@ const {mock_esm, set_global, zrequire} = require("./lib/namespace");
 const {run_test} = require("./lib/test");
 
 const compose_pm_pill = mock_esm("../src/compose_pm_pill");
+const compose_state = mock_esm("../src/compose_state");
 
 const typing = zrequire("typing");
 const typing_status = zrequire("../shared/src/typing_status");
@@ -265,6 +266,7 @@ run_test("basics", ({override, override_rewire}) => {
     // and typing_status.state.current_recipient are the same
 
     override(compose_pm_pill, "get_user_ids_string", () => "1,2,3");
+    override(compose_state, "get_message_type", () => "private");
     typing_status.state.current_recipient = typing.get_recipient();
 
     const call_count = {

After committing I realized there were further conflicts in these files:

• web/src/typing_data.ts
• web/tests/typing_data.test.js

I'm not sure where the conflicts end, to be honest.

[chdinesh1089]

Thanks for bumping this thread, @pdurbin!

I haven't been getting much free time these days. I might delay this if I pick up again. I'd be super happy to see this feature merged soon so I'd appreciate if @brijsiyag or anyone would like to take this forward. 🙂

@pdurbin it's quite simple to setup development environment. see https://zulip.readthedocs.io/en/latest/development/overview.html

[pdurbin]

@chdinesh1089 sure. Coming from Slack and Matrix, I'm interested in this feature.

@brijsiyag are you interested in taking over? I do have a dev environment set up now (you're right, @chdinesh1089, not too bad to set up), but I'm having trouble resolving all the merge conflicts. I put a few additional details in my comment above. There are more conflicts than I realized. (I'm used to a merge workflow rather than a rebase workflow.) Also, the label "size: XL" makes me a bit nervous, as I haven't contributed before.

[brijsiyag]

@brijsiyag are you interested in taking over?

Yes @pdurbin , I'm working on this.

[timabbott]

Closing in favor of #26904, thanks for all the work on this @chdinesh1089!