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

unread: Make the n key respect the left sidebar filter. #22798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jai2201
Copy link
Collaborator

@jai2201 jai2201 commented Aug 27, 2022

When going to the next unread message we should restrict
the streams according to the stream filter, if it is filled in.
And similarly for the topic filter, if you're currently
viewing "more topics".

So, modified the algorithm to iterate through the unread
actively-filtered topics first, then the other unread
topics in the stream, and then all the unfiltered
topics of other streams (no filters applied for any of those).

Fixes #21437.

Screenshots and screen captures:

Self-review checklist

  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

When going to the next unread message we should restrict
the streams according to the stream filter, if it is filled in.
And similarly for the topic filter, if you're currently
viewing "more topics".

So, modified the algorithm to iterate through the unread
actively-filtered topics first, then the other unread
topics in the stream, and then all the unfiltered
topics of other streams (no filters applied for any of those).

Fixes zulip#21437.
@zulipbot
Copy link
Member

Hello @zulip/server-sidebars members, this pull request was labeled with the "area: left-sidebar" label, so you may want to check it out!

@jai2201
Copy link
Collaborator Author

jai2201 commented Aug 27, 2022

@sayamsamal , Can you please review this PR.

@jai2201
Copy link
Collaborator Author

jai2201 commented Sep 1, 2022

@alya can you please take a look at this.

@alya
Copy link
Contributor

alya commented Sep 8, 2022

@sayamsamal please review this PR when you get a chance. Thanks!

@alya
Copy link
Contributor

alya commented Sep 12, 2022

Since it looks like @sayamsamal is not currently available, @punchagan I wonder if you'd be up for doing an initial review on this PR?

@sayamsamal
Copy link
Collaborator

@alya Sorry for the delayed heads up, I have actually completed walking through and reviewing this PR.

Here are my observations,

  • While using the topic filter
  1. The 'n' key first goes through all the filtered list of topics.
  2. Then it iterates over all the unread messages, irrespective of the topic filter applied.
  • While using the stream filter
  1. The 'n' key first goes through all the topics of the filtered stream(s)
  2. Once all the unread messages from the filtered stream(s) are read, it stops iteration even if other unread messages exist.

From these observations, and having gone through the CZO and the issue, do we want the topic filter to reach the second step, where it iterates over all the unread messages, or should it stop once the filtered list of unread messages are iterated through?

FYI, @jai2201

@alya
Copy link
Contributor

alya commented Sep 13, 2022

Hm, good question!

I don't have a super clear intuition on this, other than that we should be consistent between stream and topic filters. :)

@jai2201 would you be up for asking what folks think in a #design conversation?

@alya
Copy link
Contributor

alya commented Sep 13, 2022

I also think either approach would be an improvement over the current state (as long as streams/topics are consistent), so if one is much easier to implement, we could start with that.

@zulipbot
Copy link
Member

Heads up @jai2201, we just merged some commits that conflict with the changes you 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.

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

Successfully merging this pull request may close these issues.

Make the n key respect the left sidebar filter
4 participants