[WIP] Handle typing events from multiple users in a PM narrow #1291

Subhasish-Behera · 2023-02-07T16:01:26Z

What does this PR do?

Fixes #992

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Notes & Questions

Basically now the controller.active_conversation_info is a set which adds a sender if the event is "start" and removes if the event is "stop".
Reasons for change :-

There seems to be no requirement for Dict as It had only one key -"sender"
in dict , discarding/removing by element name is not possible (but possible in set)

Interactions

Visual changes

zulipbot · 2023-02-07T16:01:34Z

Hello @Subhasish-Behera, it seems like you have referenced #992 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #992..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

tests/core/test_core.py

tests/model/test_model.py

neiljp

@Subhasish-Behera Thanks for working on this 👍

I've left a little feedback, but I'll follow up in the stream regarding the datastructure point (dict vs set).

zulipterminal/model.py

zulipterminal/core.py

zulipterminal/model.py

mounilKshah

I tried running this PR on my local machine, and it worked well for PMs having 3 users and 2 typing simultaneously.
I had one query. All the tests currently present seem to have only one other user in the pm_with narrow, so test cases for multiple users typing simultaneously are yet to be implemented right?

Subhasish-Behera · 2023-03-24T17:17:54Z

Thanks, for your review and time @mounilKshah. Yep, I haven't committed the new tests.
(currently working on adjusting the tests after the index by user_id refactor as well as the new test.Should be complete soon)

Subhasish-Behera · 2023-03-26T18:27:23Z

@zulipbot add "PR needs review"

neiljp

@Subhasish-Behera This looks to be working fine from a manual test 👍

I left feedback inline. My main thought at this point, having not looked at the topic recently until again now just now, is that the logic would certainly be simpler if we tracked user ids instead of emails. We then have user_name_from_id to avoid diving into the internal model data, which should minimize some of the mocking.

This PR also currently has the commit text directly under the title as you had before, but that should be straightforward since you fixed other PRs already.

neiljp · 2023-05-13T21:44:43Z

zulipterminal/core.py

-            sender_name = self.active_conversation_info["sender_name"]
+            active_conversation_info = ", ".join(
+                self.model.user_dict[x]["full_name"]
+                for x in self.active_conversation_info


We should avoid single letter variable names except in extreme situations.

neiljp · 2023-05-13T21:52:48Z

zulipterminal/model.py

@@ -1381,7 +1381,7 @@ def _handle_typing_event(self, event: Event) -> None:
        if (
            len(narrow) == 1
            and narrow[0][0] == "pm_with"
-            and sender_email in narrow[0][1].split(",")
+            and sender_email in list(narrow[0][1].split(", "))


I was going to merge this change, since it's a good bugfix for >=3 people in a DM, and the rest of the PR seems to need rebasing. However, I noticed there is no accompanying test yet, so to put it differently: what (new) test case would fail without this code? We should add that to this commit.

Do we need the additional list()?

Since this is a bugfix, you can prepend that to commit title.

neiljp · 2023-05-13T22:03:54Z

tests/core/test_core.py

+                    mocker.call(
+                        [
+                            ("footer_contrast", "hamlet "),
+                            ("footer", " is typing"),
+                        ]
+                    ),


I'm a bit confused why this reformatting was necessary? If you drop the comma after the second list element, does black shrink it back again?

zulipterminal/core.py

neiljp · 2023-05-13T22:13:05Z

zulipterminal/model.py

@@ -1392,7 +1392,8 @@ def _handle_typing_event(self, event: Event) -> None:
                    controller.show_typing_notification()

            elif event["op"] == "stop":
-                controller.active_conversation_info = {}
+                sender_email = self.user_dict[sender_email]["email"]
+                active_conversation_info.discard(sender_email)


This commit doesn't yet add tests for this case, or the output the user sees.

Does the model change along show reasonable improvements? If so, you could add a test change for the model in this commit, and move the core changes into the next commit with the test you added.

neiljp · 2023-05-13T22:16:22Z

tests/core/test_core.py

+                        ),
+                    ]
+                )
+            elif length < 4:


This conditional should not be present in the test. Instead add a different parameter to the test for what you expect in each case.

neiljp · 2023-05-13T22:21:13Z

tests/model/test_model.py

+                {"claudius@zulip.com", "hamlet@zulip.com"},
+                {"claudius@zulip.com", "hamlet@zulip.com"},
+                False,
+                id="in_group_pm_narrow,usertyping:stop_" "while animation in progress",


These ids look confusing.

neiljp · 2023-05-13T22:33:15Z

tests/core/test_core.py

        ],
    )
    def test_show_typing_notification(
        self,
        mocker: MockerFixture,
        controller: Controller,
-        active_conversation_info: Dict[str, str],
+        active_conversation_info: Set[Any],


Check the types across commits - whether we settle on Set[str] as now or Set[int] for user ids, we shouldn't need to use Any now.

prah23

Thanks for the updates @Subhasish-Behera!

My feedback would be to let active_conversation_info remain a Dict with emails as the keys and full names as the values. This way, the lookup for names in the controller's show_typing_notifications can be pushed to the model and can be performed in place of the line I marked in the comment below.

zulipterminal/model.py

The strict section now matches entries and ordering from mypy --strict. Other general checking options are extracted into their own sections according to the order in the mypy --help page, except for those already in the strict section. Where the main command-line flag loosens the type-checking, these options are treated as mypy defaults, and commented in each section. This commit also starts enforcing the following, which require no changes to code: - strict_concatenate [strict] - truthy-iterable [error-code] - ignore-without-code [error-code] - unused-awaitable [error-code]

This improves the message UI by adding stream access type markers as prefixes to stream names, when rendering headers of stream messages in the messages view. This gives users an easier way to determine what type of stream the message belongs to. Tests and fixtures are updated accordingly.

The previous commit added a prefix marker for headers of stream messages; this commit does the same for headers of direct messages. No symbol was previously defined for direct messages, so one is added, and applied in a similar way as with stream messages. Tests updated.

This commit adds a stream_topic_from_message_id method which returns the topic of the currently focused message. This is done in preparation of the change in algorithm of the get_next_unread_topic function. Tests added.

This commit changes the behavior of the get_next_unread_topic method to use the current message state (ie. the current topic) in calculating the next unread topic to be cycled to. If there is no current message available, ie. an empty narrow with no focus, the logic attempts to use the current narrow to determine the best course of action. This replaces the previous approach of using a _last_unread_topic stored in the model. Tests updated.

This commit renames the "get_next_unread_topic" function to "next_unread_topic_from_message_id". This makes the function name clearer by indicating that the function argument is the context from which we get the next unread topic. Tests updated to rename the function.

The next_unread_topic_from_message_id function returns the next topic to be narrowed to. If the topic remains same, there is no need to call narrow to the same topic again. This commit fixes this, without any change in user-facing behavior. Test case updated.

This commit aims to introduce in-stream wrap-around behavior to the next_unread_topic_from_message_id function if there are unread messages still present in the current stream. Test case added.

Hotkeys document updated.

This commit shifts test IDs for test__parse_narrow_link() to be inline with the test cases, and extracts SERVER_URL into the test function body.

This commit provides support for narrow links in message content containing 'subject' instead of 'topic', which may be present in messages before server version 2.1.0. Test cases added. Fixes zulip#1422.

Minor change to reduce confusion between values of different ids in test cases and ensure they are kept distinct for testing purposes.

This picks out O_WRONLY as a misspelling, but this version of typos allows configuration in pyproject.toml, so exclude it there.

This is achieved through a new method associate_stream_with_topic in the View, which saves to a new internal dict. This will support later restoring the topic position in the UI. Since the topic list may be reordered between saving and restoring the state, the current index in the topic list is insufficient; the name of the topic is used intead.

Information regarding the previous position in the topic list for the matching stream is retrieved from the View and used to set the initial focus. A new accessor in the View, saved_topic_in_stream_id, returns the most recent (saved) topic name. In the absence of a previous value this returns None. A new internal TopicsView helper method returns the focus (index) which corresponds to the saved topic name for the related stream. If there is no saved state or no matching topic name, it returns the top index (0), which was the previous behavior. This is isolated as a method to facilitate testing. Combined, this restores any previous topic-name state by assigning to the focus position. Test added for the internal helper method _focus_position_for_topic_name.

This commit introduces the sort_unread_topics function to the next_unread_topic_from_message_id method to sort unread_topics instead of sorting it in the model. This replaces the use of bisect since the sorting behavior changes due to the change in the sort_unread_topics function. The caveat for this replacement is that the sort is performed again. This commit serves as a preparatory commit for the change in sorting behavior in the next commit. Test added.

This commit changes the sort_unread_topics method in helper.py to use the left stream panel ordering to sort the unread data instead of implicitly using the stream id as key. Tests updated & extended.

The 'Mentioned messages' and 'Starred messages' symbols are set to ASCII characters, similar to those in the Zulip web app. The selected 'All messages' symbol is the closest match found after some testing, and appears to be available fairly widely. An appropriate descriptive comment is added, as with other non-ASCII symbols, for later reference.

This commit adds prefix symbols to the main buttons in the top left corner of the UI. This aims to make the main buttons stand out more and approximate the designs used in the web app. This change also leads to a cleaner design, since these top buttons are now indented and aligned similarly to the panels beneath them, eg. the list of streams. The symbol used as a prefix in headers of direct messages is reused for the 'Direct messages' button, with other buttons using symbols added in the previous commit. The left part of the UI is increased in width to accommodate the new additions. Note that the "title" style is used to make the icons bolder, though this should be decoupled in future.

Pygments 2.16.0 introduced a style to support a combination of bold and italic styling in pygments/pygments#2444. Both of our gruvbox themes and the light native theme gain a 'bold strong' style via pygments as a result, which urwid fails to parse and blocks the application from loading. Longer-term we should improve the pygments to urwid translation logic to allow these styles to work and an upgrade to later pygments versions, but for now this allows these themes to continue working as before. Fixes zulip#1431.

The active_converstation_information is a Dict where id of the users is the key and the full_name of the user is the value. Previously the indexing was done by name which is changed to id now because of the possibility of non-unique names present in the active_conversation_info. In core.py active_conversation_info gets the full_names of the emails present in self.active_conversation_info's values.

Add a single user and remove a single user on typing event. Previously removing means emptying the dict as dict would contain a single user.But now removing means discarding a specfic id. In core.py diffrent cases of footer are made depending on no of active user. In test_core, the test now has it's own user_dict which is used to extract name for footer notifcation out of emails present in active_conversation_info. In test_model,added a new parameter of current active_conversation_info.

Add diffrernt cases of footer notification.Earlier just one test was presnt as not much scenarios were possible.

zulipbot · 2023-11-06T22:45:00Z

Heads up @Subhasish-Behera, we just merged some commits that conflict with the changes you 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.

zulipbot added the size: L [Automatic label added by zulipbot] label Feb 7, 2023

Subhasish-Behera commented Feb 7, 2023

View reviewed changes

tests/core/test_core.py Outdated Show resolved Hide resolved

Subhasish-Behera commented Feb 7, 2023

View reviewed changes

tests/model/test_model.py Outdated Show resolved Hide resolved

neiljp reviewed Feb 9, 2023

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/core.py Outdated Show resolved Hide resolved

zulipterminal/core.py Outdated Show resolved Hide resolved

zulipterminal/core.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 9, 2023

Subhasish-Behera changed the title ~~Handle typing events from multiple users in a PM narrow~~ [WIP] Handle typing events from multiple users in a PM narrow Mar 16, 2023

Subhasish-Behera force-pushed the typing2 branch 3 times, most recently from 36805f9 to 7bc09fe Compare March 22, 2023 12:45

mounilKshah reviewed Mar 24, 2023

View reviewed changes

Subhasish-Behera force-pushed the typing2 branch from 7bc09fe to 7bcb4d4 Compare March 26, 2023 17:01

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 26, 2023

zulipbot added the PR needs review PR requires feedback to proceed label Mar 26, 2023

neiljp added missing feature: user A missing feature for all users, present in another Zulip client and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Mar 29, 2023

Subhasish-Behera force-pushed the typing2 branch 2 times, most recently from d9180bc to 06cb1ec Compare April 14, 2023 08:25

neiljp reviewed May 13, 2023

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels May 13, 2023

prah23 suggested changes May 15, 2023

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

Subhasish-Behera force-pushed the typing2 branch from 06cb1ec to 0ba3cf0 Compare May 17, 2023 05:48

Subhasish-Behera force-pushed the typing2 branch 3 times, most recently from 7f1c525 to 4ceac84 Compare September 25, 2023 06:20

supascooopa and others added 26 commits October 27, 2023 19:19

model: Add method to get stream topic from message id.

bd24748

This commit adds a stream_topic_from_message_id method which returns the topic of the currently focused message. This is done in preparation of the change in algorithm of the get_next_unread_topic function. Tests added.

model: Add in-stream wrap-around behavior to next unread topic behavior.

6186519

This commit aims to introduce in-stream wrap-around behavior to the next_unread_topic_from_message_id function if there are unread messages still present in the current stream. Test case added.

keys/views: Add = key to toggle agreement with first message reaction.

0794ab7

Hotkeys document updated.

refactor: tests: buttons: Inline IDs and extract SERVER_URL.

3221a66

This commit shifts test IDs for test__parse_narrow_link() to be inline with the test cases, and extracts SERVER_URL into the test function body.

buttons: Add support for old format for narrow links.

bb99efd

This commit provides support for narrow links in message content containing 'subject' instead of 'topic', which may be present in messages before server version 2.1.0. Test cases added. Fixes zulip#1422.

refactor: tests: buttons: Separate stream & message id in parsing links.

080c4ab

Minor change to reduce confusion between values of different ids in test cases and ensure they are kept distinct for testing purposes.

requirements[dev]: Upgrade typos from ~=1.14.9 to 1.16.11.

96e404d

This picks out O_WRONLY as a misspelling, but this version of typos allows configuration in pyproject.toml, so exclude it there.

requirements[dev]: Upgrade codespell from ~=2.2.2 to ~=2.2.5.

361ddb1

pyproject/run-spellcheck: Recognize iterm & check docs/FAQ.md.

a76ad2f

core: Prompt user before exiting using PopUpConfirmationView.

981b5f3

model/helper: Use stream panel order in sort_unread_topics.

b16063d

This commit changes the sort_unread_topics method in helper.py to use the left stream panel ordering to sort the unread data instead of implicitly using the stream id as key. Tests updated & extended.

test_core: Add tests for show_typing_notification.

81e2509

Add diffrernt cases of footer notification.Earlier just one test was presnt as not much scenarios were possible.

test_model: Added tests for _handle_typing_event_2.

78b388c

Subhasish-Behera force-pushed the typing2 branch 2 times, most recently from 1c41c44 to 78b388c Compare October 27, 2023 18:30

zulipbot added the has conflicts label Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Handle typing events from multiple users in a PM narrow #1291

[WIP] Handle typing events from multiple users in a PM narrow #1291

Subhasish-Behera commented Feb 7, 2023 •

edited

zulipbot commented Feb 7, 2023

neiljp left a comment

mounilKshah left a comment

Subhasish-Behera commented Mar 24, 2023 •

edited

Subhasish-Behera commented Mar 26, 2023

neiljp left a comment

neiljp May 13, 2023

neiljp May 13, 2023

neiljp May 13, 2023

neiljp May 13, 2023

neiljp May 13, 2023

neiljp May 13, 2023

neiljp May 13, 2023

prah23 left a comment

zulipbot commented Nov 6, 2023

[WIP] Handle typing events from multiple users in a PM narrow #1291

Are you sure you want to change the base?

[WIP] Handle typing events from multiple users in a PM narrow #1291

Conversation

Subhasish-Behera commented Feb 7, 2023 • edited

zulipbot commented Feb 7, 2023

neiljp left a comment

Choose a reason for hiding this comment

mounilKshah left a comment

Choose a reason for hiding this comment

Subhasish-Behera commented Mar 24, 2023 • edited

Subhasish-Behera commented Mar 26, 2023

neiljp left a comment

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

neiljp May 13, 2023

Choose a reason for hiding this comment

prah23 left a comment

Choose a reason for hiding this comment

zulipbot commented Nov 6, 2023

Subhasish-Behera commented Feb 7, 2023 •

edited

Subhasish-Behera commented Mar 24, 2023 •

edited