Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Fix stream_data.get_streams_for_settings_page() #10062

Open
showell opened this issue Jul 26, 2018 · 2 comments
Open

refactor: Fix stream_data.get_streams_for_settings_page() #10062

showell opened this issue Jul 26, 2018 · 2 comments

Comments

@showell
Copy link
Contributor

showell commented Jul 26, 2018

This is a fix that either @sampritipanda or I should do in the short term (as it's closely related to #9999) or, if we get diverted, by somebody with a pretty strong understanding of stream_data.js and basic performance issues with filtering and sorting.

The first thing wrong with stream_data.get_streams_for_settings_page() is that the name is a bit misleading. We use it for other stuff, like copy-from-stream and compose typeahead. So it's really more generic than the current name indicates--it gets streams in our canonical sorting order. But the problems are a bit deeper.

First of all, it doesn't implement the best possible sort. In subs.filter_table we do a locale-aware sort that's probably what we want for all three features. So we'd want to extract out the sort from that function, which unfortunately has some other stuff going on, so it's not a totally trivial extraction.

Then the next thing to be aware of is that we generally want to do filtering before sorting, and two of the three features support filtering. Our stream_data API should ideally be something more like this:

  • stream_data.get_ids_matching_filter (for compose typeahead and stream settings left-panel)
  • stream_data.sort_ids_by_locale_and_subscribed (for all three features)
  • stream_data.ids_to_subs
  • stream_data.get_sorted_subs (only to be used by copy-from-stream, although it should really have a filter)

I'm not 100% sure what the exact API should be, but it should allow the following:

  • all features share the same sorting algorithm
  • features have control on being able to pre-filter ids before sorting
@zulipbot
Copy link
Member

zulipbot commented Aug 7, 2018

Hello @showell, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

@zulipbot
Copy link
Member

zulipbot commented Aug 28, 2018

Hello @zulip/server-refactoring, @zulip/server-streams members, this issue was labeled with the "area: refactoring", "area: stream settings" labels, so you may want to check it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants