Skip to content

Fix unread topic case sensitivity. #1214

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix unread topic case sensitivity. #1214

wants to merge 1 commit into from

Conversation

apoorvapendse
Copy link
Contributor

@apoorvapendse apoorvapendse commented Dec 27, 2024 •
edited
Loading

Fixes #980.

Before After
before after

CZO Discussion.

@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch from 31879bb to 475f35b Compare December 27, 2024 06:18
@apoorvapendse apoorvapendse marked this pull request as draft December 27, 2024 06:19
@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch from 475f35b to cc82251 Compare February 2, 2025 18:14
@apoorvapendse apoorvapendse marked this pull request as ready for review February 2, 2025 18:14
@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch 4 times, most recently from 12ab0a9 to 711d659 Compare February 2, 2025 18:36
gnprice
gnprice reviewed Feb 4, 2025
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @apoorvapendse for building this! Sorry this PR fell through the cracks.

One high-level comment below. Then see chat on what I think would be a good strategy for the solution, if you'd like to take that forward.

lib/model/unreads.dart Outdated
Comment on lines 99 to 100
// Maps lowercase topic names for lookup to one of the variants of the topic (case preserving).
final Map<String, TopicName> topicMapper;
Copy link
Member

Choose a reason for hiding this comment

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

This won't have the right behavior because this map is shared between channels. So if there's one conversation at #islands > Ios and another at #mobile > iOS, then one or the other would get wrongly rendered with the other's casing.

@gnprice
Copy link
Member

gnprice commented Feb 4, 2025

Sorry this PR fell through the cracks.

Ah, I guess it was marked as draft until yesterday — so that at least explains why we didn't review it sooner 🙂. Anyway, thanks for sending it and for the update yesterday (and in chat).

@apoorvapendse
Copy link
Contributor Author

Yes, it was updated yesterday (had conflicts since the TopicName introduction).

@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch 4 times, most recently from 07f29ad to 8eafd1d Compare February 8, 2025 17:30
@gnprice
Copy link
Member

gnprice commented Feb 13, 2025

Thanks for the revision! I see you mentioned in chat that this is ready for review again.

This looks like broadly the right structure. A few high-level comments:

  • Let's put TopicMap in lib/model/channel.dart. Topics live within channels, and that file is already the home of other data about topics (namely the visibility policies).
  • Let's have a good set of unit tests for TopicMap itself. I mentioned "a quick test case or two in unreads_test.dart", but that was in the context of what comes on top of already having TopicMap 🙂
  • Do we need all those methods on TopicMap? If there are fewer, that's less to test.
  • The names and signatures of the methods should match Map. So e.g. a wrapper for Map.remove should be called remove, not delete. That helps make it easy to remember and find the name of each method.

@gnprice
Copy link
Member

gnprice commented Feb 13, 2025

After a revision addressing that high-level feedback, this PR can go to maintainer review to get more detailed feedback.

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Feb 13, 2025
@chrisbobbe chrisbobbe self-assigned this Feb 13, 2025
@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch from 8eafd1d to 74ed495 Compare February 14, 2025 17:56
@apoorvapendse
Copy link
Contributor Author

Thanks for the review @gnprice,
I eliminated a couple methods and renamed a few.
Added tests for the TopicMap class and moved it to lib/model/channel.dart.

@apoorvapendse
unreads: Fix topic case-sensitivity.
6f6c85d 
Maps canoncalized topic names to
a variant of the actual topic name.

This is achieved by introducing a TopicMap class
similar to FoldDict in web.

I referred to the FoldDict code in
zulip/zulip as @gnprice suggested.
Thanks to the folks who wrote it!

See https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Expected.20behaviour.20for.20case.20insensitivity.20for.20unreads.20.23F980/near/2076806

Fixes zulip#980.
@apoorvapendse apoorvapendse force-pushed the fix-unread-topic-case-sensitivity branch from 74ed495 to 6f6c85d Compare February 14, 2025 18:09
@gnprice
Copy link
Member

gnprice commented Feb 27, 2025

Thanks for your work on this. It feels like there's still significant changes needed before we'd want to merge this; so in the interest of getting the issue resolved soon, I've asked @chrisbobbe to take over writing an implementation.

For a brief high-level picture of some of the things that still need revision:

  • There are still several examples of this comment, from above:

    • The names and signatures of the methods should match Map. So e.g. a wrapper for Map.remove should be called remove, not delete. That helps make it easy to remember and find the name of each method.

    In addition to keeping it easy to remember and find the names, this would simplify the diff.

  • The implementation can be simplified by using the stdlib's handy MapBase mixin. That also makes toMap unnecessary and simplifies tests.

  • The comment in _addAllInStreamTopic shouldn't get deleted by this change, since it isn't related to what the comment is about.

@gnprice gnprice closed this Feb 27, 2025
@apoorvapendse apoorvapendse deleted the fix-unread-topic-case-sensitivity branch February 27, 2025 02:27
@gnprice gnprice mentioned this pull request Apr 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees

@chrisbobbe chrisbobbe

Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Case-insensitive topics in unreads data
3 participants
@apoorvapendse @gnprice @chrisbobbe