Skip to content

Add support for 'starred' typeaheads #794

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.

Sign up for GitHub

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

Merged
neiljp merged 5 commits into from
Sep 20, 2020
Merged

Add support for 'starred' typeaheads #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8938c09
refactor: boxes: Simplify generic_autocomplete reverse lookup.
neiljp Sep 1, 2020
9e8513b
boxes: Support optional '**' prefix in stream autocomplete.
neiljp Sep 1, 2020
275ec11
refactor: boxes: Split mention autocomplete into users & groups.
neiljp Sep 2, 2020
a3be878
boxes: Support optional '**' prefix in silent user mentions.
neiljp Sep 2, 2020
195bc42
boxes: Support extra star formats in non-silent group & user mentions.
neiljp Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions tests/ui_tools/test_boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,26 @@ def test_generic_autocomplete_no_prefix(self, mocker, write_box, text,
write_box.view.set_typeahead_footer.assert_not_called()

@pytest.mark.parametrize('text, state, footer_text', [
# no-text mentions
Copy link
Member

@preetmishra preetmishra Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

[minor] The commit title says that * is added for non-silent mentions for groups & users. However, ** is being added for users as well.

Copy link
Collaborator Author

@neiljp neiljp Sep 16, 2020

Choose a reason for hiding this comment

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

I've clarified this text 👍

('@', 0,
['Human Myself', 'Human 1', 'Human 2',
'Group 1', 'Group 2', 'Group 3', 'Group 4']),
('@*', 0, ['Group 1', 'Group 2', 'Group 3', 'Group 4']),
('@**', 0, ['Human Myself', 'Human 1', 'Human 2']),
# mentions
('@Human', 0, ['Human Myself', 'Human 1', 'Human 2']),
('@**Human', 0, ['Human Myself', 'Human 1', 'Human 2']),
('@_Human', 0, ['Human Myself', 'Human 1', 'Human 2']),
('@_*Human', None, []), # NOTE: Optional single star fails
('@_**Human', 0, ['Human Myself', 'Human 1', 'Human 2']),
('@Human', None, ['Human Myself', 'Human 1', 'Human 2']),
('@NoMatch', None, []),
# streams
('#Stream', 0, ['Stream 1', 'Stream 2', 'Secret stream',
'Some general stream']),
('#*Stream', None, []), # NOTE: Optional single star fails
('#**Stream', 0, ['Stream 1', 'Stream 2', 'Secret stream',
'Some general stream']), # Optional 2-stars
('#Stream', None, ['Stream 1', 'Stream 2', 'Secret stream',
'Some general stream']),
('#NoMatch', None, []),
Expand Down Expand Up @@ -118,6 +130,19 @@ def test_generic_autocomplete_set_footer(self, mocker, write_box,
('@', 6, '@*Group 4*'),
('@', 7, None), # Reached last match
('@', 8, None), # Beyond end
# Expected sequence of autocompletes from '@**' (no groups)
('@**', 0, '@**Human Myself**'),
('@**', 1, '@**Human 1**'),
('@**', 2, '@**Human 2**'),
('@**', 3, None), # Reached last match
('@**', 4, None), # Beyond end
# Expected sequence of autocompletes from '@*' (only groups)
('@*', 0, '@*Group 1*'),
('@*', 1, '@*Group 2*'),
('@*', 2, '@*Group 3*'),
('@*', 3, '@*Group 4*'),
('@*', 4, None), # Reached last match
('@*', 5, None), # Beyond end
# Expected sequence of autocompletes from '@_'
('@_', 0, '@_**Human Myself**'), # NOTE: No silent group mention
('@_', 1, '@_**Human 1**'),
Expand All @@ -143,6 +168,8 @@ def test_generic_autocomplete_set_footer(self, mocker, write_box,
(':@_H', 0, ':@_**Human Myself**'),
('#@_H', 0, '#@_**Human Myself**'),
('@@_H', 0, '@@_**Human Myself**'),
('@@_*H', 0, None), # Optional single star fails
('@@_**H', 0, '@@_**Human Myself**'), # Optional stars
])
def test_generic_autocomplete_mentions(self, write_box, text,
required_typeahead, state):
Expand Down Expand Up @@ -199,6 +226,8 @@ def test_generic_autocomplete_mentions_subscribers(self, write_box, text,
('@_#Stream', 0, '@_#**Stream 1**', []),
(':#Stream', 0, ':#**Stream 1**', []),
('##Stream', 0, '##**Stream 1**', []),
('##*Stream', 0, None, []), # NOTE: Optional single star fails
('##**Stream', 0, '##**Stream 1**', []), # Optional 2-stars
# With 'Secret stream' pinned.
('#Stream', 0, '#**Secret stream**',
['Secret stream', ]), # 2nd-word startswith match (pinned).
Expand Down
87 changes: 63 additions & 24 deletions zulipterminal/ui_tools/boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,30 +164,43 @@ def _stream_box_autocomplete(self, text: str, state: Optional[int]
def generic_autocomplete(self, text: str, state: Optional[int]
) -> Optional[str]:
autocomplete_map = OrderedDict([
('@_', self.autocomplete_mentions),
('@_', self.autocomplete_users),
('@_**', self.autocomplete_users),
('@', self.autocomplete_mentions),
('@*', self.autocomplete_groups),
('@**', self.autocomplete_users),
('#', self.autocomplete_streams),
('#**', self.autocomplete_streams),
(':', self.autocomplete_emojis),
])

# Look in a reverse order to find the last autocomplete prefix used in
# the text. For instance, if text='@#example', use '#' as the prefix.
Copy link
Member

@kaustubh-nair kaustubh-nair Sep 2, 2020

Choose a reason for hiding this comment

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

Not related to this commit, but I think I should point out that the server does allow names to start with #.

Copy link
Collaborator Author

@neiljp neiljp Sep 2, 2020

Choose a reason for hiding this comment

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

This is a bug I suppose, but I'm not sure what we can do unless we disable # autocomplete entirely? Even the new @**#example wouldn't find this in this PR, as # is later. We could maybe prioritize the length over the position, but that could be problematic for other reasons. We could explore this with other test cases.

Copy link
Collaborator Author

@neiljp neiljp Sep 16, 2020

Choose a reason for hiding this comment

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

@kaustubh-nair I added a FIXME to the code here for now.

One option I just thought of might be to allow multiple different autocompletes - maybe #@market<ctrl f> could give the option for #@**marketing** but also #**@markets**, if both actually matched?

reversed_text = text[::-1]
for reverse_index, char in enumerate(reversed_text):
# Patch for silent mentions.
if (char == '_' and reverse_index + 1 < len(reversed_text)
and reversed_text[reverse_index + 1] == '@'):
char = '@_'

if char in autocomplete_map:
prefix = char
autocomplete_func = autocomplete_map[prefix]
prefix_index = max(text.rfind(prefix), 0)
break
else:
# Return text if it doesn't have any of the autocomplete prefixes.
# FIXME: Mentions can actually start with '#', and streams with
# anything; this implementation simply chooses the right-most
# match of the longest length
prefix_indices = {
prefix: text.rfind(prefix)
for prefix in autocomplete_map
}
found_prefix_indices = {
prefix: index
for prefix, index in prefix_indices.items()
if index > -1
}
# Return text if it doesn't have any of the autocomplete prefixes.
if not found_prefix_indices:
return text

# Use latest longest matching prefix (so @_ wins vs @)
prefix_index = max(found_prefix_indices.values())
prefix = max(
(len(prefix), prefix)
for prefix, index in found_prefix_indices.items()
if index == prefix_index
)[1]
autocomplete_func = autocomplete_map[prefix]

# NOTE: The following block only executes if any of the autocomplete
# prefixes exist.
typeaheads, suggestions = (
Expand Down Expand Up @@ -222,11 +235,21 @@ def autocomplete_mentions(self, text: str, prefix_string: str
) -> Tuple[List[str], List[str]]:
# Handles user mentions (@ mentions and silent mentions)
# and group mentions.
groups = [group_name
for group_name in self.model.user_group_names
if match_group(group_name, text[1:])]
group_typeahead = format_string(groups, '@*{}*')

user_typeahead, user_names = self.autocomplete_users(
text, prefix_string
)
group_typeahead, groups = self.autocomplete_groups(
text, prefix_string
)

combined_typeahead = user_typeahead + group_typeahead
combined_names = user_names + groups

return combined_typeahead, combined_names

def autocomplete_users(self, text: str, prefix_string: str
) -> Tuple[List[str], List[str]]:
users_list = self.view.users
matching_users = [user
for user in users_list
Expand All @@ -241,12 +264,26 @@ def autocomplete_mentions(self, text: str, prefix_string: str
reverse=True)

user_names = [user['full_name'] for user in sorted_matching_users]
user_typeahead = format_string(user_names, prefix_string + '**{}**')
extra_prefix = "{}{}".format(
'*' if prefix_string[-1] != '*' else '',
'*' if prefix_string[-2:] != '**' else '',
)
user_typeahead = format_string(user_names,
prefix_string + extra_prefix + '{}**')

combined_typeahead = user_typeahead + group_typeahead
combined_names = user_names + groups
return user_typeahead, user_names

return combined_typeahead, combined_names
def autocomplete_groups(self, text: str, prefix_string: str
) -> Tuple[List[str], List[str]]:
prefix_length = len(prefix_string)
groups = [group_name
for group_name in self.model.user_group_names
if match_group(group_name, text[prefix_length:])]

extra_prefix = '*' if prefix_string[-1] != '*' else ''
group_typeahead = format_string(groups,
prefix_string + extra_prefix + '{}*')
return group_typeahead, groups

def autocomplete_streams(self, text: str, prefix_string: str
) -> Tuple[List[str], List[str]]:
Expand All @@ -256,7 +293,9 @@ def autocomplete_streams(self, text: str, prefix_string: str
stream_typeahead = format_string(streams, '#**{}**')
stream_data = list(zip(stream_typeahead, streams))

matched_data = match_stream(stream_data, text[1:],
prefix_length = len(prefix_string)

matched_data = match_stream(stream_data, text[prefix_length:],
self.view.pinned_streams)
return matched_data

Expand Down