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: Indicate which streams and topics have unread @-mentions. #22583

Merged

Conversation

jai2201
Copy link
Collaborator

@jai2201 jai2201 commented Jul 25, 2022

Fixes: #21637

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.

@jai2201 jai2201 marked this pull request as draft July 25, 2022 16:06
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 4 times, most recently from 2ee493e to 985fd7f Compare August 5, 2022 03:09
@zulipbot zulipbot added size: XS and removed size: XL labels Aug 5, 2022
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch from 985fd7f to 86cf17b Compare August 5, 2022 03:12
@jai2201 jai2201 marked this pull request as ready for review August 5, 2022 03:18
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch from 86cf17b to 41f4c79 Compare August 5, 2022 03:24
@zulipbot zulipbot added size: L and removed size: XS labels Aug 5, 2022
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch from de3146e to 921bc0f Compare August 5, 2022 05:07
static/js/unread.js Outdated Show resolved Hide resolved
static/js/unread.js Outdated Show resolved Hide resolved
static/js/stream_list.js Outdated Show resolved Hide resolved
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 2 times, most recently from db946fd to 6bd9478 Compare August 5, 2022 20:49
@jai2201 jai2201 requested a review from timabbott August 5, 2022 20:49
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 3 times, most recently from 5335ca3 to 51d4a4f Compare August 5, 2022 20:56
static/js/unread.js Outdated Show resolved Hide resolved
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 3 times, most recently from 5c69b44 to 84937a4 Compare August 6, 2022 01:23
@zulipbot zulipbot added size: XL and removed size: L labels Aug 6, 2022
@timabbott
Copy link
Sponsor Member

I did some rewriting of this, primarily inspired by the insight that the reverse_lookup function just needed a small tweak to return the bucket_key rather than the bucket to support this without larger changes to the Bucketer class. I also eliminated some code duplication in stream_list.js, and fixed case-sensitivity issues with topics and undefined being incorrectly added to the set of topics with unread mentions.

I'd appreciate your tweaking the tests so that they would fail if we didn't handle case-sensitivity correctly.

There's a TODO I added that I'd like you to investigate.

@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch from c0930c5 to 10ad027 Compare August 20, 2022 06:06
static/js/stream_list.js Outdated Show resolved Hide resolved
@timabbott
Copy link
Sponsor Member

Also can you fix the node test? Might be best done in a new commit we plan to squash, just so it's easy for me to see your changes.

@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 2 times, most recently from 25bc93e to bb62169 Compare August 21, 2022 21:06
@jai2201
Copy link
Collaborator Author

jai2201 commented Aug 21, 2022

@timabbott , I added a prep commit above and rebased the other commits accordingly and made each individual commit pass the CI, can you please take a look at them now?

@timabbott
Copy link
Sponsor Member

timabbott commented Aug 22, 2022

I'm going to test-deploy this on chat.zulip.org.

Can you add a proper commit message to the new prep commit explaining why it's correct? Specifically, what code fills in the unread count already? And does the history suggest why this function existed if it's a noop? git log -p / git log -S may help explore that.

@jai2201
Copy link
Collaborator Author

jai2201 commented Aug 23, 2022

Ahh, Thanks a lot @timabbott for suggesting to look in the history why this function was introduced if it's a noop;

I looked through the commits in history and have found something tricky which makes this prep commit buggy;
Here's the link of the commit which introduces this piece of code.

@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 2 times, most recently from f107062 to 068a654 Compare August 23, 2022 10:26
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 2 times, most recently from a76da9d to a0a8bd9 Compare August 23, 2022 10:50
@jai2201
Copy link
Collaborator Author

jai2201 commented Aug 23, 2022

@timabbott , I've updated the PR with completing the TODO added and made the node tests pass for each individual commit;

Left some comments above for you to have a look;
Would like to request for a re-test deployment of this PR on CZO.

@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch 5 times, most recently from 4ddd0fd to 22a74f2 Compare August 25, 2022 10:09
@jai2201 jai2201 force-pushed the indicate_mention_info_on_stream_and_topic branch from 22a74f2 to 71204ab Compare August 26, 2022 08:56
@@ -30,6 +30,7 @@ const scroll_util = mock_esm("../../static/js/scroll_util", {
mock_esm("../../static/js/ui", {get_scroll_element: ($element) => $element});
mock_esm("../../static/js/unread", {
num_unread_for_stream: () => num_unread_for_stream,
stream_has_any_unread_mentions: () => stream_has_any_unread_mentions,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'm going to merge this, but I think it'd be better to change this test to use the real unread.js module; it's a data module, so it should be easy to setup with a bit of data for the relevant streams rather than mocking it.

@timabbott timabbott merged commit 663e9fe into zulip:main Aug 29, 2022
@timabbott
Copy link
Sponsor Member

Merged, thanks for doing this @jai2201! Hopefully you've learned a bit about how to do simple but efficient data structures through this project :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicate which streams and topics have unread @-mentions
3 participants