Order typeahead by narrow/recipients #730

kaustubh-nair · 2020-07-22T12:25:44Z

This PR orders the mentions typeahead such that participants/subscribers in the current conversation are shown first.
There are two parts to this:

Part 1: Add ordering logic (Commits 1 & 2) - This is done by adding a recipient_user_ids attribute to WriteBox, storing the user ids of the participants. This is supported only for private messages because subscriber data for streams is not available. Note that storing recipient ids in MessageBox is necessary because there is no other way to obtain the user ids.
Part 2: Add subscriber data for streams thus enabling ordering (Commits 4 & 5) - Set include_subscribers to True while fetching initial data to get stream subscriber data. Just store this in WriteBox.recipient_user_ids and voila!

neiljp

@kaustubh-nair This looks useful :)

Regarding streams, I think we should be careful that we don't forget to keep our stream-subscriber lists up to date via subscription events. If ZT runs for some time then these will become out of date. For a long time we didn't update the presence list and it was a little too easy to forget it was out of date.

I was testing the stream autocomplete on ZT, but there's more users subscribed to that stream than I realized!

tests/ui_tools/test_boxes.py

zulipterminal/ui_tools/boxes.py

tests/model/test_model.py

zulipterminal/model.py

kaustubh-nair · 2020-07-23T21:26:37Z

@neiljp Thanks for the review, updated the PR

neiljp

@kaustubh-nair This user ordering is definitely better without 'ourselves' 👍

There are a few remaining points to fix/discuss, but this is close to merging, I think?

With a workaround for the 'no subscriber' case, I think we could leave the problem of updating potential 'subscribers' when the target of the message changes in the compose box to a a later PR, unless you want to add it as an extra commit? If we do leave it then could you please open an issue if you don't plan to work on it straight away?

Another follow-up aspect we could consider would be 'dividers' in the footer, perhaps, to separate 'this conversation' from 'others'? That could be reflected in partitioning the user list based on 'in this conversation' too? (sort of pinning them above?) That's probably worth discussing :)

neiljp · 2020-07-23T22:26:59Z

zulipterminal/ui_tools/boxes.py

+        print(self.subscriber_ids, flush=True)
+        # Remove current user.
+        self.subscriber_ids.remove(self.model.user_id)
+        print(self.subscriber_ids, flush=True)


Perhaps add commits between the ones you push, for debugging, to drop in the push but which you can put back? :)

Oh no, I keep forgetting to check for these 🤦

zulipterminal/ui_tools/boxes.py

neiljp · 2020-07-23T23:03:51Z

zulipterminal/ui_tools/boxes.py

@@ -36,8 +37,14 @@ def main_view(self, new: bool) -> Any:
    def set_editor_mode(self) -> None:
        self.view.controller.enter_editor_mode_with(self)

-    def private_box_view(self, button: Any=None, email: str='') -> None:
+    def private_box_view(self, button: Any=None, email: str='',
+                         subscriber_ids: List[int]=[]) -> None:


A default argument like this is one of the traps in python. If we truly need a default argument then we need to use None as default.

Note that there are other calls to private_box_view, and at least x can cause a crash right now. We could work around that in the first instance, but ultimately I think we want to update subscribers/recipients when the stream/users list is updated. For example, if I compose to #announce, but edit it to go to #test here, then autocomplete is still based on announce right now.

OK, a shallow copy is causing the crash here rather than the change in recipients. I think it is fine to not have the current user stored in the recipients at all? That would also be consistent with the other recipient data we're storing in MessageBox

Updating subscribers requires a little more discussion, opened #737

On second thought, this wouldn't be too hard I'll fix this in the same PR

zulipterminal/ui_tools/boxes.py

neiljp · 2020-07-24T01:16:52Z

I also just got a crash from P @ Esc @, which is probably the same issue as before.

neiljp

@kaustubh-nair Thanks for adding the subscription events 👍 Here's the next iteration of comments.

neiljp · 2020-07-24T23:30:59Z

zulipterminal/ui_tools/boxes.py

@@ -36,8 +37,10 @@ def main_view(self, new: bool) -> Any:
    def set_editor_mode(self) -> None:
        self.view.controller.enter_editor_mode_with(self)

-    def private_box_view(self, button: Any=None, email: str='') -> None:
+    def private_box_view(self, button: Any=None, email: str='',
+                         recipient_user_ids: List[int]=[]) -> None:


I know that we're not modifying the list right now, but this would be safer if we treated this as Optional[List[int]]=None, as I suggested in the last review, in case we later do so. Here's an article discussing this: https://towardsdatascience.com/python-pitfall-mutable-default-arguments-9385e8265422

Oh no, that's horrible!

Another approach I've seen which we could try here is to use the type checker and only allow passing in a Sequence, but the change you've made looks fine.

zulipterminal/ui_tools/boxes.py

tests/model/test_model.py

zulipterminal/ui_tools/boxes.py

tests/conftest.py

zulipterminal/model.py

neiljp · 2020-07-25T21:45:37Z

tests/model/test_model.py

+    def test__handle_subscription_event_muting(self, model, mocker,
+                                               stream_button, event,
+                                               final_muted_streams):


Is this intentional?

Yes, this test deals only with muting, I'll move this to the previous commit though

neiljp · 2020-07-25T21:50:30Z

zulipterminal/ui_tools/boxes.py

        self.set_editor_mode()
+        subscribers = deepcopy(self.model.get_subscribers_in_stream(stream_id))


Why not return a new list in the method, and skip the need for the deepcopy? Alternatively, we're only using it here so far, so we could change it to be get_other_subscribers_in_stream and incorporate the extra code below?

Thismethod can be used for displaying subscriber counts.

Returning a new list is the same as using deepcopy(I think?), so I would just be moving the deepcopy to the model?

OK, modified it to get_other_subscribers_in_stream() and used the stream_dict directly for subscribers counts. Removed the test because I don't see a point in writing an entire test just for a simple assert_called_once_with(). This can added when the whole method is tested.

neiljp · 2020-07-25T22:04:31Z

@kaustubh-nair In addition to the few minor points above, I'd suggest you checking the other WriteBox uses since eg. R causes a crash for me from a stream - though this seems to be related to MessageBox not having recipient_ids.

Show user-name suggestions first instead of groups, since they are used more often. This is important when a user is using typeahead with an empty query ( for example just '@' ) because in the future, when typeaheads are ordered by the participants in the current narrow - having groups first would not make this ordering apparent on empty queries.

This commit orders typeahead for mentions such that participants of the private message/group message are displayed first. WriteBox now stores an additional attribute called recipient_user_ids which stores the user_ids of the recipients/subscribers in a conversation. Test added.

The subscription event includes an 'op' attribute to differentiate between different operations possible for updating subscriptions. This this commit changes subscription handling to use the 'update' operation for identifying the mute/unmute events. Tests parameters updated, and renamed the test appropriately.

The optional flag include_subscribers in client.register is set to True so that subscriber data for streams is returned. This data can be useful for multiple features, such as ordering typeahead by subscribers, and displaying number of subscribers in stream information. This commit also handles the subscription event for updating the stream subscribers. Test added.

Update recipient_user_ids in WriteBox for stream messages which is used to order mentions typeahead. The logic for doing this ordering has already been implemented in WriteBox.autocomplete_mentions(). stream_id is passed to WriteBox instead of using stream names to look-up subscriber data to avoid bugs due to duplicate stream names.

neiljp · 2020-08-05T23:09:25Z

@kaustubh-nair Thanks for getting this work done, it makes the typeahead much more useful and the footer list more informative 👍
I just merged this manually after updating a few commit texts only, which you should be able to determine using git range-diff 🎉

I know you have other typeahead work underway, but this change of footer ordering suggests we could consider other styles to use in the footer for typeahead or temporary notices, and/or maybe separators to mark different sections.

Since Model.stream_dict does not contain data for unsubscribed streams, trying to handle this event raises exceptions. Fixes issue introduced in #730. Test added.

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 22, 2020

kaustubh-nair force-pushed the order-typeahead-by-narrow branch 4 times, most recently from ac53aae to 25b38c1 Compare July 22, 2020 19:24

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 22, 2020

neiljp reviewed Jul 23, 2020

View reviewed changes

neiljp added this to the Release after upcoming milestone Jul 23, 2020

neiljp removed the PR needs review PR requires feedback to proceed label Jul 23, 2020

kaustubh-nair force-pushed the order-typeahead-by-narrow branch from 25b38c1 to 1ec052f Compare July 23, 2020 05:50

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

kaustubh-nair force-pushed the order-typeahead-by-narrow branch from 1ec052f to 5e152f6 Compare July 23, 2020 06:19

kaustubh-nair mentioned this pull request Jul 23, 2020

Improve group typeahead #732

Closed

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 23, 2020

neiljp reviewed Jul 24, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Jul 24, 2020

kaustubh-nair mentioned this pull request Jul 24, 2020

Deal with manual changes to recipients for WriteBox #737

Closed

kaustubh-nair force-pushed the order-typeahead-by-narrow branch from 5e152f6 to e0816b3 Compare July 24, 2020 20:10

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

kaustubh-nair force-pushed the order-typeahead-by-narrow branch 4 times, most recently from bbc238a to b6d21a9 Compare July 24, 2020 20:42

neiljp reviewed Jul 25, 2020

View reviewed changes

kaustubh-nair force-pushed the order-typeahead-by-narrow branch 2 times, most recently from 4ab3291 to 1dfa9f4 Compare July 25, 2020 17:32

kaustubh-nair commented Jul 25, 2020

View reviewed changes

zulipterminal/model.py Show resolved Hide resolved

neiljp reviewed Jul 25, 2020

View reviewed changes

kaustubh-nair force-pushed the order-typeahead-by-narrow branch 5 times, most recently from 3678459 to ae1c02a Compare July 27, 2020 15:34

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 29, 2020

kaustubh-nair mentioned this pull request Jul 30, 2020

Add member list to stream info view #753

Closed

kaustubh-nair removed the PR needs review PR requires feedback to proceed label Aug 4, 2020

kaustubh-nair added 4 commits August 4, 2020 18:15

kaustubh-nair force-pushed the order-typeahead-by-narrow branch from ae1c02a to c9635db Compare August 4, 2020 14:09

kaustubh-nair force-pushed the order-typeahead-by-narrow branch from c9635db to 0c84676 Compare August 4, 2020 15:28

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

neiljp closed this Aug 5, 2020

neiljp mentioned this pull request Nov 22, 2020

TRACKING: Typeahead improvements #448

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order typeahead by narrow/recipients #730

Order typeahead by narrow/recipients #730

kaustubh-nair commented Jul 22, 2020 •

edited

Loading

neiljp left a comment

kaustubh-nair commented Jul 23, 2020

neiljp left a comment

neiljp Jul 23, 2020

kaustubh-nair Jul 24, 2020

neiljp Jul 23, 2020

kaustubh-nair Jul 24, 2020

kaustubh-nair Jul 24, 2020

kaustubh-nair Jul 24, 2020

neiljp commented Jul 24, 2020

neiljp left a comment

neiljp Jul 24, 2020

kaustubh-nair Jul 25, 2020

neiljp Jul 25, 2020

neiljp Jul 25, 2020

kaustubh-nair Jul 26, 2020

neiljp Jul 25, 2020

kaustubh-nair Jul 26, 2020 •

edited

Loading

kaustubh-nair Jul 26, 2020 •

edited

Loading

kaustubh-nair Jul 26, 2020

neiljp commented Jul 25, 2020

neiljp commented Aug 5, 2020

		self.set_editor_mode()
		subscribers = deepcopy(self.model.get_subscribers_in_stream(stream_id))

Order typeahead by narrow/recipients #730

Order typeahead by narrow/recipients #730

Conversation

kaustubh-nair commented Jul 22, 2020 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

kaustubh-nair commented Jul 23, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Jul 24, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-nair Jul 26, 2020 • edited Loading

Choose a reason for hiding this comment

kaustubh-nair Jul 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Jul 25, 2020

neiljp commented Aug 5, 2020

kaustubh-nair commented Jul 22, 2020 •

edited

Loading

kaustubh-nair Jul 26, 2020 •

edited

Loading

kaustubh-nair Jul 26, 2020 •

edited

Loading