[WIP] Add help text on how to interact with message links. #1478

Sushmey · 2024-03-28T18:21:28Z

What does this PR do, and why?

External discussion & connections

Discussed in Open external links with browser #T1440
Fully fixes #
Partially fixes issue Message information popup actions (open, copy, ...) vs content (narrows, links, code, ...) #1372
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

neiljp

@Sushmey I left some preliminary feedback. Please let me know here or on czo if you have points that I've not answered.

zulipterminal/ui_tools/views.py

neiljp · 2024-04-03T19:54:48Z

zulipterminal/ui_tools/views.py

                    widgets.append(
                        urwid.AttrWrap(strip, None if index % 2 else "popup_contrast")
                    )


This may be better moved into the loop:

updating the the loop index in the loop is confusing

do we want the category wrapped in popup_contrast?

maybe we could style the second column title differently to the category title?

Yes updating the loop index is confusing. Figured out a better way to get what I expected.

No we don't need to wrap the popup_contrast. I did it because I was trying to make it visually discernible, updated it to work without the wrap.

We can have a discussion about the style.

We don't necessarily need to think about a style in this first version, but at the time it struck me as an additional motivator for adjusting the loop.

zulipterminal/ui_tools/views.py

neiljp · 2024-04-06T19:17:48Z

I believe I covered your questions so far in the stream or here.

neiljp

@Sushmey It'd be great to get the category headings infrastructure merged, so I left some more feedback on what you have here 👍

Please do reply if you have other issues with the typing.

tests/ui_tools/test_popups.py

neiljp · 2024-04-14T21:15:39Z

tests/ui_tools/test_popups.py

+        [
+            "contents",
+            "column_widths",
+            "expected_instance",


instance is rather vague - what does this function return?

I believe each part of the instance is essentially a row, which we don't mention in the docstring and maybe in the code that uses it, but could be a useful refactor?

Changed the parameter name to expected_widget_type to be more descriptive since the docstring also mentions that the function is returning list of widgets.

Re second part, are you saying each widget of the list is essentially a row and that the docstring needs a refactor? I feel the current docstring is good enough, it does return a list of widgets that is used to make the table.

Using widget is consistent with the current docstring and terms used 👍

The second point was my noting that the code might be more readable if we referred to the returned data as rows; widgets are any UI element, so are less specific.

neiljp · 2024-04-14T21:20:25Z

tests/ui_tools/test_popups.py

+            "widget_with_str",
+        ],
+    )
+    def test_make_table_with_categories(


Having this test is great, but it does focus on the urwid form of the output, so it may be worth adding a suffix to the test name to emphasize that, eg. __instances based on your naming elsewhere (which can be improved).

Adding an __instances suffix. Although I wonder if this implies there's a test that focuses on make_table_with_categories as a whole and not on some specific thing like instances.

I agree that it could suggest another test exists, but also means it won't conflict with another later ;) If there are multiple tests for a function, my preferred form now is to separate the function and the intention behind the test.

I suggested __instances since that's what you were checking in the test, and used in the expected part of the test case name, as per another comment - so let's sync them up if you think the updated expected parameter name is better :)

I'e used types everywhere instead of instance because it seems more intuitive than instances. So let's change it to __types in the function name also?

Yeah, that'd be good 👍

zulipterminal/ui_tools/views.py

neiljp · 2024-04-14T21:30:34Z

Should the linked issue be #1372, not #1002?

This is not a part of this PR. This is just to resolve issue zulip#1226.

neiljp

@Sushmey Glad to see you got the types sorted 👍 There seems to be a failing test? Maybe try check-branch locally, to see what commit is failing?

I replied to some previous comments and left a few more inline.

The PR could do with removing the exploratory parts, eg. commented out, and that would make it clearer what exactly is changing.

neiljp · 2024-04-25T20:18:56Z

tests/ui_tools/test_popups.py

+        widgets = self.pop_up_view.make_table_with_categories(contents, column_widths)
+        for index, widget in enumerate(widgets):
+            assert isinstance(widget, expected_widget_type[index])


My only thought on rereading this would be to suggest zip, since it's clearer.

One caveat with zip is that it pauses at the shorter, but we should test that the length is the same in any case, since that would be a bigger issue.

zip is used to combine two iterables, right? Not sure how we can use that here.

Edit: Just realised what you meant.

assert len(widgets) == len(expected_widget_type) for widget, expected_type in zip(widgets,expected_widget_type): assert isinstance(widget, expected_type)

Yep, I already saw you changed it in the code :)

tests/ui_tools/test_popups.py

neiljp · 2024-04-25T20:37:04Z

zulipterminal/ui_tools/views.py

@@ -1603,7 +1611,7 @@ def __init__(
        full_raw_message_keys = "[{}]".format(
            ", ".join(map(str, display_keys_for_command("FULL_RAW_MESSAGE")))
        )
-        msg_info = [
+        msg_info: PopUpViewTableContent = [


I know I mentioned this might be an option previously, but you should be able to skip this and therefore not need the isinstance assert further below.

Without the explicit typing, mypy will know it's a list, so accept that it's mutable (modifiable).

It doesn't work otherwise since PopUpViewTableContent also has types which doesn't support append so need an explicit assertion.
I can't not explicitly type it as PopUpViewTableContent since Mypy assumes it to be the type of the first data assigned to it.

neiljp · 2024-04-25T20:51:18Z

zulipterminal/ui_tools/views.py

+                if isinstance(category, tuple) and isinstance(content, list):
+                    content.append(category)


I didn't pick up on this before, but the isinstance check on the content allows mypy to treat the content as mutable. That allows the code you have to work, but also makes the use of Sequence pointless - the code here does not work unless it is specifically a list being passed in.

Is there a reason the other style of category heading cannot be added to the widgets here, rather than copied into the content?

Thanks for pointing it out. There was no reason to put it as content. Changed it and removed the condition too.

Updating the function `make_table_with_categories`. So that right aligned hint text can be appended to heading.

Add help text.

Amend `calculate_table_widths` to add case of category with help text.

zulipbot added the size: M [Automatic label added by zulipbot] label Mar 28, 2024

Sushmey marked this pull request as draft March 29, 2024 18:11

neiljp reviewed Apr 3, 2024

View reviewed changes

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

Sushmey force-pushed the clarify-message-link-interaction branch from e2d625f to 8916629 Compare April 6, 2024 11:02

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Apr 6, 2024

neiljp reviewed Apr 14, 2024

View reviewed changes

neiljp added the area: UI General user interface update label Apr 14, 2024

Sushmey force-pushed the clarify-message-link-interaction branch from 8916629 to 62008af Compare April 18, 2024 13:29

views: Comment this to resolve issue zulip#1226.

324ccaf

This is not a part of this PR. This is just to resolve issue zulip#1226.

Sushmey force-pushed the clarify-message-link-interaction branch 2 times, most recently from 5a41187 to fbad442 Compare April 18, 2024 23:23

Sushmey force-pushed the clarify-message-link-interaction branch from fbad442 to 09b5850 Compare April 25, 2024 17:47

neiljp reviewed Apr 25, 2024

View reviewed changes

Sushmey added 5 commits April 27, 2024 16:34

tests: ui_tools: Add test for make_table_with_categories.

ad4d8f3

views: Amend the function to add help text.

56bfda6

Updating the function `make_table_with_categories`. So that right aligned hint text can be appended to heading.

views: Add help text on how to interact with message links.

c767302

Add help text.

tests: ui_tools: Add test for calculate_table_width.

d903191

views: Amend function to calculate width with help text.

8ba5ad7

Amend `calculate_table_widths` to add case of category with help text.

Sushmey force-pushed the clarify-message-link-interaction branch from 09b5850 to 8ba5ad7 Compare April 27, 2024 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add help text on how to interact with message links. #1478

[WIP] Add help text on how to interact with message links. #1478

Sushmey commented Mar 28, 2024 •

edited

Loading

neiljp left a comment

neiljp Apr 3, 2024

Sushmey Apr 6, 2024

neiljp Apr 6, 2024

neiljp commented Apr 6, 2024

neiljp left a comment

neiljp Apr 14, 2024

Sushmey Apr 18, 2024

neiljp Apr 25, 2024

neiljp Apr 14, 2024

Sushmey Apr 18, 2024

neiljp Apr 25, 2024

Sushmey Apr 27, 2024

neiljp Apr 27, 2024

neiljp commented Apr 14, 2024

neiljp left a comment

neiljp Apr 25, 2024

Sushmey Apr 27, 2024 •

edited

Loading

neiljp Apr 27, 2024

neiljp Apr 25, 2024

Sushmey Apr 27, 2024

neiljp Apr 25, 2024

Sushmey Apr 27, 2024

		if isinstance(category, tuple) and isinstance(content, list):
		content.append(category)

[WIP] Add help text on how to interact with message links. #1478

Are you sure you want to change the base?

[WIP] Add help text on how to interact with message links. #1478

Conversation

Sushmey commented Mar 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Apr 6, 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

neiljp commented Apr 14, 2024

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushmey Apr 27, 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

Sushmey commented Mar 28, 2024 •

edited

Loading

Sushmey Apr 27, 2024 •

edited

Loading