Show sender's presence status in message content header #987

Ezio-Sarthak · 2021-04-07T04:41:03Z

What does this PR do?!

Adds sender's presence status marker in a message content header

Commit flow

Centralize the state icons in ui_mappings.py (refactor)
Add state marker as a prefix to sender names in the message header.
Add helper in MessageBox to update author field if present.
Add a method to render presence markers in messages regularly.

Screenshots/GIFs

Fixes #896.

Ezio-Sarthak · 2021-04-11T14:12:22Z

@zulipbot add "PR needs review"

preetmishra

@Ezio-Sarthak Thanks for working on this! I have tested this locally and it seems to work well. 👍

I have left some quick comments for the first review iteration. Let's squash the last commit as you mention in the PR description.

zulipterminal/helper.py

preetmishra · 2021-04-18T14:02:44Z

zulipterminal/helper.py

@@ -101,6 +107,14 @@ class UnreadCounts(TypedDict):
 }


+STATE_ICON = {


Should we call it STATE_ICONS instead?

Hmm. Seems more apt 👍

I was also thinking if extracting this in symbols would be more suitable?

When naming like this, it's good to think of how it's used - STATE_ICONS['active'] suggests there are more than one active one? Generally I err towards the non-plural for this reason.

Also the values are icons, but the keys aren't - so as always, naming is tricky to get perfect!

This relates application state values to representation, a little like edit_mode_captions, so perhaps it belongs in with that, though I'm not keen on helper.py for that.

Just to resolve this discussion, I've migrated the STATE_ICON to ui_mappings.py as added to main recently via #1013.

zulipterminal/ui_tools/boxes.py

neiljp

@Ezio-Sarthak I do enjoy this feature, so wanted to give some more focused feedback.

Our (event+actions)->message-view handling is something I can ideally see being refactored soon, but what you have here does seem to work fine for the most part 👍

neiljp · 2021-04-24T18:02:45Z

zulipterminal/ui_tools/boxes.py

+                email = self.message.get('sender_email', '')
+                user = self.model.user_dict.get(email, None)


We will want to refactor this in future as we make changes to the user/presence details to improve encapsulation and move towards using ids for lookup.

Sure. I'll add a relavant TODO note 👍

neiljp · 2021-04-24T18:07:09Z

tests/ui/test_ui_tools.py

+        ([STATUS_INACTIVE, 'alice', ' ', 'DAYDATETIME'],
+         {'sender_full_name': 'bob'}),
+        ([' ', ' ', ' ', 'DAYDATETIME'], {'timestamp': 1532103779}),
+        ([STATUS_INACTIVE, 'alice', ' ', 'DAYDATETIME'],
+         {'timestamp': 0}),


I'm not sure whether this is the right test to add it to, but we're not extending the testing to cover the range of activity markers, or being a bot?

Yeah. Currently, in the PR, I've marked all users (including bots) as per their activity. Thus, a bot would currently be marked inactive.

Yes, my intent was to just amend the tests re header content to be shown (currently), not testing the activity marker rendering in particular.

tests/ui/test_ui_tools.py

neiljp · 2021-04-24T18:25:20Z

zulipterminal/ui_tools/views.py

+            if is_sender_header:
+                self.model._update_rendered_view(msg_box.message['id'])


The new method you've added does a lot of digging into MessageBox - it seems like it would be simpler to have the logic inside that class/object?

I'm also not sure that model._update_rendered_view is required here, or that maybe we can try a different approach. Most of the explicit logic there is more for reordering - though other handlers do use it for updating, albeit one message at a time. Given the way that the latter method works, this is going to filter every message and re-generate/render them all, one by one. Each box does need updating, but maybe we can add a method to MessageBox to just update the one element?

I'm slightly concerned with the asynch here, as it's yet another function updating the message list in addition to events, conceptually in parallel.

Separated message box digging in 3rd commit.
Proposed a better approach for rendering in the last commit

Ezio-Sarthak · 2021-04-27T07:17:56Z

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

preetmishra

@Ezio-Sarthak Thanks for the update! This continues to work well. 👍

I have left some minor in-line comments. Other than that, this looks pretty close.

We could log issues for partial updates and also the bot presence marker depending upon whether you would take them next once the PR is merged.

zulipterminal/ui_tools/boxes.py

preetmishra · 2021-06-26T12:06:07Z

zulipterminal/ui_tools/boxes.py

+        update_required = False
+
+        if self.need_recipient_header():
+            update_required = self[1][1].text != " "


Could we store self[1][1] in a variable before so that it is more clear what is being checked?

zulipterminal/ui_tools/boxes.py

preetmishra · 2021-06-26T12:14:45Z

tests/ui/test_ui_tools.py

+        update_required,
+        mocker,
+        to_vary_in_each_message,
+        header_needed,


header_needed and mocker are never used in the method body?

preetmishra · 2021-06-26T12:20:31Z

zulipterminal/ui_tools/views.py

@@ -547,6 +547,18 @@ def get_next_unread_pm(self) -> Optional[int]:
            return pm
        return None

+    def update_msg_list_presence_markers(self) -> None:
+        for index in range(len(self.body.log)):


Since you're using self.body.log more than once, you could extract it in a variable.

Yes, though I think it would be not helpful for the line L558 (where we're updating self.body.log itself)?

Or do you mean updating the whole message list outside the loop in the end:
self.body.log = msg_list (if msg_list is the variable for instance) ?

I see that you're only mutating the attribute inside the loop, so extracting self.body.log would work.

(Assuming by not helpful you meant it is not going to update the original log.)

preetmishra

@Ezio-Sarthak Thanks for incorporating the changes. This seems to work well. 👍

@neiljp This looks good from my end.

neiljp

@Ezio-Sarthak Thanks for the last update - just a few points inline left I hope :)

@preetmishra If you are satisfied with the points you raised in the review, can you resolve those?

neiljp · 2021-07-01T21:47:13Z

zulipterminal/ui_tools/views.py

+    def update_msg_list_presence_markers(self) -> None:
+        for msg_btn in self.body.log:


Let's aim to use message since we have space. Is the loop variable more accurately a widget wrapper around a message_box, not a 'button'?

neiljp · 2021-07-01T21:58:29Z

zulipterminal/ui_tools/views.py

+            # Update a message box content header if required.
+            msg_btn.original_widget.update_msg_content_header()


Would message_box = widget.original_widget and message_box.update_content_header() remove the need for a comment?

neiljp · 2021-07-01T22:00:28Z

zulipterminal/ui_tools/boxes.py

+            # FIXME: Render specific element (here content header) instead?
+            super().__init__(self.main_view())
+
+        return update_required


Do we need this? Ah, you're using this in the test?

neiljp · 2021-07-01T22:21:13Z

zulipterminal/ui_tools/boxes.py

+        Update the content header of a message box (if required).
+        """
+        update_required = False
+        content_header = None


This isn't the content_header, but the author field? If so, let's name it that? Maybe set author_column = 1 and use that in the indexing to make it clearer?

neiljp · 2021-07-01T22:24:52Z

zulipterminal/ui_tools/boxes.py

+        # Update the content header only if it's present and non-empty.
+        if content_header is not None:
+            update_required = content_header.text != " "


It's not obvious where the " " test comes from - it's the default author text, when there's no author in the header, right? That's partly unclear due to the content_header variable name, but "non empty" and "content header" just repeat the code, so if we're going to include a comment, let's make it more useful - or set a variable like author_is_present = ... to help.

Makes sense. I'll go with author_is_present and skip the comment 👍

neiljp · 2021-07-01T22:26:06Z

zulipterminal/ui_tools/boxes.py

+
+        if update_required:
+            # Re initialize the message if update is required.
+            # FIXME: Render specific element (here content header) instead?


This doesn't seem too much of an issue performance-wise right now, but let's open an issue for improving this.

neiljp · 2021-07-01T22:35:31Z

tests/ui/test_ui_tools.py

@@ -2211,6 +2211,52 @@ def test_main_view_generates_EDITED_label(
            assert label[0].text == "EDITED"
            assert label[1][1] == 7

+    @pytest.mark.parametrize(
+        "message",


This is specifically a stream message, though the behavior shouldn't vary across message types. Can we add a note here regarding that we should refactor this? We have other tests with a single parametrize entry, but it's a little misleading.

Yeah, I used basically it as base message template, which I'm modifying as per the to_vary_in_each_message parameters. Surely I'll add a note here 👍.

preetmishra · 2021-07-02T13:25:08Z

@neiljp I don't have the "resolve" button available on my end.

This commit centralizes the use of state icons corresponding to a user status in `ui_mappings.py` as `STATE_ICON`.

This commit adds a status marker right before the sender's name in a message content header. The users' dict from model is used to get status of the sender. The default status of sender is kept `inactive`. NOTE: Currently bots are marked as `inactive`. A later version would be to render markers for bots as well. Tests amended (Message content header related).

neiljp · 2021-07-04T03:10:02Z

@preetmishra No "resolve conversation"? If so, the GitHub UI is even less effective than I thought in this regard :/

This commit adds a method to update author status of a specific message box (if required). The current logic re-builds the whole message box if an update is required. This might need improvement as to update a specific element only. (Though that might prove useful only if that element is rendered via a dedicated method). Tests added.

This commit: * introduces a method `update_message_list_status_markers` in middle column view class. It would be responsible for rendering the status markers (if required) in the message list. * adds presence renderer function to the handler `start_presence_updates`. This way, the method `update_message_list_status_markers` is called every minute. The helper `update_message_list_status_markers` uses MessageBox method `update_message_author_status` (added in previous commit) to update message boxes if author field is present. Fixes zulip#896.

neiljp · 2021-07-04T21:44:41Z

@Ezio-Sarthak Thanks for making the updates - this will be great to use! I'm going to merge shortly 🎉

Thoughts for some follow-ups that we should discuss and implement promptly or file as issues: (some we discussed already)

Show bot markers (this was also suggested in my role-markers PR)
Handle "unavailable" visibility and explicit changes (are they events?)
Only update the status rather than the entire box - maybe not that tricky?
Show user presence in PM compose?
Show user presence in user mentions?

zulipbot added size: L [Automatic label added by zulipbot] area: UI General user interface update enhancement New feature or request labels Apr 7, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from fca75c4 to 067ff37 Compare April 7, 2021 15:58

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 7, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch 3 times, most recently from 56ef2d0 to 590391d Compare April 10, 2021 08:35

zulipbot added the PR needs review PR requires feedback to proceed label Apr 11, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from 590391d to d1b740e Compare April 17, 2021 03:37

preetmishra reviewed Apr 18, 2021

View reviewed changes

preetmishra 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 Apr 18, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch 2 times, most recently from 8db0ba9 to d47ee08 Compare April 19, 2021 09:12

neiljp reviewed Apr 24, 2021

View reviewed changes

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from d47ee08 to b844b8c Compare April 27, 2021 03:49

Ezio-Sarthak requested review from neiljp and preetmishra April 27, 2021 04:19

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 27, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from b844b8c to 62fc329 Compare April 28, 2021 10:54

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from 62fc329 to 776d795 Compare May 29, 2021 08:36

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch 4 times, most recently from 8a7dfc3 to 7a7883e Compare June 22, 2021 03:53

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from 7a7883e to b1be9b6 Compare June 25, 2021 17:17

preetmishra reviewed Jun 26, 2021

View reviewed changes

preetmishra 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 Jun 26, 2021

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from b1be9b6 to 5d2b7e4 Compare June 26, 2021 17:22

Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 26, 2021

preetmishra reviewed Jun 27, 2021

View reviewed changes

neiljp reviewed Jul 1, 2021

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 Jul 1, 2021

Ezio-Sarthak added 2 commits July 4, 2021 08:02

refactor: views/ui_mappings: Extract status markers in a constant dict.

a81c247

This commit centralizes the use of state icons corresponding to a user status in `ui_mappings.py` as `STATE_ICON`.

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from 5d2b7e4 to 0f696ba Compare July 4, 2021 09:08

Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 4, 2021

Ezio-Sarthak and others added 2 commits July 5, 2021 00:11

Ezio-Sarthak force-pushed the user-presence-stats-in-msg branch from 0f696ba to 9bc51fb Compare July 4, 2021 18:51

neiljp merged commit cb5b7ae into zulip:main Jul 4, 2021

neiljp added this to the Next Release milestone Jul 4, 2021

neiljp mentioned this pull request Feb 28, 2022

Added '•' in front of active senders #501

Closed

Ezio-Sarthak mentioned this pull request Feb 28, 2022

Improvement of presence markers UI #1155

Open

6 tasks

		@@ -101,6 +107,14 @@ class UnreadCounts(TypedDict):
		}


		STATE_ICON = {

		email = self.message.get('sender_email', '')
		user = self.model.user_dict.get(email, None)

		if is_sender_header:
		self.model._update_rendered_view(msg_box.message['id'])

		def update_msg_list_presence_markers(self) -> None:
		for msg_btn in self.body.log:

		# Update a message box content header if required.
		msg_btn.original_widget.update_msg_content_header()

Show sender's presence status in message content header #987

Show sender's presence status in message content header #987

Conversation

Ezio-Sarthak commented Apr 7, 2021 • edited Loading

Ezio-Sarthak commented Apr 11, 2021

preetmishra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak commented Apr 27, 2021

preetmishra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetmishra Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

preetmishra left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

preetmishra commented Jul 2, 2021

neiljp commented Jul 4, 2021

neiljp commented Jul 4, 2021

Ezio-Sarthak commented Apr 7, 2021 •

edited

Loading

Ezio-Sarthak Apr 19, 2021 •

edited

Loading

Ezio-Sarthak Apr 26, 2021 •

edited

Loading

preetmishra left a comment •

edited

Loading

preetmishra Jun 26, 2021 •

edited

Loading

Ezio-Sarthak Jul 4, 2021 •

edited

Loading