Add support for stream + topic autocomplete #1004

Ezio-Sarthak · 2021-04-21T14:48:22Z

The PR does the following tasks:

Fixes a bug re space-separated typeaheads (commit 1),
Extracts all regexes to a new file regexes.py (commit 2-5)
Adds support for stream+topic autocompletes. (commit 6-9).

Ticks a check box in #448.

mkp6781 · 2021-04-21T18:43:16Z

zulipterminal/ui_tools/boxes.py

+        else:
+            topic_names = []
+
+        stream_prefix = f"#**{stream}>"


Is there a reason you aren't using prefix_string directly here. On doing a quick check, I don't think we need this new variable.

Thanks. I updated the PR to get the required prefix using prefix_string

mkp6781 · 2021-04-21T18:44:28Z

@Ezio-Sarthak Thanks for taking this up. I tested this locally and it seems to work fine 👍🏼.

Ezio-Sarthak · 2021-04-27T07:19:14Z

@zulipbot add "feedback wanted"

prah23

Thanks for working on this @Ezio-Sarthak!
This work and the extension to the autocomplete looks good to me and functions well too 👍! I've left a couple of minor suggestions in-line.

prah23 · 2021-07-17T21:53:39Z

zulipterminal/ui_tools/boxes.py

+        m = re.search(REGEX_AUTOCOMPLETE_STREAM_TOPIC, text)
+        m_fenced = re.search(REGEX_AUTOCOMPLETE_STREAM_TOPIC_FENCED, text)


minor: You could use more descriptive aliases, maybe match and fenced_match? (just to improve readability)

tests/ui_tools/test_boxes.py

neiljp

@Ezio-Sarthak This looks really useful: it works and that first commit looks like it does fix that issue without an obvious drawback?

tests/ui_tools/test_boxes.py

neiljp · 2021-07-24T20:05:43Z

zulipterminal/config/regexes.py

+# (*) Example text: #**stream name>Topic name
+REGEX_AUTOCOMPLETE_STREAM_TOPIC = r"#\*\*([^*>]+)>([^*]*)$"


I would add this to the next commit, but use this commit as a refactor of at least some other regexes, particularly those where the content of the regex is not as critical to understand in the subsequent code.

This constant is not different from in @prah23's feedback?

Ah yes. Adding small understandable regexes and integrating them would be more apt. 👍

Re your latter point, I think Hari was referring to the local variable renaming, particularly, m_fenced to match_fenced that are used in the next commit.

tests/ui_tools/test_boxes.py

neiljp · 2021-07-24T20:24:37Z

tests/ui_tools/test_boxes.py

+            # Fenced prefix
+            ("#**Stream 1**>T", 0, "#**Stream 1>Topic 1**", True),


For reference, the web app also supports #**Stream 1** >, presumably for the stream complete generating a space after it?

I think this could be an extra feature on top, depending how straightforward it is, since we don't add a space.

Yeah. I just quickly tested some autocompletes in the web app. All of them added a space in the end. For us, that might be a future normal, I think :)

neiljp

@Ezio-Sarthak The refactor makes this cleaner and close to merging 👍
As per inline, we don't need to add the regexes until we use them though.

We discussed the #streamname> autocomplete being OK on the stream; could you file an issue or start a discussion regarding the space-after-completion point? Maybe there's discussion re the web app that's relevant?

neiljp · 2021-07-26T07:40:06Z

zulipterminal/config/regexes.py

+# (*) Stream and topic regexes
+REGEX_STREAM_NAME = r"([^*>]+)"
+REGEX_TOPIC_NAME = r"([^*]*)"
+
+
+# (*) Example: #**zulip-terminal
+REGEX_STREAM_FENCED_HALF = r"#\*\*{stream}".format(stream=REGEX_STREAM_NAME)
+# (*) Example: #**zulip-terminal**
+REGEX_STREAM_FENCED = r"#\*\*{stream}\*\*".format(stream=REGEX_STREAM_NAME)


These are relevant only in the next commit(s) where they're first used?

Some of these still seem to be present?

zulipterminal/ui_tools/buttons.py

This small commit explicitly sets delimiter for msg_write_box, in order to support autocompletion of text with whitespaces, for both private and stream write boxes. Note that the delims specifed are default ones, except that whitespace is ignored, so as to make it function properly in autocompletes. The default completer delims for message compose are stored in the constant `DELIMS_MESSAGE_COMPOSE`. The commit also fixes a bug in which a user isn't able to use autocompletes for space-containing texts. Tests amended (comments) and added. The existing tests for space-containing prefixes are justified with this commit. Fixes zulip#925 as a side effect.

This commit adds a method that would process and return typeaheads for stream+topic prefixes. The current approach uses regex for identifying the last occurence of required format. The dedicated autocomplete function is: `autocomplete_stream_and_topic`. Note that the regex used for accessing stream and topic data is robust to the function, as the prefix would be manipulated to match this regex type, and then a call to this function would be made. See the next commit for more clarification. The stream and topic names regexes are inspired from webapp's static/js/compose_typeahead.js

This is the first implementation of stream+topic autocomplete in the message content box. This commit adds a validation helper that does the following things: * Checks if a given prefix string is a valid candidate for stream+topic autocomplete. * If the prefix validation is successful, the metadata, i.e, autocomplete map, prefix indices (to include the helper added in previous commit) and the prefix text are updated. Tests added for `autocomplete_stream_and_topic`, since it's the first commit where this function is invoked.

This commit extends support for stream+topic autocomplete from the previous commit to include the stream fences case: `#**stream name**>Topic name`. ^^ The current approach uses a regex for identifying the last occurrence of required format containing stream fences. The dedicated autocomplete function remains the same: `autocomplete_stream_and_topic`. Tests added.

This commit adds an enhancement to show typeaheads for stream+topic prefix string that don't have stream fences, e.g., #stream>topic. This would allow a user to avoid adding fences and just filling stream and/or topic name right away. Tests added. Ticks a checkbox in zulip#448.

neiljp · 2021-07-31T01:20:56Z

@Ezio-Sarthak Thanks for getting this done and making the updates to this 👍 I've shifted the stream/topic regexes to the first commit they're used to hopefully make it clearer, made a few commit text adjustments and am merging now 🎉

zulipbot added the size: M [Automatic label added by zulipbot] label Apr 21, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch 2 times, most recently from 5352028 to 1164b75 Compare April 21, 2021 15:15

mkp6781 reviewed Apr 21, 2021

View reviewed changes

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 1164b75 to 2c0a2f2 Compare April 22, 2021 06:26

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Apr 22, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch 3 times, most recently from f729812 to 935ffdd Compare April 27, 2021 07:15

zulipbot added the feedback wanted label Apr 27, 2021

neiljp added the area: autocomplete label Apr 28, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 935ffdd to 3e8fe4e Compare April 29, 2021 10:35

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 3e8fe4e to 9fb9df2 Compare July 17, 2021 11:57

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jul 17, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 9fb9df2 to 67ebf26 Compare July 17, 2021 12:17

Ezio-Sarthak changed the title ~~[WIP]: Add support for stream + topic autocomplete~~ Add support for stream + topic autocomplete Jul 17, 2021

prah23 suggested changes Jul 17, 2021

View reviewed changes

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 67ebf26 to a9e8d18 Compare July 21, 2021 03:08

neiljp reviewed Jul 24, 2021

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jul 24, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch 2 times, most recently from 191e664 to 449066e Compare July 26, 2021 04:58

neiljp reviewed Jul 26, 2021

View reviewed changes

Ezio-Sarthak force-pushed the stream-topic-typeahead branch from 449066e to 201a10a Compare July 26, 2021 18:59

Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 27, 2021

Ezio-Sarthak force-pushed the stream-topic-typeahead branch 2 times, most recently from c099dd1 to 980e94b Compare July 30, 2021 15:56

Ezio-Sarthak added 6 commits July 30, 2021 15:49

refactor: helper/regexes: Extract helper regexes into new file.

a4b6971

refactor: boxes/regexes: Centralize and use PM recipient regexes.

b64b8cf

refactor: buttons/regexes: Centralize and use encoded stream ID regex.

f48f25a

refactor: regexes/test_themes: Centralize and use valid color regex.

fda0ef0

neiljp force-pushed the stream-topic-typeahead branch from 980e94b to 0232a26 Compare July 31, 2021 00:18

Ezio-Sarthak added 3 commits July 30, 2021 17:34

neiljp force-pushed the stream-topic-typeahead branch from 0232a26 to e4a0e59 Compare July 31, 2021 00:42

neiljp merged commit dcd6b6e into zulip:main Jul 31, 2021

neiljp removed PR needs review PR requires feedback to proceed feedback wanted labels Jul 31, 2021

neiljp added this to the Next Release milestone Jul 31, 2021

neiljp mentioned this pull request Jul 31, 2021

TRACKING: Typeahead improvements #448

Closed

12 tasks

neiljp added bug Something isn't working missing feature: user A missing feature for all users, present in another Zulip client labels Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for stream + topic autocomplete #1004

Add support for stream + topic autocomplete #1004

Ezio-Sarthak commented Apr 21, 2021 •

edited

Loading

mkp6781 Apr 21, 2021

Ezio-Sarthak Apr 22, 2021

mkp6781 commented Apr 21, 2021

Ezio-Sarthak commented Apr 27, 2021

prah23 left a comment

prah23 Jul 17, 2021

neiljp left a comment

neiljp Jul 24, 2021

Ezio-Sarthak Jul 25, 2021

neiljp Jul 24, 2021

Ezio-Sarthak Jul 25, 2021

neiljp left a comment

neiljp Jul 26, 2021

neiljp Jul 30, 2021

neiljp commented Jul 31, 2021

		m = re.search(REGEX_AUTOCOMPLETE_STREAM_TOPIC, text)
		m_fenced = re.search(REGEX_AUTOCOMPLETE_STREAM_TOPIC_FENCED, text)

		# () Example text: #*stream name>Topic name
		REGEX_AUTOCOMPLETE_STREAM_TOPIC = r"#\\([^>]+)>([^]*)$"

		# Fenced prefix
		("#Stream 1>T", 0, "#Stream 1>Topic 1", True),

Add support for stream + topic autocomplete #1004

Add support for stream + topic autocomplete #1004

Conversation

Ezio-Sarthak commented Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkp6781 commented Apr 21, 2021

Ezio-Sarthak commented Apr 27, 2021

prah23 left a comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

neiljp commented Jul 31, 2021

Ezio-Sarthak commented Apr 21, 2021 •

edited

Loading