Recipient box typeahead for streams and topics #746

kaustubh-nair · 2020-07-28T18:37:54Z

No description provided.

kaustubh-nair · 2020-07-28T18:42:22Z

zulipterminal/helper.py

+def match_stream_names(stream_names: List[str], search_text: str) -> List[str]:
+    return [name for name in stream_names
+            if name.lower().startswith(search_text.lower())]
+
+


There are no pinned stream considerations here - I'm not using the existing function.
Refactoring match_streams looks a little hard, and pinning order is not super important to this PR - so I'd like to deal with this in a follow-up.

Though one hacky way to use the current match_streams would be to pass [(stream_name, stream_name)..] in data.

Don't you think pinning preferences should be relevant here?

They are certainly an improvement, but I don't see it as being a necessity for this particular feature. I'll make a temp fix but open an issue and make a follow-up PR for refactoring match_stream

neiljp · 2020-07-31T17:56:24Z

@kaustubh-nair Is this PR supposed to be functional at this point?
update Apologies, I had temporarily installed the latest release in non-editable mode!

neiljp

@kaustubh-nair This is pretty effective already 👍

In addition to the points inline, if I r or <enter> to a message, backspace the topic by one and press an autocomplete key, it claims no matches - and continues to do so if I add back the last letter. This also fails if I just leave it as it is! If I delete everything, then it finds it as the first choice. Is this related to not exiting the autocomplete mode properly?

neiljp · 2020-07-31T21:19:19Z

zulipterminal/helper.py

+def match_stream_names(stream_names: List[str], search_text: str) -> List[str]:
+    return [name for name in stream_names
+            if name.lower().startswith(search_text.lower())]
+
+


Don't you think pinning preferences should be relevant here?

zulipterminal/ui_tools/boxes.py

neiljp

@kaustubh-nair Thanks for the rebase & conflict resolution 👍

Re stream autocomplete, I've tried some manual tests and combined with the validation it's generally great.

One problem is definitely that "test "`^f" autocompletes to "test announce", etc, which at least on czo is invalid - it's a combination of the first stream and another. Is this the multi-word match problem you mentioned on czo? (it's not matching the full text, just a word since last space). I can see the same behavior in the topic list. This is a subtly different bug from the same origin?

We can now improve the stream validation so that on we don't just say 'invalid stream name' but suggest using ^f/^r (or just looking in the stream list!). This could be another commit/PR, but would definitely help the user be aware of the new feature in this PR and avoid the "my stream is invalid" experience.

Re topic autocomplete, this seems to be failing for changing streams, after @preetmishra 's dynamic topic-loading. That is, it finds no matches if the stream topics have not been loaded.

It would be great if we could add some tests for this, given these behaviors - we could track the strange multi-word behavior with xfail too, for example.

zulipterminal/model.py

zulipterminal/ui_tools/boxes.py

kaustubh-nair · 2020-08-07T18:18:31Z

tests/ui_tools/test_boxes.py

+    def test__process_typeaheads(self, write_box, typeaheads, suggestions,
+                                 state, expected_state, expected_typeahead,
+                                 is_truncated, mocker):
+        write_box.view.set_typeahead_footer = mocker.patch(
+                                'zulipterminal.ui.View.set_typeahead_footer')
+
+        typeahead = write_box._process_typeaheads(typeaheads, state,
+                                                  suggestions)
+
+        assert typeahead == expected_typeahead
+        write_box.view.set_typeahead_footer.assert_called_once_with(
+                            suggestions[:10], expected_state, is_truncated)
+


I'm not sure what to do about the existing generic_autocomplete tests now, since now they span two different methods. Changing them to check for _process_typeaheads would be hard because the parameters would get complex. (Because we'd have to create lists of suggestions)

zulipterminal/ui_tools/boxes.py

kaustubh-nair · 2020-08-07T19:38:03Z

@neiljp Thanks for the review! The problem you see for stream autocomplete is also the same bug caused due to readline's ability to autocomplete only words. It shows all possible suggestions for a an empty search string.
I've pushed a newer version with the following updates:

Added tests with cases for failing whitespace errors.
Fix for changing topics by dynamically loading them when needed.
New commit for improving invalid stream error message.

neiljp

@kaustubh-nair Thanks for the update and clarifying the issues so far - I really want to get this in!

We likely do want to update the help text for autocomplete keys slightly with the topic change?

Could we file a ZT (maybe also urwid-readline) issue and refer to it in the xfails and commit text? (we mention it not quite working already)

Good follow-ups might be stream+topic autocomplete (#**stream>topic**) in the message content. Refactoring tests and unifying the match_stream behavior would be a nice to have, but less directly impactful, and are perhaps longer topics to discuss.

tests/ui_tools/test_boxes.py

zulipterminal/ui_tools/boxes.py

zulipterminal/model.py

zulipterminal/ui_tools/boxes.py

neiljp · 2020-08-13T02:05:49Z

zulipterminal/ui_tools/boxes.py

+            typeahead = text[:prefix_index] + typeahead
+        return typeahead
+
+    def _process_typeaheads(self, typeaheads: List[str], state: Optional[int],


This is a very general method name - is there anything more specific we could use?

I kept it general because there are multiple things happening here - returning typeahead after checking for state constraints, and setting suggestions to footer.

tests/ui_tools/test_boxes.py

neiljp · 2020-08-13T02:13:53Z

tests/ui_tools/test_boxes.py

+        'invalid_state-None',
+        'invalid_state-greater_than_possible_index',
+        'invalid_state-less_than_possible_index',


While accurate, are these literally invalid state? Do they represent internal state that should not happen? If we're testing the range of inputs, should we cover those that the typeahead footer understands/expects?

It's also difficult knowing whether to test this method directly, rather than just cases where it's used, which I think you're getting the feeling of from your other review comment.

@neiljp I'm not sure whose internal state you're referring to - is that zt or readline? They are invalid, in the sense that they are indexes that do not exist for the array under consideration.

We don't have to cover all cases for footer since it is tested completely in test_ui?

I think it is okay to test method in isolation, as long as we cover all the cases for invalid states. This would mean that other tests for autocomplete methods will require fewer test parameters, but they would be difficult to write because we'd have to use lists instead, unless of course we use really small lists.

By invalid, I meant whether these are states that we shouldn't reach inside ZT when we call this method. If so, how much of them should we test? If they can be reached, then of course we could test them, but there may be something more meaningful than 'invalid' to describe them.

The challenge here is that these methods are quite internal to all the typeahead and have been changing around recently with all the development of this area!

Some of this is tested elsewhere - we certainly should consider whether we're over-testing the autocomplete to some degree?

tests/ui_tools/test_boxes.py

This is done so that state and typeahead calculations can be moved to a new method.

This helps in generalizing the autocomplete methods so that they can be used by other widgets. Test added.

kaustubh-nair · 2020-08-16T16:50:03Z

@neiljp Thanks for the review! PR updated.

Note that this does not work correctly when the search text includes whitespaces, due to lack of urwid-readline support. Refer to zulip#776. Test added.

kaustubh-nair · 2020-08-16T16:58:42Z

README.md

+| -----------------------------------------------------     | --------------------------------------------- |
+| Cycle through recipient and content boxes                 | <kbd>tab</kbd>                                |
+| Send a message                                            | <kbd>Alt Enter</kbd> / <kbd>Ctrl d</kbd>      |
+| Autocomplete @mentions, #stream_names, :emoji: and topics | <kbd>Ctrl</kbd> + <kbd>f</kbd>                |


I hope this is okay, we don't have wrapping text for topics, nor do we support typeahead for it in message box

Not yet! ;)

…nt box. Note that this does not work correctly when the search text includes whitespaces, due to lack of urwid-readline support. Refer to zulip#776. Test added.

Now that autocomplete for stream box has been added, we can show a hint to use autocomplete when the user inputs an invalid stream name.

neiljp

@kaustubh-nair Thanks for the updates - I'm merging this manually now 🎉 (with just a very minor adjustment of the second commit title to illustrate what I meant in the comment below)

As per earlier discussion, typeahead and validation is pretty solid now, within the bounds of urwid-readline, with a few outstanding issues that would really gives us some parity with the webapp and polish - we can discuss on czo if you want to follow these up, though I know there are other PRs to complete 👍

neiljp · 2020-08-18T16:04:11Z

tests/ui_tools/test_boxes.py

+        'invalid_state-None',
+        'invalid_state-greater_than_possible_index',
+        'invalid_state-less_than_possible_index',


By invalid, I meant whether these are states that we shouldn't reach inside ZT when we call this method. If so, how much of them should we test? If they can be reached, then of course we could test them, but there may be something more meaningful than 'invalid' to describe them.

The challenge here is that these methods are quite internal to all the typeahead and have been changing around recently with all the development of this area!

Some of this is tested elsewhere - we certainly should consider whether we're over-testing the autocomplete to some degree?

neiljp · 2020-08-18T16:08:54Z

tests/ui_tools/test_boxes.py

@@ -251,6 +251,36 @@ def test_generic_autocomplete_emojis(self, write_box, text,
        typeahead_string = write_box.generic_autocomplete(text, state)


minor: 2nd commit title: While 'move into new method' is fine, you might consider 'Extract (into) method' in future, in line with the refactoring terminology?
(eg. https://refactoring.guru/extract-method)

neiljp · 2020-08-18T16:46:19Z

README.md

+| -----------------------------------------------------     | --------------------------------------------- |
+| Cycle through recipient and content boxes                 | <kbd>tab</kbd>                                |
+| Send a message                                            | <kbd>Alt Enter</kbd> / <kbd>Ctrl d</kbd>      |
+| Autocomplete @mentions, #stream_names, :emoji: and topics | <kbd>Ctrl</kbd> + <kbd>f</kbd>                |


Not yet! ;)

zulipbot added the size: L [Automatic label added by zulipbot] label Jul 28, 2020

kaustubh-nair commented Jul 28, 2020

View reviewed changes

kaustubh-nair force-pushed the recipient-typeahead branch from 7cb3c7b to fc0a882 Compare July 28, 2020 18:43

kaustubh-nair added the feedback wanted label Jul 28, 2020

neiljp reviewed Jul 31, 2020

View reviewed changes

kaustubh-nair force-pushed the recipient-typeahead branch from fc0a882 to 7d6cb0f Compare August 5, 2020 18:12

neiljp added has conflicts and removed has conflicts labels Aug 6, 2020

kaustubh-nair force-pushed the recipient-typeahead branch from 7d6cb0f to 20b6c31 Compare August 6, 2020 14:17

zulipbot removed the has conflicts label Aug 6, 2020

neiljp reviewed Aug 6, 2020

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

neiljp removed the feedback wanted label Aug 6, 2020

kaustubh-nair force-pushed the recipient-typeahead branch from 20b6c31 to f6d1f71 Compare August 6, 2020 20:55

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Aug 6, 2020

kaustubh-nair force-pushed the recipient-typeahead branch 2 times, most recently from a66b82b to 508cf9a Compare August 7, 2020 17:54

kaustubh-nair commented Aug 7, 2020

View reviewed changes

zulipterminal/ui_tools/boxes.py Show resolved Hide resolved

kaustubh-nair commented Aug 7, 2020

View reviewed changes

zulipterminal/ui_tools/boxes.py Show resolved Hide resolved

kaustubh-nair force-pushed the recipient-typeahead branch from 508cf9a to 6fbcc15 Compare August 7, 2020 19:31

kaustubh-nair added the PR needs review PR requires feedback to proceed label Aug 8, 2020

kaustubh-nair force-pushed the recipient-typeahead branch from 6fbcc15 to fbe2027 Compare August 10, 2020 14:19

neiljp reviewed Aug 13, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 13, 2020

neiljp mentioned this pull request Aug 13, 2020

Single message drafts #772

Closed

neiljp added the has conflicts label Aug 16, 2020

kaustubh-nair force-pushed the recipient-typeahead branch from fbe2027 to 9ea018b Compare August 16, 2020 16:17

zulipbot removed the has conflicts label Aug 16, 2020

kaustubh-nair force-pushed the recipient-typeahead branch from 9ea018b to 5baba7c Compare August 16, 2020 16:21

kaustubh-nair commented Aug 16, 2020

View reviewed changes

tests/ui_tools/test_boxes.py Show resolved Hide resolved

kaustubh-nair force-pushed the recipient-typeahead branch 3 times, most recently from a56859a to 495a1c2 Compare August 16, 2020 16:33

kaustubh-nair added 2 commits August 16, 2020 22:05

refactor: boxes: Rearrange typeahead calculation to group general logic.

d5878b7

This is done so that state and typeahead calculations can be moved to a new method.

refactor: boxes: Move typeahead and state calculation to new method.

e204e43

This helps in generalizing the autocomplete methods so that they can be used by other widgets. Test added.

kaustubh-nair force-pushed the recipient-typeahead branch 2 times, most recently from 77a6f0d to 16b207a Compare August 16, 2020 16:49

kaustubh-nair added the PR needs review PR requires feedback to proceed label Aug 16, 2020

neiljp added this to the Next Release milestone Aug 16, 2020

boxes: Support typeahead for stream name in recipient box.

bd64930

Note that this does not work correctly when the search text includes whitespaces, due to lack of urwid-readline support. Refer to zulip#776. Test added.

kaustubh-nair force-pushed the recipient-typeahead branch from 16b207a to 306e984 Compare August 16, 2020 16:55

kaustubh-nair commented Aug 16, 2020

View reviewed changes

kaustubh-nair added 2 commits August 17, 2020 17:21

README/keys/boxes/helper: Support typeahead for topic name in recipie…

1e076ba

…nt box. Note that this does not work correctly when the search text includes whitespaces, due to lack of urwid-readline support. Refer to zulip#776. Test added.

boxes: Improve error message for invalid stream name.

64ce684

Now that autocomplete for stream box has been added, we can show a hint to use autocomplete when the user inputs an invalid stream name.

kaustubh-nair force-pushed the recipient-typeahead branch from 306e984 to 64ce684 Compare August 17, 2020 12:21

neiljp approved these changes Aug 18, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 18, 2020

neiljp closed this Aug 18, 2020

This was referenced Aug 30, 2020

Show & autocomplete PM recipient names & emails #587

Closed

Recipient box typeaheads/autocomplete (streams) #586

Closed

		@@ -251,6 +251,36 @@ def test_generic_autocomplete_emojis(self, write_box, text,
		typeahead_string = write_box.generic_autocomplete(text, state)

Uh oh!

Recipient box typeahead for streams and topics #746

Recipient box typeahead for streams and topics #746

Uh oh!

Conversation

kaustubh-nair commented Jul 28, 2020

Uh oh!

kaustubh-nair Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaustubh-nair commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaustubh-nair commented Aug 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaustubh-nair Jul 28, 2020 •

edited

Loading

kaustubh-nair Jul 28, 2020 •

edited

Loading

neiljp commented Jul 31, 2020 •

edited

Loading

kaustubh-nair commented Aug 7, 2020 •

edited

Loading