Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Unify case sensitive topic names #733

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
7 changes: 4 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,8 @@ def index_topic(empty_index):
Expected index of initial_data when model.narrow = [['stream', '7'],
['topic', 'Test']]
"""
diff = {'topic_msg_ids': defaultdict(dict, {205: {'Test': {537286}}})}
# NOTE: Use canonicalized topic name for indexing.
diff = {'topic_msg_ids': defaultdict(dict, {205: {'test': {537286}}})}
return dict(empty_index, **diff)


Expand Down Expand Up @@ -799,8 +800,8 @@ def classified_unread_counts():
'all_msg': 12,
'all_pms': 8,
'unread_topics': {
(1000, 'Some general unread topic'): 3,
(99, 'Some private unread topic'): 1
(1000, 'some general unread topic'): 3,
(99, 'some private unread topic'): 1
},
'unread_pms': {
1: 2,
Expand Down
3 changes: 2 additions & 1 deletion tests/core/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ def test_narrow_to_topic(self, mocker, controller,
widget = (controller.view.message_view.log
.extend.call_args_list[0][0][0][0])
stream_id, topic_name = msg_box.stream_id, msg_box.topic_name
id_list = index_topic['topic_msg_ids'][stream_id][topic_name]
# NOTE: Use canonicalized topic name for lookup.
id_list = index_topic['topic_msg_ids'][stream_id][topic_name.lower()]
assert {widget.original_widget.message['id']} == id_list

def test_narrow_to_user(self, mocker, controller, user_button, index_user):
Expand Down
10 changes: 5 additions & 5 deletions tests/helper/test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,16 +189,16 @@ def test_powerset(iterable, map_func, expected_powerset):


@pytest.mark.parametrize('muted_streams, muted_topics, vary_in_unreads', [
([99], [['Some general stream', 'Some general unread topic']], {
([99], [['Some general stream', 'some general unread topic']], {
'all_msg': 8,
'streams': {99: 1},
'unread_topics': {(99, 'Some private unread topic'): 1},
'unread_topics': {(99, 'some private unread topic'): 1},
'all_mentions': 0,
}),
([1000], [['Secret stream', 'Some private unread topic']], {
([1000], [['Secret stream', 'some private unread topic']], {
'all_msg': 8,
'streams': {1000: 3},
'unread_topics': {(1000, 'Some general unread topic'): 3},
'unread_topics': {(1000, 'some general unread topic'): 3},
'all_mentions': 0,
}),
([1], [], {'all_mentions': 0})
Expand All @@ -214,7 +214,7 @@ def test_classify_unread_counts(mocker, initial_data, stream_dict,
model.initial_data = initial_data
model.is_muted_topic = mocker.Mock(side_effect=(
lambda stream_id, topic:
[model.stream_dict[stream_id]['name'], topic] in muted_topics
[model.stream_dict[stream_id]['name'], topic.lower()] in muted_topics
))
model.muted_streams = muted_streams
assert classify_unread_counts(model) == dict(classified_unread_counts,
Expand Down
6 changes: 4 additions & 2 deletions tests/model/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,15 +286,17 @@ def test_set_narrow_not_already_set(self, model, initial_narrow, narrow,
['topic', 'BOO']], {
'topic_msg_ids': {
1: {
'BOO': {0, 1}
# NOTE: Use canonicalized topic name for indexing.
'boo': {0, 1}
}
}
}, {0, 1}),
([['stream', 'FOO'], # Covers one empty-set case
['topic', 'BOOBOO']], {
'topic_msg_ids': {
1: {
'BOO': {0, 1}
# NOTE: Use canonicalized topic name for indexing.
'boo': {0, 1}
}
}
}, set()),
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/test_ui_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1144,8 +1144,8 @@ def mock_external_classes(self, mocker):
1000: 1,
},
'unread_topics': {
(205, 'TOPIC1'): 34,
(205, 'TOPIC2'): 100,
(205, 'topic1'): 34,
(205, 'topic2'): 100,
},
'all_mentions': 1,
}
Expand Down Expand Up @@ -1200,7 +1200,7 @@ def test_topics_view(self, mocker, stream_button, width=40):
topic_button = mocker.patch(VIEWS + '.TopicButton')
topics_view = mocker.patch(VIEWS + '.TopicsView')
line_box = mocker.patch(VIEWS + '.urwid.LineBox')
topic_list = ['TOPIC1', 'TOPIC2', 'TOPIC3']
topic_list = ['topic1', 'topic2', 'topic3']
unread_count_list = [34, 100, 0]
self.view.model.topics_in_stream = (
mocker.Mock(return_value=topic_list)
Expand Down
47 changes: 36 additions & 11 deletions zulipterminal/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
'all_msg': int,
'all_pms': int,
'all_mentions': int,
'unread_topics': Dict[Tuple[int, str], int], # stream_id, topic
'unread_topics': Dict[Tuple[int, str], int], # stream_id, canonical topic
'unread_pms': Dict[int, int], # sender_id
'unread_huddles': Dict[FrozenSet[int], int], # Group pms
'streams': Dict[int, int], # stream_id
Expand Down Expand Up @@ -139,8 +139,10 @@ def update_unreads(unreads: Dict[KeyT, int], key: KeyT) -> None:
for message in changed_messages:
if message['type'] == 'stream':
stream_id = message['stream_id']
# Use lowercase topic names in unread_topics to backup the
# invariant in index.
update_unreads(unread_counts['unread_topics'],
(stream_id, message['subject']))
(stream_id, canonicalize_topic(message['subject'])))
update_unreads(unread_counts['streams'], stream_id)
# self-pm has only one display_recipient
# 1-1 pms have 2 display_recipient
Expand Down Expand Up @@ -201,7 +203,7 @@ def _set_count_in_view(controller: Any, new_count: int,
# If topic_view is open for incoming messages's stream,
# We update the respective TopicButton count accordingly.
for topic_button in topic_buttons_log:
if topic_button.topic_name == msg_topic:
if compare_lowercase(topic_button.topic_name, msg_topic):
topic_button.update_count(topic_button.count
+ new_count)
else:
Expand Down Expand Up @@ -245,11 +247,13 @@ def index_messages(messages: List[Message],
{
'pointer': {
'[]': 30 # str(ZulipModel.narrow)
# NOTE: canonicalize_topic() is used for indexing.
'[["stream", "verona"]]': 32,
...
}
'topic_msg_ids': {
123: { # stream_id
# NOTE: canonicalize_topic() is used for indexing.
'topic name': {
51234, # message id
56454,
Expand Down Expand Up @@ -409,12 +413,19 @@ def index_messages(messages: List[Message],
(index['stream_msg_ids_by_stream_id'][msg['stream_id']]
.add(msg['id']))

if (msg['type'] == 'stream' and len(narrow) == 2
and narrow[1][1] == msg['subject']):
topics_in_stream = index['topic_msg_ids'][msg['stream_id']]
if not topics_in_stream.get(msg['subject']):
topics_in_stream[msg['subject']] = set()
topics_in_stream[msg['subject']].add(msg['id'])
if msg['type'] == 'stream' and len(narrow) == 2:
narrow_topic = narrow[1][1]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

narrow_topic vs narrow_topics?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The narrow can only have one topic, right?

message_topic = msg['subject']
# The comparison is in lowercase to avoid missing other case
# sensitive topic variants. For instance, case, CASE, cAsE.
if compare_lowercase(narrow_topic, message_topic):
topics_in_stream = index['topic_msg_ids'][msg['stream_id']]
preetmishra marked this conversation as resolved.
Show resolved Hide resolved
# To have an invariant, use canonicalize_topic() to index IDs
# in topic_msg_ids.
message_topic = canonicalize_topic(message_topic)
if not topics_in_stream.get(message_topic):
topics_in_stream[message_topic] = set()
topics_in_stream[message_topic].add(msg['id'])

return index

Expand Down Expand Up @@ -450,8 +461,10 @@ def classify_unread_counts(model: Any) -> UnreadCounts:
continue
if model.is_muted_topic(stream_id, stream['topic']):
continue
stream_topic = (stream_id, stream['topic'])
unread_counts['unread_topics'][stream_topic] = count
stream_topic = (stream_id, canonicalize_topic(stream['topic']))
unread_counts['unread_topics'][stream_topic] = (
count + unread_counts['unread_topics'].get(stream_topic, 0)
)
if not unread_counts['streams'].get(stream_id):
unread_counts['streams'][stream_id] = count
else:
Expand Down Expand Up @@ -629,3 +642,15 @@ def display_error_if_present(response: Dict[str, Any], controller: Any
) -> None:
if response['result'] == 'error' and hasattr(controller, 'view'):
controller.view.set_footer_text(response['msg'], 3)


def canonicalize_topic(topic_name: str) -> str:
"""
Returns a canonicalized topic name to be used for indexing and lookups
locally.
"""
return topic_name.lower()


def compare_lowercase(a: str, b: str) -> bool:
return a.lower() == b.lower()
52 changes: 38 additions & 14 deletions zulipterminal/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import time
from collections import OrderedDict, defaultdict
from concurrent.futures import Future, ThreadPoolExecutor, wait
from copy import deepcopy
from typing import (
Any, Callable, DefaultDict, Dict, FrozenSet, Iterable, List, Optional, Set,
Tuple, Union,
Expand All @@ -13,8 +14,9 @@

from zulipterminal.config.keys import keys_for_command
from zulipterminal.helper import (
Message, StreamData, asynch, canonicalize_color, classify_unread_counts,
display_error_if_present, index_messages, initial_index, notify, set_count,
Message, StreamData, asynch, canonicalize_color, canonicalize_topic,
classify_unread_counts, compare_lowercase, display_error_if_present,
index_messages, initial_index, notify, set_count,
)
from zulipterminal.ui_tools.utils import create_msg_box_list

Expand Down Expand Up @@ -124,8 +126,9 @@ def __init__(self, controller: Any) -> None:
muted_topics = self.initial_data['muted_topics']
assert set(map(len, muted_topics)) in (set(), {2}, {3})
self._muted_topics = {
(stream_name, topic): (None if self.server_feature_level is None
else date_muted[0])
(stream_name, canonicalize_topic(topic)): (
None if self.server_feature_level is None else date_muted[0]
)
for stream_name, topic, *date_muted in muted_topics
} # type: Dict[Tuple[str, str], Optional[int]]

Expand All @@ -138,15 +141,26 @@ def __init__(self, controller: Any) -> None:
self.new_user_input = True
self._start_presence_updates()

def narrow_with_canonical_topic(self) -> List[Any]:
"""
Returns the narrow with its topic name replaced with the invariant
version that we maintain locally.
"""
narrow = deepcopy(self.narrow)
if len(narrow) == 2 and narrow[1][0] == 'topic':
narrow[1][1] = canonicalize_topic(narrow[1][1])
return narrow

def get_focus_in_current_narrow(self) -> Union[int, Set[None]]:
"""
Returns the focus in the current narrow.
For no existing focus this returns {}, otherwise the message ID.
"""
return self.index['pointer'][repr(self.narrow)]
return self.index['pointer'][repr(self.narrow_with_canonical_topic())]

def set_focus_in_current_narrow(self, focus_message: int) -> None:
self.index['pointer'][repr(self.narrow)] = focus_message
narrow_str = repr(self.narrow_with_canonical_topic())
self.index['pointer'][narrow_str] = focus_message

def is_search_narrow(self) -> bool:
"""
Expand Down Expand Up @@ -219,7 +233,7 @@ def get_message_ids_in_current_narrow(self) -> Set[int]:
if len(narrow) == 1:
ids = index['stream_msg_ids_by_stream_id'][stream_id]
elif len(narrow) == 2:
topic = narrow[1][1]
topic = canonicalize_topic(narrow[1][1])
ids = index['topic_msg_ids'][stream_id].get(topic, set())
elif narrow[0][1] == 'private':
ids = index['private_msg_ids']
Expand Down Expand Up @@ -372,7 +386,7 @@ def get_messages(self, *,
response = self.client.get_messages(message_filters=request)
if response['result'] == 'success':
self.index = index_messages(response['messages'], self, self.index)
narrow_str = repr(self.narrow)
narrow_str = repr(self.narrow_with_canonical_topic())
if first_anchor and response['anchor'] != 10000000000000000:
self.index['pointer'][narrow_str] = response['anchor']
if 'found_newest' in response:
Expand All @@ -396,6 +410,11 @@ def _fetch_topics_in_streams(self, stream_list: Iterable[int]) -> str:
for stream_id in stream_list:
response = self.client.get_stream_topics(stream_id)
if response['result'] == 'success':
# NOTE: The server only sends the latest topic name version for
# case sensitive topic names.
# See (https://github.com/zulip/zulip/blob
# /2e5f860d41bb441597f7f088a477d5946cab4b13/zerver/lib
# /topic.py#L144).
self.index['topics'][stream_id] = [topic['name'] for
topic in response['topics']]
else:
Expand Down Expand Up @@ -427,7 +446,7 @@ def is_muted_topic(self, stream_id: int, topic: str) -> bool:
Returns True if topic is muted via muted_topics.
"""
stream_name = self.stream_dict[stream_id]['name']
topic_to_search = (stream_name, topic)
topic_to_search = (stream_name, canonicalize_topic(topic))
return topic_to_search in self._muted_topics.keys()

def _update_initial_data(self) -> None:
Expand Down Expand Up @@ -862,8 +881,9 @@ def _handle_message_event(self, event: Event) -> None:
if 'read' not in message['flags']:
set_count([message['id']], self.controller, 1)

narrow_str = repr(self.narrow_with_canonical_topic())
if (hasattr(self.controller, 'view')
and self._have_last_message[repr(self.narrow)]):
and self._have_last_message[narrow_str]):
msg_log = self.controller.view.message_view.log
if msg_log:
last_message = msg_log[-1].original_widget.message
Expand Down Expand Up @@ -896,7 +916,8 @@ def _handle_message_event(self, event: Event) -> None:
if (append_to_stream
and (len(self.narrow) == 1
or (len(self.narrow) == 2
and self.narrow[1][1] == message['subject']))):
and compare_lowercase(self.narrow[1][1],
message['subject'])))):
msg_log.append(msg_w)

elif (message['type'] == 'private' and len(self.narrow) == 1
Expand All @@ -915,8 +936,10 @@ def _update_topic_index(self, stream_id: int, topic_name: str) -> None:
"""
topic_list = self.topics_in_stream(stream_id)
for topic_iterator, topic in enumerate(topic_list):
if topic == topic_name:
topic_list.insert(0, topic_list.pop(topic_iterator))
if compare_lowercase(topic, topic_name):
topic_list.pop(topic_iterator)
# Use the latest topic name version to update.
topic_list.insert(0, topic_name)
break
else:
# No previous topics with same topic names are found
Expand Down Expand Up @@ -1025,7 +1048,8 @@ def _update_rendered_view(self, msg_id: int) -> None:
# Remove the message if it no longer belongs in the current
# narrow.
if (len(self.narrow) == 2
and msg_box.message['subject'] != self.narrow[1][1]):
and not compare_lowercase(msg_box.message['subject'],
self.narrow[1][1])):
view.message_view.log.remove(msg_w)
# Change narrow if there are no messages left in the
# current narrow.
Expand Down
6 changes: 3 additions & 3 deletions zulipterminal/ui_tools/boxes.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
STREAM_TOPIC_SEPARATOR, TIME_MENTION_MARKER,
)
from zulipterminal.helper import (
Message, format_string, match_emoji, match_group, match_stream,
match_topics, match_user,
Message, compare_lowercase, format_string, match_emoji, match_group,
match_stream, match_topics, match_user,
)
from zulipterminal.ui_tools.tables import render_table
from zulipterminal.urwid_types import urwid_Size
Expand Down Expand Up @@ -432,7 +432,7 @@ def need_recipient_header(self) -> bool:
if self.message['type'] == 'stream':
return not (
last_msg['type'] == 'stream'
and self.topic_name == last_msg['subject']
and compare_lowercase(self.topic_name, last_msg['subject'])
and self.stream_name == last_msg['display_recipient']
)
elif self.message['type'] == 'private':
Expand Down
Loading