Add support for 'starred' typeaheads#794
Conversation
neiljp
added
enhancement
zulipbot
added
size: XL
preetmishra
left a comment
preetmishra
left a comment
There was a problem hiding this comment.
@neiljp Thanks for refactoring and extending the typeaheads. This would make fixing any typeahead typo easier. 👍
I pulled the changes locally and they seem to work well. However, I am not sure if we should support * for any other typeahead than group (as it is actually a form of the original syntax).
kaustubh-nair
left a comment
kaustubh-nair
left a comment
There was a problem hiding this comment.
@neiljp This looks like a great addition 👍
I've gone through this and it looks good to me.
| @@ -171,22 +171,28 @@ def generic_autocomplete(self, text: str, state: Optional[int] | |||
|
|
|||
| # Look in a reverse order to find the last autocomplete prefix used in | |||
| # the text. For instance, if text='@#example', use '#' as the prefix. | |||
There was a problem hiding this comment.
Not related to this commit, but I think I should point out that the server does allow names to start with #.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
|
Following discussion in #general>Typeahead prefixes, I plan to remove at least the single-star silent mention and possibly the stream single-star version - these should be easy enough to add back later, and even without these this seems a very useful addition to typeahead. |
neiljp
force-pushed
the
2020-09-01-starred-typeaheads
branch
from
3c70149 to
1512017
Compare
|
As per discussion, I've just updated this, and I think it's ready for merging 👍 @aero31aero This summarizes the ZT position, which I think is essentially as I proposed and we (almost) agreed! Let's discuss further if we want to bring the different front ends together in this or a similar way. The clarified main outcome, other than the refactors, is for autocomplete to work subsequent times after backspacing previously autocompleted text, ie. from As it stands, this specifically now leaves single-star completion absent, except for groups. So, we cannot do |
preetmishra
left a comment
preetmishra
left a comment
There was a problem hiding this comment.
@neiljp Thanks for the further amendments. 👍
This would add on to the user experience and make editing typeheads easier. Moreover, the consensus that we have reached on would hopefully promote the valid syntax.
This seems to works great locally. I have left two in-line comments.
zulipterminal/ui_tools/boxes.py
Outdated
| if not found_prefix_indices: | ||
| return text | ||
|
|
||
| # Use latest longest matching prefix (so @_ beats @ and #* beats #) |
There was a problem hiding this comment.
[minor] Is this supposed to be '#** beats #'?
There was a problem hiding this comment.
I removed this later text, as it's added in a later commit now.
| write_box.view.set_typeahead_footer.assert_not_called() | ||
|
|
||
| @pytest.mark.parametrize('text, state, footer_text', [ | ||
| # no-text mentions |
There was a problem hiding this comment.
[minor] The commit title says that * is added for non-silent mentions for groups & users. However, ** is being added for users as well.
There was a problem hiding this comment.
I've clarified this text 👍
neiljp
force-pushed
the
2020-09-01-starred-typeaheads
branch
from
1512017 to
4cb9cee
Compare
This generalizes the previous approach using `rfind`, to avoid the special case for silent mentions. Added note regarding limitation due to overlap between allowed characters in streams and mentions.
This retains the existing @ format for autocompleting groups and users, while adding the `@**` and `@*` prefixes to allow specifically autocompleting only users and groups respectively. Tests added. Fixes zulip#732 (with the group autocomplete).
neiljp
force-pushed
the
2020-09-01-starred-typeaheads
branch
from
4cb9cee to
195bc42
Compare
|
Thanks for the feedback so far everyone - I've addressed the concerns I'm aware of, and opened #796 for the issue @kaustubh-nair raised. |
preetmishra
left a comment
preetmishra
left a comment
There was a problem hiding this comment.
@neiljp Thanks for further fine-tuning the commits and logging the related issue. This seems to work well and with my previous concerns resolved, I believe this is ready to be merged. 👍
4 participants
This allows multiple aspects which improve the typeahead experience:
@usmay generate@**user**, but backspacing to@**udoesn't autocomplete)@**for autocompleting only users@*for autocompleting only groups (fixing Improve group typeahead #732)This doesn't resolve the same issue experienced in topic typeahead in the topic recipient box, namely that only full words can be autocompleted.