Add support for TODO Widget on ZT #1549

rsashank · 2024-10-04T04:32:33Z

What does this PR do, and why?

Adds support for TODO Widget

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes #
Partially fixes issue TRACKING: Increase support for widgets (**poll**, todo, trivia, ...) #986
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

Before	After	Webapp

neiljp

@rsashank See my comments in the channel, but from quick testing this appears to render todo widgets well statically, except for changing the todo title 👍

Having the code in its own file is fine, though as I possibly commented in the other PR, the processing is something I'd consider a model feature. However, that file is large and I suspect we'll split that file in due course, so it's fine to have this file separate as long as it's clear it's not UI-related.

For find_widget_type, it seems odd to see the poll case in this PR, but more importantly note that there are other cases the code handles beyond the two expected cases. That is, it doesn't validate to only todo or poll, and the empty list is only triggering one "unknown" case. For a contrived example, consider the unexpected behavior of:

{
    ...
    "content": {"widget_type": "unknown", ...}
    ...
}

We do essentially validate that in the rendering code, but it could prove confusing depending on future widgets.

Generally I'd like to see the expected submessage types documented in api_types, even if we don't assume the data matches every time. Similarly, while you have a clear type for the return of process_todo_widget, this doesn't define the field names or types - typeddicts would make that clearer, but you could also use a dataclass or class.

Next steps

Other than handling the renaming issue, and fine-tuning the code you have, the main aspect I'd like to see is event-handling, so that todo list messages at least update from other clients, without needing to restart ZT.

rsashank · 2024-10-10T18:01:55Z

@neiljp Thanks for the feedback! I've updated the PR to handle the Submessage event, so ZT reflects updates in real-time without needing a refresh.

Thanks for pointing out the issue with the title not updating - it's fixed now! 👍

Added tests for both.

neiljp

@rsashank Thanks for the update - this looks to work very well, including the dynamic updating now 👍

This shouldn't need much more final polishing; most comments should be small changes?

Ideally the types would be added to the raw data, and we'd define the internal data we're passing between the model (widget) and UI. The latter would be preferable to include here, since you can type the new files better than with Any in more places - and you are defining new data to pass around. The API may be more complex to cover well so would be fine later instead.

neiljp · 2024-10-19T01:12:30Z

zulipterminal/model.py

+        message_id = event["message_id"]
+        if message_id in self.index["messages"]:
+            message = self.index["messages"][message_id]
+            message["submessages"].append(cast(Dict[str, Any], event))


Not needing this cast would make me feel more comfortable that we know the format of events and the original data, as per my other typing comment in this review cycle.

Understood.

I constructed the dictionary instead of casting it. The casting was necessary to satisfy mypy, not for functional reasons.

Constructing the dictionary is certainly an improvement. The aspects this doesn't cover is what 'str' are the keys in the dict, and what the 'Any' are in Message submessages. Note that this appears to be distinct from submessage events, and mypy would still be complaining about this if we went further than Dict[str, Any].

neiljp · 2024-10-19T01:13:33Z

zulipterminal/api_types.py

+class SubmessageEvent(TypedDict):
+    type: Literal["submessage"]
+
+    message_id: int
+


There's much more in this type, based on the tests and code - and that this works :)

The way you're assigning it in the event suggests there's at least some compatibility in what you expect to see between this type and those that can appear in the original submessages dict of a message - but maybe not all?

In particular

I expect there's one 'original' type (dict), and then different ones that can appear after it?

Do all the fields match between the events and those types, when you're adding them?

Can you dig out those types and add them? That would help document the Message type in this file, and make this code clearer.

Oops, got it. Added the other fields for SubmessageEvent, is this fine?

For SubmessageEvent this is fine, though we might drop id since I believe that's an event identifier which is common to all events.

Re the typing more generally, I was mainly looking to see if we could improve the submessage(s) typing in the Message typedict - I did mention that above. Combined with the definition of SubmessageEvent, then it would be clearer how compatible these two types are, including via mypy.

Reading again, some of my comments above may not be applicable, since the content is a string.

neiljp · 2024-10-19T01:46:32Z

zulipterminal/ui_tools/messages.py

+
+                if tasks:
+                    for task_id, task_info in tasks.items():
+                        task_status = "[✔ ]" if task_info["completed"] else "[  ]"


We discussed this briefly in the channel, and you indicated that the double space was to avoid overrunning the square brackets?

Do you see 'shifted' tick marks for resolved topics too?

Yes, it's actually a bit shifted for resolved topics as well

The only thing I can think of here is that the resolve-topic marker somehow has a unicode object which is setting a color or variant (if that's possible), or alternatively that it's something in the font you have locally?

🤔I tried changing a few fonts, didn't seem to make much of a difference. Does it appear well on your terminal?

Changed it to single space, I'll debug this soon.

Started #zulip-terminal>Resolved symbol not rendering for discussing this.

neiljp · 2024-10-19T01:52:00Z

tests/widget/test_widget.py

+                    "content": '{"widget_type": "todo", "extra_data": '
+                    '{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '
+                    '"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',


Nit: I'm aware these are JSON fields, but do you think there's a cleaner way of expressing this? It looks like it's doing the right thing, but it's not that clear being line-wrapped, at least for the longer ones.

Adding noqa comments is one approach, but I think breaking up the lines like this improves readability more than having a single long line.

Oh yes, a single long line would not be great for this, I was more thinking of making it more clearer to read again.

For example, ensuring the value was more distinct from the submessage keys:

Suggested change

"content": '{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',

"content": (

'{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}'

),

or even adjusting the layout of the string to make this clearer (though this is difficult to do well):

Suggested change

"content": '{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',

"content": (

'{"widget_type": "todo",'

' "extra_data": {"task_list_title": "Today\'s Tasks",'

' "tasks": [{"task": "Write code", "desc": ""}, '

'{"task": "Sleep", "desc": ""}]}}'

),

Got it! Did the first one :)

neiljp · 2024-10-19T01:52:47Z

tests/widget/test_widget.py

+                    "content": '{"type":"new_task","key":2,"task":"Eat","desc":"",'
+                    '"completed":false}',


Does this represent the real way it is formatted from the server? These look more compact than the widget_type version.

Yes, I used debug mode to retrieve.

neiljp · 2024-10-19T05:22:05Z

zulipterminal/widget.py

@@ -15,3 +15,49 @@ def find_widget_type(submessage: Any) -> str:
            return "unknown"
    else:
        return "unknown"
+
+
+def process_todo_widget(todo_list: Any) -> Tuple[str, Dict[str, Dict[str, Any]]]:


The input here is the raw data, which relates back to my other comment, though at a minimum we know it is a List or an abstract form of that (to avoid modifying it).

For the return, the Dict is one you're defining yourself, so the Any can be avoided there based on the fields you use. If you use a simpler type, mypy will have no idea if the UI code is using the names/types correctly. You can use whatever names you want in the returned data, to make it read clearly.

This is improved 👍

To clarify my point re the processed data, as per other comments, using a TypedDict (or class or dataclass) enables the str to be constrained to what you expect, as well as the value types for each key :)

Right, I'll update that in a follow-up PR, if that's fine.

zulipterminal/ui_tools/messages.py

tests/widget/test_widget.py

neiljp · 2024-10-19T05:47:55Z

zulipterminal/widget.py

+            if widget.get("widget_type") == "todo":
+                if "extra_data" in widget and widget["extra_data"] is not None:


Nit: This part could be slightly clearer and simpler in how it handles extra_data.

When is extra_data None? Is there an example/test for that? Should it be something else?

I didn't come across any instances where extra_data is None, but I added a check as a precaution. I couldn't find well-documented handling for the submessages API, so I wanted to be thorough.

neiljp

@rsashank Thanks for the update! A few minor checks left

The remaining priority for me here would be consistency of the typing names you've used so far, eg. avoiding Any and using SubMessage across various files instead of it (or Dict[str, Any]).

The other small change I saw was you are now using an or [] form in the widget processing - can we just use [] in the .get?

The message output looks fine for now.

We could improve the definition of SubMessage in a followup PR, since that would allow work on the other widget PR sooner.

Similarly, the typing of the processed widget data could be a followup improvement.

neiljp · 2024-11-01T06:29:21Z

tests/widget/test_widget.py

+                    "content": '{"widget_type": "todo", "extra_data": '
+                    '{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '
+                    '"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',


Oh yes, a single long line would not be great for this, I was more thinking of making it more clearer to read again.

For example, ensuring the value was more distinct from the submessage keys:

Suggested change

"content": '{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',

"content": (

'{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}'

),

or even adjusting the layout of the string to make this clearer (though this is difficult to do well):

Suggested change

"content": '{"widget_type": "todo", "extra_data": '

'{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '

'"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',

"content": (

'{"widget_type": "todo",'

' "extra_data": {"task_list_title": "Today\'s Tasks",'

' "tasks": [{"task": "Write code", "desc": ""}, '

'{"task": "Sleep", "desc": ""}]}}'

),

neiljp · 2024-11-01T06:44:05Z

zulipterminal/widget.py

+from typing import Any
+
+
+def find_widget_type(submessage: Any) -> str:


A few points here:

nit: for types it's fine to stick with typical class type naming, so eg. Submessage or SubMessage, instead of all upper case

the Any that was originally passed in here is the submessages entry of a Message, so it should have the same typing, and can use the same name/type, so can go in api_types

it can also be applied to various other places where you seem to still use Any?

as per another comment recently, a typeddict will pin down the valid keys (str) and value types per key, which is much more specific and clean than Dict[str, Union[int, str]]

the core structure of SubMessage may be common among all the widgets, so we may not need TodoSubMessage at this point

... but where it may be useful is if we were to define the different layouts of the content when it is extracted out of json form

neiljp · 2024-11-01T06:56:40Z

zulipterminal/widget.py

@@ -15,3 +15,49 @@ def find_widget_type(submessage: Any) -> str:
            return "unknown"
    else:
        return "unknown"
+
+
+def process_todo_widget(todo_list: Any) -> Tuple[str, Dict[str, Dict[str, Any]]]:


This is improved 👍

To clarify my point re the processed data, as per other comments, using a TypedDict (or class or dataclass) enables the str to be constrained to what you expect, as well as the value types for each key :)

zulipterminal/ui_tools/messages.py

neiljp · 2024-11-01T07:26:27Z

@rsashank We may need some followups on widget messages, such as limiting direct editing? I'm not sure what else may be necessary at this point.

rsashank · 2024-11-03T20:36:47Z

@neiljp Thanks for the feedback! Updated this PR 👍

We may need some followups on widget messages, such as limiting direct editing? I'm not sure what else may be necessary at this point.

Isn't editing on widget messages already restricted? I could be mistaken, but I can't seem to edit.

Added find_widget_type to identify the type of widget present in the message (e.g., polls, todo, etc.). Added test.

neiljp · 2024-11-04T05:34:17Z

@rsashank Thanks for the update; this looks good to go now - the remaining elements can be fine-tuned later 🎉

Added process_todo_widget function to process submessages containing a todo widget. Returns the title of the widget along with tasks and their state (completed/uncompleted) as a dict. Tests added.

Updated tests.

Added _handle_submessage_event method to Model to handle new changes to widgets (polls, todo etc.). Tests added.

neiljp · 2024-11-04T05:49:28Z

@rsashank You should be able to confirm using git range-commit, but I only updated the commit text slightly before pushing back here.

neiljp · 2024-11-04T20:34:59Z

I've started #zulip-terminal>Widget followup work to discuss what seem to be the outstanding points after this work.

zulipbot added the size: XL [Automatic label added by zulipbot] label Oct 4, 2024

rsashank force-pushed the todowidget branch 5 times, most recently from ac81d5c to 6587636 Compare October 4, 2024 05:00

rsashank added the area: widgets label Oct 4, 2024

rsashank force-pushed the todowidget branch from 6587636 to 76f236e Compare October 4, 2024 11:31

neiljp reviewed Oct 5, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Oct 5, 2024

rsashank mentioned this pull request Oct 9, 2024

Add support for Polls Widget on ZT. #1551

Open

18 tasks

rsashank force-pushed the todowidget branch 2 times, most recently from 433da5e to daaf808 Compare October 10, 2024 17:56

rsashank force-pushed the todowidget branch from daaf808 to 6f23e83 Compare October 10, 2024 18:02

rsashank 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 Oct 13, 2024

neiljp reviewed Oct 19, 2024

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 Oct 19, 2024

neiljp added this to the Next Release milestone Oct 19, 2024

rsashank force-pushed the todowidget branch 3 times, most recently from 721a9e5 to 88719fb Compare October 25, 2024 11:53

rsashank 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 Oct 25, 2024

neiljp reviewed Nov 1, 2024

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 Nov 1, 2024

rsashank force-pushed the todowidget branch from 88719fb to 82d057c Compare November 3, 2024 20:33

rsashank 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 Nov 3, 2024

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Nov 4, 2024

widget: Add find_widget_type function.

9d3afe6

Added find_widget_type to identify the type of widget present in the message (e.g., polls, todo, etc.). Added test.

rsashank added 5 commits November 3, 2024 21:42

widget: Add process_todo_widget function.

086985e

Added process_todo_widget function to process submessages containing a todo widget. Returns the title of the widget along with tasks and their state (completed/uncompleted) as a dict. Tests added.

messages: Implement logic to display TODO widgets.

f547b99

widget: Add support for updating title in process_todo_widget.

2066b77

Updated tests.

api_types: Add SubmessageEvent TypedDict.

0096e3d

model: Add handling for Submessage events.

0cd0feb

Added _handle_submessage_event method to Model to handle new changes to widgets (polls, todo etc.). Tests added.

neiljp force-pushed the todowidget branch from 82d057c to 0cd0feb Compare November 4, 2024 05:45

neiljp merged commit d88adae into zulip:main Nov 4, 2024
21 checks passed

-                    "content": '{"widget_type": "todo", "extra_data": '
-                    '{"task_list_title": "Today\'s Tasks", "tasks": [{"task": '
-                    '"Write code", "desc": ""}, {"task": "Sleep", "desc": ""}]}}',
+                    "content": (
+                        '{"widget_type": "todo",'
+                        ' "extra_data": {"task_list_title": "Today\'s Tasks",'
+                        ' "tasks": [{"task": "Write code", "desc": ""}, '
+                        '{"task": "Sleep", "desc": ""}]}}'
+                    ),

		"content": '{"type":"new_task","key":2,"task":"Eat","desc":"",'
		'"completed":false}',

		if widget.get("widget_type") == "todo":
		if "extra_data" in widget and widget["extra_data"] is not None:

		from typing import Any


		def find_widget_type(submessage: Any) -> str:

Add support for TODO Widget on ZT #1549

Add support for TODO Widget on ZT #1549

Conversation

rsashank commented Oct 4, 2024

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

neiljp left a comment

Choose a reason for hiding this comment

Next steps

rsashank commented Oct 10, 2024

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

rsashank Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

neiljp Nov 4, 2024 • 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

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

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

neiljp commented Nov 1, 2024

rsashank commented Nov 3, 2024

neiljp commented Nov 4, 2024

neiljp commented Nov 4, 2024

neiljp commented Nov 4, 2024

rsashank Nov 3, 2024 •

edited

Loading

neiljp Nov 4, 2024 •

edited

Loading