From 6d48472c13e1598bc316cbf07a0c2434b59c55e9 Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 31 Mar 2023 01:41:05 +0530 Subject: [PATCH 1/6] api_types: Add edit_history to Message. --- zulipterminal/api_types.py | 1 + 1 file changed, 1 insertion(+) diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py index 8acf4d05e2..a3383cad77 100644 --- a/zulipterminal/api_types.py +++ b/zulipterminal/api_types.py @@ -182,6 +182,7 @@ class Message(TypedDict, total=False): # NOTE: `subject_links` in Zulip 2.1; deprecated from Zulip 3.0 / ZFL 1 subject_links: List[str] is_me_message: bool + edit_history: List[Dict[str, Any]] reactions: List[Dict[str, Any]] submessages: List[Dict[str, Any]] flags: List[MessageFlag] From b212f6f05a291ffada80ff68f7c215fe8172aad5 Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 31 Mar 2023 01:49:55 +0530 Subject: [PATCH 2/6] helper: Add a function analyse_edit_history. The function analyse_edit_history is called from index _messages of helper.py. It decouples the edit/moved label adding mechanism from index_messages to be used later in _handle_update_messages_event.Stream/content changes comes under "edited" while topic editing checks cases related to resolve/unresolve etc which can go either way of "moved"/"edited". But moved label is applied to specific cases while rest of the cases are "edited" using else conditional(expect the resolve_change case). --- tests/conftest.py | 1 + zulipterminal/helper.py | 69 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index fe771d095c..e81dd87856 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1073,6 +1073,7 @@ def empty_index( stream_msg_ids_by_stream_id=defaultdict(set, {}), topic_msg_ids=defaultdict(dict, {}), edited_messages=set(), + moved_messages=set(), topics=defaultdict(list), search=set(), messages=defaultdict( diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py index 52e407628a..c3569932ad 100644 --- a/zulipterminal/helper.py +++ b/zulipterminal/helper.py @@ -32,7 +32,12 @@ import requests from typing_extensions import Literal, ParamSpec, TypedDict -from zulipterminal.api_types import Composition, EmojiType, Message +from zulipterminal.api_types import ( + RESOLVED_TOPIC_PREFIX, + Composition, + EmojiType, + Message, +) from zulipterminal.config.keys import primary_key_for_command from zulipterminal.config.regexes import ( REGEX_COLOR_3_DIGIT, @@ -110,6 +115,7 @@ class Index(TypedDict): topic_msg_ids: Dict[int, Dict[str, Set[int]]] # Extra cached information edited_messages: Set[int] # {message_id, ...} + moved_messages: Set[int] topics: Dict[int, List[str]] # {topic names, ...} search: Set[int] # {message_id, ...} # Downloaded message data by message id @@ -126,6 +132,7 @@ class Index(TypedDict): stream_msg_ids_by_stream_id=defaultdict(set), topic_msg_ids=defaultdict(dict), edited_messages=set(), + moved_messages=set(), topics=defaultdict(list), search=set(), # mypy bug: https://github.com/python/mypy/issues/7217 @@ -305,6 +312,44 @@ def set_count(id_list: List[int], controller: Any, new_count: int) -> None: controller.update_screen() +def analyse_edit_history( + msg_id: int, + index: Index, + content_changed: bool, + stream_changed: bool, + current_topic: Any = None, + old_topic: Any = None, +) -> None: + resolve_change = False + resolved_prefix = RESOLVED_TOPIC_PREFIX + " " + if content_changed or stream_changed: + index["edited_messages"].add(msg_id) + elif old_topic: + old_topic_resolved = old_topic.startswith(resolved_prefix) + current_topic_resolved = current_topic.startswith(resolved_prefix) + if not current_topic_resolved: + if old_topic_resolved: + if old_topic[2:] != current_topic: + index["moved_messages"].add(msg_id) + if old_topic[2:] == current_topic: + resolve_change = True + if not old_topic_resolved and not current_topic_resolved: + index["moved_messages"].add(msg_id) + else: + if old_topic_resolved and old_topic[2:] != current_topic[2:]: + index["moved_messages"].add(msg_id) + if not old_topic_resolved: + if current_topic[2:] != old_topic: + index["moved_messages"].add(msg_id) + if current_topic[2:] == old_topic: + resolve_change = True + + else: + index["edited_messages"].add(msg_id) + if msg_id not in index["moved_messages"] and not resolve_change: + index["edited_messages"].add(msg_id) + + def index_messages(messages: List[Message], model: Any, index: Index) -> Index: """ STRUCTURE OF INDEX @@ -433,8 +478,26 @@ def index_messages(messages: List[Message], model: Any, index: Index) -> Index: narrow = model.narrow for msg in messages: if "edit_history" in msg: - index["edited_messages"].add(msg["id"]) - + stream_changed = False + content_changed = False + current_topic = None + old_topic = None + for edit_history_event in msg["edit_history"]: + if "prev_content" in edit_history_event: + content_changed = True + if "prev_stream" in edit_history_event: + stream_changed = True + if "prev_topic" in edit_history_event: + current_topic = edit_history_event["topic"] + old_topic = edit_history_event["prev_topic"] + analyse_edit_history( + msg["id"], + index, + content_changed, + stream_changed, + current_topic, + old_topic, + ) index["messages"][msg["id"]] = msg if not narrow: index["all_msg_ids"].add(msg["id"]) From 39cc8cb772f65c1e1b100252dd7d3e466cd0e7fe Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 31 Mar 2023 01:53:17 +0530 Subject: [PATCH 3/6] messages: Add condition for moved label Instead of EDITED label now urwid.Text takes label_text as argument which can be "EDITED" and "MOVED". --- zulipterminal/ui_tools/messages.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/zulipterminal/ui_tools/messages.py b/zulipterminal/ui_tools/messages.py index 47600b4606..0a640e96ca 100644 --- a/zulipterminal/ui_tools/messages.py +++ b/zulipterminal/ui_tools/messages.py @@ -732,14 +732,20 @@ def main_view(self) -> List[Any]: if self.message["id"] in self.model.index["edited_messages"]: edited_label_size = 7 left_padding = 1 + label_text = "EDITED" + elif self.message["id"] in self.model.index["moved_messages"]: + edited_label_size = 6 + left_padding = 2 + label_text = "MOVED" else: edited_label_size = 0 left_padding = 8 + label_text = "EDITED" wrapped_content = urwid.Padding( urwid.Columns( [ - (edited_label_size, urwid.Text("EDITED")), + (edited_label_size, urwid.Text(label_text)), urwid.LineBox( urwid.Columns( [ From 47b451da0d9089bfaa23f8f8378377c0382bd04e Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 31 Mar 2023 01:59:52 +0530 Subject: [PATCH 4/6] model: update _handle_update_messages_event. Uses the analyse_edit_history funciton from helper.py The parameters passed to analyse_edit_history is slightly diffrent from earlier as it gets the required values from event information. Tests adapted. --- tests/model/test_model.py | 17 +++++++++++++---- zulipterminal/model.py | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/tests/model/test_model.py b/tests/model/test_model.py index 915621f8fc..a5088828a0 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -2150,7 +2150,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting( "topic_msg_ids": { 10: {"new subject": {1}, "old subject": {2}}, }, - "edited_messages": {1}, + "edited_messages": set(), + "moved_messages": {1}, "topics": {10: []}, }, False, @@ -2186,7 +2187,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting( "topic_msg_ids": { 10: {"new subject": {1, 2}, "old subject": set()}, }, - "edited_messages": {1}, + "edited_messages": set(), + "moved_messages": {1}, "topics": {10: []}, }, False, @@ -2292,6 +2294,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting( 10: {"new subject": set(), "old subject": {1, 2}}, }, "edited_messages": {1}, + "moved_messages": set(), "topics": {10: ["new subject", "old subject"]}, }, False, @@ -2329,7 +2332,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting( "topic_msg_ids": { 10: {"new subject": {1}, "old subject": {2}}, }, - "edited_messages": {1}, + "edited_messages": set(), + "moved_messages": {1}, "topics": {10: []}, }, False, @@ -2363,6 +2367,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting( 10: {"new subject": set(), "old subject": {1, 2}}, }, "edited_messages": {1}, + "moved_messages": set(), "topics": {10: ["new subject", "old subject"]}, }, False, @@ -2401,6 +2406,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting( 10: {"new subject": {3}, "old subject": {1, 2}}, }, "edited_messages": set(), + "moved_messages": set(), "topics": {10: []}, # This resets the cache }, False, @@ -2439,6 +2445,7 @@ def test_notify_users_hides_PM_content_based_on_user_setting( 10: {"new subject": {3}, "old subject": {1, 2}}, }, "edited_messages": set(), + "moved_messages": set(), "topics": {10: ["new subject", "old subject"]}, }, True, @@ -2476,7 +2483,8 @@ def test_notify_users_hides_PM_content_based_on_user_setting( "topic_msg_ids": { 10: {"new subject": {1}, "old subject": {2}}, }, - "edited_messages": {1}, + "edited_messages": set(), + "moved_messages": {1}, "topics": {10: ["new subject", "old subject"]}, }, True, @@ -2512,6 +2520,7 @@ def test__handle_update_message_event( 10: {"new subject": set(), "old subject": {1, 2}}, }, "edited_messages": set(), + "moved_messages": set(), "topics": {10: ["new subject", "old subject"]}, } mocker.patch(MODEL + "._update_rendered_view") diff --git a/zulipterminal/model.py b/zulipterminal/model.py index 9c6fce61b5..2f2228b9f1 100644 --- a/zulipterminal/model.py +++ b/zulipterminal/model.py @@ -64,6 +64,7 @@ StreamData, TidiedUserInfo, UserStatus, + analyse_edit_history, asynch, canonicalize_color, classify_unread_counts, @@ -1709,9 +1710,29 @@ def _handle_update_message_event(self, event: Event) -> None: # they are not all marked as edited, as per server optimization message_id = event["message_id"] indexed_message = self.index["messages"].get(message_id, None) - + current_topic = None + old_topic = None if indexed_message: - self.index["edited_messages"].add(message_id) + if "prev_content" in event: + content_changed = True + else: + content_changed = False + if "prev_stream" in event: + stream_changed = True + else: + stream_changed = False + if "subject" in event: + current_topic = event["subject"] + old_topic = event["orig_subject"] + + analyse_edit_history( + message_id, + self.index, + content_changed, + stream_changed, + current_topic, + old_topic, + ) # Update the rendered content, if the message is indexed if "rendered_content" in event and indexed_message: From 1a940dcac47c010653341a8e1eb054aa599d3d25 Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 14 Apr 2023 06:41:48 +0000 Subject: [PATCH 5/6] test_helper: Added tests for analyse_edit_history. It tests the possible scenarios for the function as expressed in the test ids. --- tests/helper/test_helper.py | 128 ++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py index 8a79359ea4..255c88dae5 100644 --- a/tests/helper/test_helper.py +++ b/tests/helper/test_helper.py @@ -8,6 +8,7 @@ from zulipterminal.config.keys import primary_key_for_command from zulipterminal.helper import ( Index, + analyse_edit_history, canonicalize_color, classify_unread_counts, display_error_if_present, @@ -112,6 +113,133 @@ def test_index_messages_narrow_user_multiple( assert index_messages(messages, model, model.index) == index_user_multiple +@pytest.mark.parametrize( + ("content_changed, stream_changed, old_topic, current_topic, expected_result"), + [ + case( + False, + True, + None, + None, + {"edited_messages": {12345}, "moved_messages": set()}, + id="only_stream_changed", + ), + case( + True, + False, + None, + None, + {"edited_messages": {12345}, "moved_messages": set()}, + id="only_content_changed", + ), + case( + True, + True, + "Topic B", + "Topic C", + {"edited_messages": {12345}, "moved_messages": set()}, + id="both_stream_and_content_changed", + ), + case( + True, + False, + "Topic B", + "Topic C", + {"edited_messages": {12345}, "moved_messages": set()}, + id="both_content_and_topic_changed", + ), + case( + False, + False, + "Topic D", + "Topic E", + {"edited_messages": set(), "moved_messages": {12345}}, + id="actual_topic_change", + ), + case( + False, + True, + "Topic F", + "Topic G", + {"edited_messages": {12345}, "moved_messages": set()}, + id="topic_and_stream_changed", + ), + case( + False, + False, + "✔ Topic I", + "✔ Topic J", + {"edited_messages": set(), "moved_messages": {12345}}, + id="topic_changed_from_resolved_to resolved_but_different_topic", + ), + case( + False, + False, + "Topic K", + "Topic L", + {"edited_messages": set(), "moved_messages": {12345}}, + id="topic_changed_when_both_are_unresolved", + ), + case( + False, + False, + "✔ Topic M", + "Topic M", + {"edited_messages": set(), "moved_messages": set()}, + id="only_resolved_topic_to_unresolved_topic", + ), + case( + False, + False, + "Topic M", + "✔ Topic M", + {"edited_messages": set(), "moved_messages": set()}, + id="only_unresolved_topic_to_resolved_topic", + ), + case( + False, + False, + "✔ Topic N", + "Topic M", + {"edited_messages": set(), "moved_messages": {12345}}, + id="resolved_topic_to_changed_unresolved_topic", + ), + case( + False, + False, + "Topic M", + "✔ Topic N", + {"edited_messages": set(), "moved_messages": {12345}}, + id="unresolved_topic_to_changed_resolved_topic", + ), + ], +) +def test_analyse_edit_history( + content_changed: bool, + stream_changed: bool, + current_topic: str, + old_topic: str, + expected_result: Dict[str, Any], + initial_index: Index, +) -> None: + msg_id = 12345 + expected_index = dict( + initial_index, + edited_messages=expected_result["edited_messages"], + moved_messages=expected_result["moved_messages"], + ) + analyse_edit_history( + msg_id, + initial_index, + content_changed, + stream_changed, + current_topic, + old_topic, + ) + + assert initial_index == expected_index + + @pytest.mark.parametrize( "edited_msgs", [ From 9c54b8e227b4e6424702419b166edb06177f2a3c Mon Sep 17 00:00:00 2001 From: Subhasish-Behera Date: Fri, 31 Mar 2023 02:06:28 +0530 Subject: [PATCH 6/6] test_messages: Add and modify test for message labels. Tests the conditions for moved and edited label along wth UI differences. Fixes #1253 --- tests/ui_tools/test_messages.py | 44 +++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/tests/ui_tools/test_messages.py b/tests/ui_tools/test_messages.py index c87cc07203..cbf4381f7b 100644 --- a/tests/ui_tools/test_messages.py +++ b/tests/ui_tools/test_messages.py @@ -1135,18 +1135,52 @@ def test_main_view_compact_output( assert len(view_components) == 1 assert isinstance(view_components[0], Padding) - def test_main_view_generates_EDITED_label( - self, mocker, messages_successful_response + @pytest.mark.parametrize( + [ + "message_edited", + "message_moved", + "expected_left_padding", + "expected_label_text", + ], + [ + case(True, False, 7, "EDITED", id="message_indexed_to_edited_messages"), + case(False, True, 6, "MOVED", id="message_indexed_to_moved_messages"), + case( + False, + False, + 0, + "EDITED", + id="message_neither_index_to_edited_messages_or_moved_messages", + ), + ], + ) + def test_main_view_generates_EDITED_or_MOVED_label( + self, + mocker, + messages_successful_response, + message_edited, + message_moved, + expected_left_padding, + expected_label_text, ): messages = messages_successful_response["messages"] for message in messages: - self.model.index["edited_messages"].add(message["id"]) + msg_id = message["id"] + message_index = { + "edited_messages": {msg_id} if message_edited else set(), + "moved_messages": {msg_id} if message_moved else set(), + } + self.model.index = dict( + self.model.index, + edited_messages=message_index["edited_messages"], + moved_messages=message_index["moved_messages"], + ) msg_box = MessageBox(message, self.model, message) view_components = msg_box.main_view() label = view_components[0].original_widget.contents[0] - assert label[0].text == "EDITED" - assert label[1][1] == 7 + assert label[0].text == expected_label_text + assert label[1][1] == expected_left_padding @pytest.mark.parametrize( "to_vary_in_last_message, update_required",