Make topic headers case-insensitive #1106

shivanshsharma13 · 2024-12-05T18:12:03Z

Topics in Zulip are case-insensitive. This change makes the message list's topic headers match that behavior, so messages whose topics differ only in case (like "missing string" and "Missing string") share a single header. This brings the behavior in line with Zulip web.

What is the change?

The change modifies the topic comparison logic in haveSameRecipient() to use case-insensitive comparison when determining whether to show a new recipient header.

Screenshots

Before	After

Fixes #739

chrisbobbe · 2024-12-05T22:04:48Z

Hi, thanks for the PR! Please revise the commit message to follow our style (we have a useful tip that will help you see examples of mergeable commit messages in the project's history). Also, please write a test for this bugfix; that should go in the same commit that adds the code being tested.

shivanshsharma13 · 2024-12-06T19:54:14Z

Implemented the requested changes. Please let me know if there’s anything else that needs adjustment. Thanks

chrisbobbe

Thanks!

For the commit message:

Message_list: Make topic headers case-insensitive

This change in the commit makes the message list's topic headers match that behavior, so messages whose topics differ only in case (like "missing string" and "Missing string") share a single header.  This brings the behavior in line with Zulip web. (Added the relevant test case)

We use the prefix msglist: for commits about the message list; see examples using the tip I shared earlier for browsing history.

Also let's make the summary line more explicit about the bug it fixes. How about:

msglist: Remove extra recipient header when topic changes case

Also your commit is missing a Fixes: line and isn't word-wrapped properly.

chrisbobbe · 2024-12-06T21:25:04Z

test/model/message_list_test.dart

@@ -1703,6 +1703,23 @@ void main() {
        }
      }
    });
+    test('stream messages match with case-insensitive topics', () {


nit: put this new test just under the test 'stream messages match just if same stream/topic'

Also here's a version that has different tests for each case (instead of multiple checks in the same test), along with a descriptive label for each test, which will appear in output when it fails. This also tests some additional cases that are interesting:

group('topics compared case-insensitively', () { void doTest(String description, String topicA, String topicB, bool expected) { test(description, () { final stream = eg.stream(); final messageA = eg.streamMessage(stream: stream, topic: topicA); final messageB = eg.streamMessage(stream: stream, topic: topicB); check(haveSameRecipient(messageA, messageB)).equals(expected); }); } doTest('same case, all lower', 'abc', 'abc', true); doTest('same case, all upper', 'ABC', 'ABC', true); doTest('same case, mixed', 'AbC', 'AbC', true); doTest('same non-letter chars', '嗎', '嗎', true); doTest('different case', 'aBc', 'ABC', true); doTest('different case, same diacritics', 'AbÇ', 'aBç', true); doTest('same letters, different diacritics', 'ma', 'mǎ', false); doTest('having different non-letter chars', 'ma 嗎', 'mǎ 馬', false); });

Yes, thank you for the help! This test looks much more promising and thoroughly checks each case. I'll replace my current implementation with this one and ensure to take care of such details from next time onward.

chrisbobbe · 2024-12-09T19:20:29Z

Thanks! Please wrap the commit-message body to 68 characters and format the "Fixes" line the same way we do in other commits (see the project's history for examples).

shivanshsharma13 · 2024-12-11T04:45:38Z

Thanks! Please wrap the commit-message body to 68 characters and format the "Fixes" line the same way we do in other commits (see the project's history for examples).

Done, Please review.

Wrapped the cotent in 68 chars a line
Used Fixes: format as done on other commits (sorry for overlooking previously)

chrisbobbe · 2024-12-11T21:00:45Z

Thanks! Marking for Greg's review.

gnprice

Thanks @shivanshsharma13, and thanks @chrisbobbe for the previous reviews!

Generally this looks good; comments below.

gnprice · 2024-12-17T21:38:18Z

test/model/message_list_test.dart

  });
+  group('topics compared case-insensitively', () {


nit: separate with blank line

gnprice · 2024-12-17T21:38:49Z

test/model/message_list_test.dart

  });
+  group('topics compared case-insensitively', () {


nit: nest this inside the haveSameRecipient group

gnprice · 2024-12-17T21:44:28Z

test/model/message_list_test.dart

+    doTest('same case, all lower',               'abc',  'abc',  true);
+    doTest('same case, all upper',               'ABC',  'ABC',  true);
+    doTest('same case, mixed',                   'AbC',  'AbC',  true);
+    doTest('same non-letter chars',              '嗎',    '嗎',    true);


Suggested change

doTest('same non-letter chars', '嗎', '嗎', true);

doTest('same non-cased chars', '嗎', '嗎', true);

Calling this character "non-letter" is confusing, because Unicode in fact classifies it in its "Letter" category, specifically "Lo (Letter, Other)":

$ unicode --long 嗎 U+55CE CJK UNIFIED IDEOGRAPH-55CE UTF-8: e5 97 8e UTF-16BE: 55ce Decimal: 嗎 Octal: \052716 嗎 Category: Lo (Letter, Other); East Asian width: W (wide) Unicode block: 4E00..9FFF; CJK Unified Ideographs Bidi: L (Left-to-Right)

(By comparison, cased letters like a and A are in categories "Ll (Letter, Lowercase)" and "Lu (Letter, Uppercase)" respectively.)

gnprice · 2024-12-17T21:47:58Z

test/model/message_list_test.dart

+    doTest('same letters, different diacritics', 'ma',   'mǎ',   false);
+    doTest('having different non-letter chars',  'ma 嗎', 'mǎ 馬', false);


Suggested change

doTest('same letters, different diacritics', 'ma', 'mǎ', false);

doTest('having different non-letter chars', 'ma 嗎', 'mǎ 馬', false);

doTest('same letters, different diacritics', 'ma', 'mǎ', false);

doTest('having different non-letter chars', '嗎', '馬', false);

(plus the renaming above)

Otherwise the last test doesn't really exercise the comparison of the two sinographs — the two names are already going to be different because of the "a" vs "ǎ".

shivanshsharma13 · 2024-12-18T17:59:58Z

Thanks for the detailed review. I acknowledged the suggested changes. Please let me know if anything needs to be changed!🙂

gnprice

Thanks for the revision! All looks good except one nit below.

(Reviews are a bit slow this week and next, due to holidays.)

gnprice · 2024-12-27T00:06:40Z

test/model/message_list_test.dart

    });
+    group('topics compared case-insensitively', () {


bump #1106 (comment)

Oh, sorry for overlooking that. Done now!

shivanshsharma13 · 2024-12-27T20:08:52Z

Thanks! Totally understandable with the holiday season—Christmas and New Year can be a busy time. Wishing you happy holidays.

This change makes the message list's topic headers case-insensitive Messages with topics differing only in case (e.g., "missing string" and "Missing string") will now share a single header. This aligns the behavior with Zulip web and ensures a consistent user experience. Fixes: zulip#739

gnprice · 2025-01-02T20:37:09Z

Looks good — merged. Thanks again @shivanshsharma13 for the contribution!

shivanshsharma13 force-pushed the case-insensitive_header branch from a474a94 to 39ee766 Compare December 6, 2024 19:48

shivanshsharma13 force-pushed the case-insensitive_header branch from 4947842 to 4b7c01b Compare December 6, 2024 20:19

chrisbobbe reviewed Dec 6, 2024

View reviewed changes

shivanshsharma13 force-pushed the case-insensitive_header branch from 4b7c01b to 12c7969 Compare December 7, 2024 08:40

shivanshsharma13 force-pushed the case-insensitive_header branch from 12c7969 to f4fc7ff Compare December 11, 2024 04:39

chrisbobbe requested a review from gnprice December 11, 2024 21:00

chrisbobbe assigned gnprice Dec 11, 2024

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Dec 11, 2024

gnprice reviewed Dec 17, 2024

View reviewed changes

shivanshsharma13 force-pushed the case-insensitive_header branch from f4fc7ff to 712e75f Compare December 18, 2024 17:56

shivanshsharma13 force-pushed the case-insensitive_header branch from 712e75f to f2abecc Compare December 18, 2024 18:42

shivanshsharma13 requested a review from gnprice December 26, 2024 08:51

gnprice reviewed Dec 27, 2024

View reviewed changes

shivanshsharma13 force-pushed the case-insensitive_header branch from f2abecc to 76515ca Compare December 27, 2024 20:06

shivanshsharma13 requested a review from gnprice December 27, 2024 20:07

gnprice force-pushed the case-insensitive_header branch from 76515ca to fcbbc9d Compare January 2, 2025 20:37

gnprice merged commit fcbbc9d into zulip:main Jan 2, 2025

shivanshsharma13 deleted the case-insensitive_header branch January 3, 2025 03:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make topic headers case-insensitive #1106

Make topic headers case-insensitive #1106

shivanshsharma13 commented Dec 5, 2024 •

edited

Loading

chrisbobbe commented Dec 5, 2024 •

edited

Loading

shivanshsharma13 commented Dec 6, 2024

chrisbobbe left a comment

chrisbobbe Dec 6, 2024

chrisbobbe Dec 6, 2024

shivanshsharma13 Dec 7, 2024 •

edited

Loading

chrisbobbe commented Dec 9, 2024

shivanshsharma13 commented Dec 11, 2024 •

edited

Loading

chrisbobbe commented Dec 11, 2024

gnprice left a comment

gnprice Dec 17, 2024

gnprice Dec 17, 2024

gnprice Dec 17, 2024

gnprice Dec 17, 2024

shivanshsharma13 commented Dec 18, 2024

gnprice left a comment

gnprice Dec 27, 2024

shivanshsharma13 Dec 27, 2024

shivanshsharma13 commented Dec 27, 2024

gnprice commented Jan 2, 2025

	doTest('same non-letter chars', '嗎', '嗎', true);
	doTest('same non-cased chars', '嗎', '嗎', true);

		doTest('same letters, different diacritics', 'ma', 'mǎ', false);
		doTest('having different non-letter chars', 'ma 嗎', 'mǎ 馬', false);

Make topic headers case-insensitive #1106

Make topic headers case-insensitive #1106

Conversation

shivanshsharma13 commented Dec 5, 2024 • edited Loading

What is the change?

Screenshots

chrisbobbe commented Dec 5, 2024 • edited Loading

shivanshsharma13 commented Dec 6, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Dec 9, 2024

shivanshsharma13 commented Dec 11, 2024 • edited Loading

chrisbobbe commented Dec 11, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 18, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 27, 2024

gnprice commented Jan 2, 2025

shivanshsharma13 commented Dec 5, 2024 •

edited

Loading

chrisbobbe commented Dec 5, 2024 •

edited

Loading

shivanshsharma13 Dec 7, 2024 •

edited

Loading

shivanshsharma13 commented Dec 11, 2024 •

edited

Loading