Handle topic links in message info view #867

Ezio-Sarthak · 2021-01-06T05:15:59Z

This PR is a headstart in handling Topic links in a stream topic.

Commit flow:

Generate link buttons in message info view via a static method (Refactor).
Add API return types for new and old topic links' responses.
Convert message responses to support modern formats (in general) via a model helper (Refactor).
Topic links stored by parsing into OrderedDict (like message links).
Topic links appended in the message info view.

Fixes #709.

View:

neiljp

@Ezio-Sarthak Thanks for looking into this - when we have the open-in-browser feature this will be really helpful 👍

This works OK for me on czo but I think it needs generalizing for Zulip 2.1+, and could be simpler both in code and possibly in commit structure too. Let me know if you have any questions.

zulipterminal/ui_tools/views.py

tests/conftest.py

neiljp · 2021-01-17T00:39:49Z

tests/conftest.py

@@ -222,7 +222,7 @@ def unicode_emojis():
    'stream_id': 205,
    'subject': 'Test',
    'reactions': [],
-    'subject_links': [],
+    'topic_links': [],


Note that we aim to support Zulip 2.1+, which is prior to feature level 1, so we need to support subject_links as well as topic_links.

Given the text below, you might consider whether we can refactor more tests to use the same message templates - so we don't need to make as many changes in multiple places.

For now, I've added topic_links wherever we have subject_links so that there is no version support issue. I've done this keeping in mind my reply to the comment below.

Both are never present together, unless there was some overlap in feature levels? We could potentially make these fixtures over feature-level, at the risk of getting more complex.

Makes sense. My suggestion would be that we let subject_links stay untouched for now in the tests and fixtures. We're validating for both *_links parameter in the helper of later commit, in such a way that only one could possibly exist (I don't know why I didn't consider that for this commit 😆)

(By untouched I'm not considering my below comment regarding PMs and group PMs)

neiljp · 2021-01-17T00:52:06Z

zulipterminal/ui_tools/boxes.py

+            return
+        for link in self.message['topic_links']:
+            self.topic_links[link] = (
+                'Link', len(self.topic_links) + 1, True


Not having the link text available is a little frustrating ☹️

Perhaps the backend could be improved in this regard, as at least for linkifiers like we regularly use, we know there is "link text", like #T867 here.

For now could we maybe special-case the view for empty link text and put it on one line?

neiljp

@Ezio-Sarthak I just realized I had an outstanding set of responses to this PR so sending them now - let me know if they still make sense 👍

zulipterminal/ui_tools/views.py

neiljp · 2021-01-31T07:07:26Z

tests/conftest.py

@@ -246,7 +247,6 @@ def unicode_emojis():
    'reactions': [],
    'type': 'private',
    'avatar_url': 'dummy_avatar_url',
-    'subject_links': [],


I just checked a connection with czo and it seems PMs send topic_links?

Ideally, this shouldn't be the case 🤔, as there's no such 'topic' in PMs (or group PMs) as you mentioned earlier. This could be potentially filed in the main repo in any case, if not done already.

EDIT: Looking through a successful JSON response for get_messages API, I see the subject parameter is said to be present only for stream messages but is present in form of an empty string for PMs too. Although an empty string/list results in a fallacy of the conditional statements, I feel they sort of mislead a user looking over the API for the first time :)

Since we should sync with our API in any case, I'll let subject links remain there for now if that seems reasonable.

neiljp · 2021-01-31T07:09:34Z

tests/conftest.py

@@ -222,7 +222,7 @@ def unicode_emojis():
    'stream_id': 205,
    'subject': 'Test',
    'reactions': [],
-    'subject_links': [],
+    'topic_links': [],


Both are never present together, unless there was some overlap in feature levels? We could potentially make these fixtures over feature-level, at the risk of getting more complex.

zulipterminal/ui_tools/boxes.py

zulipterminal/api_types.py

Ezio-Sarthak · 2021-02-17T15:24:33Z

@neiljp Thanks for the review! Your outstanding comments made us discuss client + server related aspects of the PR in detail :)

The motive of this PR would get complete potentially once @sumanthvrao's PR zulip/zulip#17124 gets merged. Till then, we might continue resolving refactor/implementation/code related aspects 😅

EDIT: Upated the PR (after rebasing some commits from upstream). Seems like there's a drop in test coverage. I'll add more tests in the coming iterations 👍

neiljp

@Ezio-Sarthak Thanks for continuing to work on this 👍

This looks to work fine, though I do think that having a "message modernization" method when receiving messages will probably be useful in general - for example with 'subject' and 'topic' themselves too! Having an accessor method is generally good over just accessing the data, but the UI already has the Message right now so get_topic_links isn't really an accessor so much as asking the model to simplify the data on-demand.

zulipterminal/ui_tools/views.py

zulipterminal/api_types.py

zulipterminal/model.py

neiljp

@Ezio-Sarthak The modernization pass makes things cleaner 👍

neiljp · 2021-02-22T22:43:01Z

zulipterminal/ui_tools/views.py

@@ -1213,6 +1213,7 @@ def __init__(self, controller: Any, msg: Message, title: str,
                 time_mentions: List[Tuple[str, str]],
                 ) -> None:
        self.msg = msg
+        self.controller = controller


This doesn't seem connected?

neiljp · 2021-02-22T22:44:12Z

zulipterminal/ui_tools/views.py

+        # slice_index = Number of labels before links + 1 newline
+        #               + 1 '<Any> Links' category label.
+        slice_index = len(msg_info[0][1]) + 2


Does this have any application outside of the conditional below?

neiljp · 2021-02-22T23:10:39Z

zulipterminal/model.py

+            # Convert message reponse to the modern format
+            response['messages'] = self.modernize_message_response(
+                response['messages']
+            )


I'm not sure the comment is necessary - it duplicates the function name? :)

Note that we need to call this for all new messages, both on explicit message fetch and for message events.

neiljp · 2021-02-22T23:13:51Z

tests/model/test_model.py

+        [{'subject_links': ['www.foo1.com', 'www.bar1.com']}],
+        [{'topic_links':   ['www.foo1.com', 'www.bar1.com']}]), (
+        [{'topic_links':   ['www.foo2.com', 'www.bar2.com']}],
+        [{'topic_links':   ['www.foo2.com', 'www.bar2.com']}]),
+    ], ids=[
+        'param_old_server_release',
+        'param_new_server_release',


It may be worth adding some empty cases too? The code should already handle it, and the test will then specify that it should.

By empty do you mean list() or None? We don't have a None value response for topic_links from API call anyways. That said I'll add some empty list tests. Thanks for pointing out :)

neiljp · 2021-02-22T23:18:12Z

zulipterminal/ui_tools/boxes.py

@@ -728,6 +729,19 @@ def need_recipient_header(self) -> bool:
        else:
            raise RuntimeError("Invalid message type")

+    def _set_topic_links(self) -> None:


Do you expect to expand upon this in future or call it in more than one place? If not then right now this appears to be a dict comprehension, so I think we can have it inline at the one call site?

neiljp · 2021-02-22T23:21:04Z

zulipterminal/ui_tools/views.py

+            if topic_links:
+                slice_index += (len(topic_links) + 2)


Is this simplified if the slice_index is in the conditional in the earlier commit?

neiljp · 2021-02-22T23:26:46Z

zulipterminal/model.py

+    @staticmethod
+    def modernize_message_response(response: List[Message]) -> List[Message]:
+        """
+        Cleans recieved messages into the modern format


This docstring could be more helpful - why is it useful/helpful?

Also "receive".

Ezio-Sarthak · 2021-02-26T02:13:42Z

@zulipbot add "PR needs review"

Ezio-Sarthak · 2021-02-26T02:15:31Z

@zulipbot remove "PR awaiting update"

`topic_links` param is new in Zulip 3.0 (feature level 1) release. Previously it was called as `subject_links`. Successful `topic_links` response earlier vs now: * earlier: ['www.link1.com'] * now: [{'url': 'www.link1.com', 'text': 'My link']}

This commit: * adds a 'message modernization' helper, that converts old release params and their corresponding responses (in general) to modern messages format. This could be extended to other message responses potentially in future. * hooks the helper to suitable model methods/event listeners. Helper: Model.modernize_message_response() Test added.

This commit: - sets the OrderedDict of topic links in a message. (using the new modernized `topic_links` response) - passes argument topic links in suitable calls. Tests updated.

This commit adds a static method to create link buttons in message information popup. Tests added.

This is a wrapper commit that appends topic links in message info view (if present). If the link text is empty, the link is appended in a single line. Tests amended. Fixes zulip#709.

neiljp · 2021-04-08T06:35:03Z

@Ezio-Sarthak Thanks for this! I just merged with the remaining few changes left over after the last back and forth as discussed 🎉
(mainly removing the need for .keys() on a dict and simplifying some text/comments)

zulipbot added size: XL [Automatic label added by zulipbot] enhancement New feature or request labels Jan 6, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch 4 times, most recently from 9420e4a to e639e97 Compare January 6, 2021 15:56

neiljp added the PR needs review PR requires feedback to proceed label Jan 6, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch 4 times, most recently from 7ef5fb0 to 99dc58c Compare January 16, 2021 16:23

neiljp reviewed Jan 17, 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 Jan 17, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch 4 times, most recently from e1446a8 to f32f1bd Compare January 23, 2021 06:37

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch from f32f1bd to 5762ed7 Compare February 14, 2021 11:19

zulipbot removed the has conflicts label Feb 14, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch from 5762ed7 to a4e9f5f Compare February 14, 2021 16:49

neiljp reviewed Feb 17, 2021

View reviewed changes

Ezio-Sarthak commented Feb 17, 2021

View reviewed changes

zulipterminal/api_types.py Outdated Show resolved Hide resolved

Ezio-Sarthak force-pushed the handle-topic-links branch from a4e9f5f to d0251c4 Compare February 17, 2021 16:32

neiljp reviewed Feb 21, 2021

View reviewed changes

Ezio-Sarthak force-pushed the handle-topic-links branch 2 times, most recently from 374476a to 1a91ba5 Compare February 22, 2021 10:38

neiljp reviewed Feb 22, 2021

View reviewed changes

Ezio-Sarthak force-pushed the handle-topic-links branch from 1a91ba5 to 1bee412 Compare February 23, 2021 18:18

zulipbot added the PR needs review PR requires feedback to proceed label Feb 26, 2021

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 26, 2021

Ezio-Sarthak force-pushed the handle-topic-links branch from 1bee412 to 5446e08 Compare February 26, 2021 03:14

Ezio-Sarthak force-pushed the handle-topic-links branch from 5446e08 to 887cc18 Compare March 8, 2021 05:57

Ezio-Sarthak force-pushed the handle-topic-links branch 2 times, most recently from 7074d1e to 40fa0f2 Compare March 27, 2021 16:09

Ezio-Sarthak force-pushed the handle-topic-links branch 3 times, most recently from 8281feb to 16d13df Compare March 31, 2021 06:53

Ezio-Sarthak mentioned this pull request Apr 3, 2021

TRACKING: Update to use modern API elements (retaining support to 2.1) #965

Open

5 tasks

Ezio-Sarthak force-pushed the handle-topic-links branch 2 times, most recently from a3a1224 to 06014d9 Compare April 8, 2021 03:13

Ezio-Sarthak added 5 commits April 8, 2021 08:58

boxes/core/views: Set and pass topic links linkifier OrderedDict.

c597740

This commit: - sets the OrderedDict of topic links in a message. (using the new modernized `topic_links` response) - passes argument topic links in suitable calls. Tests updated.

refactor: views: Get message links via create_link_buttons().

8b0c634

This commit adds a static method to create link buttons in message information popup. Tests added.

views: Append topic links in message info view.

fb6e5d2

This is a wrapper commit that appends topic links in message info view (if present). If the link text is empty, the link is appended in a single line. Tests amended. Fixes zulip#709.

Ezio-Sarthak force-pushed the handle-topic-links branch from 06014d9 to fb6e5d2 Compare April 8, 2021 03:31

neiljp added this to the Next Release milestone Apr 8, 2021

neiljp closed this Apr 8, 2021

neiljp added missing feature: user A missing feature for all users, present in another Zulip client and removed enhancement New feature or request PR needs review PR requires feedback to proceed labels Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle topic links in message info view #867

Handle topic links in message info view #867

Ezio-Sarthak commented Jan 6, 2021 •

edited

Loading

neiljp left a comment

neiljp Jan 17, 2021

Ezio-Sarthak Jan 19, 2021 •

edited

Loading

neiljp Jan 31, 2021

Ezio-Sarthak Feb 17, 2021 •

edited

Loading

neiljp Jan 17, 2021

neiljp left a comment

neiljp Jan 31, 2021

Ezio-Sarthak Feb 17, 2021 •

edited

Loading

neiljp Jan 31, 2021

Ezio-Sarthak commented Feb 17, 2021 •

edited

Loading

neiljp left a comment

neiljp left a comment

neiljp Feb 22, 2021

neiljp Feb 22, 2021

neiljp Feb 22, 2021

neiljp Feb 22, 2021

Ezio-Sarthak Feb 23, 2021

neiljp Feb 22, 2021

neiljp Feb 22, 2021

neiljp Feb 22, 2021

Ezio-Sarthak commented Feb 26, 2021

Ezio-Sarthak commented Feb 26, 2021

neiljp commented Apr 8, 2021

Handle topic links in message info view #867

Handle topic links in message info view #867

Conversation

Ezio-Sarthak commented Jan 6, 2021 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Feb 17, 2021 • edited Loading

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 Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak commented Feb 17, 2021 • edited Loading

neiljp 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

Ezio-Sarthak commented Feb 26, 2021

Ezio-Sarthak commented Feb 26, 2021

neiljp commented Apr 8, 2021

Ezio-Sarthak commented Jan 6, 2021 •

edited

Loading

Ezio-Sarthak Jan 19, 2021 •

edited

Loading

Ezio-Sarthak Feb 17, 2021 •

edited

Loading

Ezio-Sarthak Feb 17, 2021 •

edited

Loading

Ezio-Sarthak commented Feb 17, 2021 •

edited

Loading