Skip to content

Conversation

preetmishra
Copy link
Member

The first commit is a refactor that extracts streams_view_divider() from streams_view().

The streams_view_divider() is used in the second commit, in update_streams, for inserting a divider between search results.

Fixes #577.

@zulipbot zulipbot added size: L [Automatic label added by zulipbot] enhancement New feature or request area: UI General user interface update labels Jun 29, 2020
@kaustubh-nair
Copy link
Member

kaustubh-nair commented Jun 30, 2020

@preetmishra This looks like a useful visual improvement 👍
One bug I noticed is two lines are displayed if I search for something then clear the text with backspace.
(Oops closed the pr by mistake)

@preetmishra
Copy link
Member Author

@kaustubh-nair Thanks for the bug report. 👍

I couldn't notice it initially it as I only have a few streams pinned and the stream list would update past the pinned streams whenever I cleared the search with backspace (#700 will fix this). However, with a bunch of streams pinned, it was pretty straightforward to reproduce.

I have pushed an update that will resolve the bug.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preetmishra This looks good 👍 I'm surprised how much of a difference this makes to the search result display, but it demonstrates how we order the results, which perhaps explains why :)

As per my inline comment, I think the refactor doesn't belong in helper.py. That's both since we're basically generating a UI element, and if we treat it as literally a subclass then it fits with the others in UI files; it also has the workaround which refers to the StreamButton, so that points towards keeping it together with UI code too. We don't note what to have in helper.py, but I'd prefer to limit it as much as possible, where the code could belong with the other M/V/C components.

@preetmishra preetmishra force-pushed the stream-search-divider branch from 22a9e26 to d31e169 Compare July 5, 2020 18:16
@preetmishra preetmishra force-pushed the stream-search-divider branch from d31e169 to 58d8a4e Compare July 5, 2020 18:31
@preetmishra
Copy link
Member Author

@neiljp Thanks for the review and the pointers.

I have extracted the divider as a subclass now and moved it into in views.py. The subclass does improve readability and make it cleaner and the code does fit better in views.py.

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preetmishra Just a few minor points to consider 👍

This extracts StreamsViewDivider from streams_view as a class (subclass
of urwid.Divider) to facilitate code reusability.

Tests added and amended.
This uses StreamsViewDivider to insert a divider between the pinned and
the unpinned streams.

Test amended.

Fixes zulip#577.
@preetmishra preetmishra force-pushed the stream-search-divider branch from 58d8a4e to c43f8dd Compare July 8, 2020 19:14
@preetmishra
Copy link
Member Author

@neiljp Thanks for the review. I have incorporated all the suggestions in the latest update.

@neiljp
Copy link
Collaborator

neiljp commented Jul 8, 2020

@preetmishra Thanks for working on this! I'm merging this now 🎉

@neiljp neiljp merged commit ea1e55d into zulip:master Jul 8, 2020
@preetmishra preetmishra deleted the stream-search-divider branch July 10, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UI General user interface update enhancement New feature or request size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate pinned and unpinned streams while searching.
4 participants