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.

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

Make topic headers case-insensitive #1106

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

Make topic headers case-insensitive #1106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ mixin _MessageSequence {
bool haveSameRecipient(Message prevMessage, Message message) {
if (prevMessage is StreamMessage && message is StreamMessage) {
if (prevMessage.streamId != message.streamId) return false;
if (prevMessage.topic != message.topic) return false;
if (prevMessage.topic.toLowerCase() != message.topic.toLowerCase()) return false;
} else if (prevMessage is DmMessage && message is DmMessage) {
if (!_equalIdSequences(prevMessage.allRecipientIds, message.allRecipientIds)) {
return false;
Expand Down
19 changes: 19 additions & 0 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,6 +1704,25 @@ void main() {
}
});
});
group('topics compared case-insensitively', () {
Comment on lines 1706 to +1707
Copy link
Member

Choose a reason for hiding this comment

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

nit: separate with blank line

Comment on lines 1706 to +1707
Copy link
Member

Choose a reason for hiding this comment

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

nit: nest this inside the haveSameRecipient group

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);
Copy link
Member

Choose a reason for hiding this comment

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

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.)

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);
Comment on lines +1723 to +1724
Copy link
Member

Choose a reason for hiding this comment

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

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 "ǎ".

});

test('messagesSameDay', () {
// These timestamps will differ depending on the timezone of the
Expand Down