streams: Handle adding/removing stream events.

[Abhirup-99]

Fixes #816

[neiljp]

@Abhirup-99 Thanks for giving this a go 👍

There are a few issues with the PR as it stands - how have you tested this? I tried subscribing to a stream in the webapp and the list of streams in ZT doesn't update.

I would recommend checking what the existing code does for subscriptions when they are loaded at startup, ie. any translation of the data, as well as how the stream list is updated and re-rendered in other cases.

Regarding your question in the stream, adding remove events would be good to include in the same PR.

[Abhirup-99]

@neiljp
Sorry, the pr became a bit longer than I had anticipated it to be. This happened because of the addition of the tests, the incoming events from add_stream event is quite large so that primarily contributed to it, also handling of some race around conditions when the user is removed from a stream but is that stream is on view. I disabled some of the keypress events when that condition occurs. Any other thoughts/suggestions would be helpful.

[neiljp]

@Abhirup-99 This looks more detailed than the previous version and I can see that it does add and remove streams from the list 👍

The issues I raised earlier are still present however, but I hope my comments will make the color/sorting issues clearer re the addition of streams. We should also ensure other data in the model is updated appropriately, eg. at least muted streams and stream_dict (that I noted so far in this review).

Once you understand the above, you might notice some common duplicated code. It would likely be useful to make a refactoring, extracting the current processing of initial subscriptions (including colors, storing muting state) into an internal method that can process additional streams later too - and would centralize the processing of the data. Possibly we could do another which would handle the StreamData updates, including eg. the sorting. That would likely be better as a preparatory commit (or two) retaining the current behavior but generating methods you can use in the add/remove commits.

It's good to see you covering the removal of streams, but I'd suggest we split the UI warnings into a separate commit; this is important work, but the behavior needs clarifying. For example, while we should limit unnecessary server calls if they're going to fail in any case, I'm not entirely sure if posting/editing to an unsubscribed stream is limited - and maybe other similar aspects too.

[Abhirup-99]

@zulipbot add "PR needs review"

[neiljp]

@Abhirup-99 This still works well and the data is updated much more cleanly for the most part 👍

Event handling can often expose challenges with the data model, and this PR shows how we can improve it further. So, we could tidy up what you have a little (updating for at least the muted removal and stream_dict vs StreamData issue), or use some of the new code you have as a basis for quite some refactoring where we combine the generation of the model data from initial_data with how it is updated in these events. Let me know how you want to proceed.

On the basis of the latter being the case, I wanted to get some comments back to you, and have focused mainly on the first commit. Other than one function, you've got some new functions in that commit, but also some that are duplicated from elsewhere, I think? To avoid that duplication it would be great to see commits where when you add those methods for use in the new code, that you remove them from the other place(s) and call the new method. In such 'refactors' we move code around (or otherwise adjust it) so that it can be adapted or reused more easily in a later commit - and when tests don't break after a refactor commit, we hopefully know things are still good too!

[neiljp]

@Abhirup-99 You replied to some points, but do you want to do some big refactoring first, or go for a simpler version? I know what I'd like to do for the refactoring, but we could get this fixed first.

[Abhirup-99]

@zulipbot add "PR needs review".

[neiljp]

@Abhirup-99 I've pushed a slight adjustment of the first refactor commit since it's definitely a solid improvement and foundation for the other work.

There should be less to resolve here now after the few cherry-picks, and while a visual/functional change is maybe not yet evident, I hope you can see the code foundation shifting to make this easier. The challenge is often uncovering the data-structures underneath which need updating first!

[zulipbot]

Heads up @Abhirup-99, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.