Use isSameAs for topic comparison, where appropriate
#2090
Conversation
sm-sayedi
force-pushed
the
2088-accurate-topic-comparison
branch
2 times, most recently
from
ec57b6f to
3f1ae13
Compare
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
test/model/message_list_test.dart
Outdated
| StreamDestination(stream.streamId, eg.t(topic)), | ||
| StreamDestination(stream.streamId, eg.t(topic.toUpperCase())), |
There was a problem hiding this comment.
If topic and topic.toUpperCase() happened to be identical, then we'd lose coverage of the case-insensitivity check, which means a reader has to check and make sure topic won't be something like "TOPIC" or "" or "中文" or something. How about using literal strings, inline, to remove this need for nonlocal reasoning? Like for example "abc" and "ABC".
| // checkNotNotified(); | ||
| checkNotifiedOnce(); // unrelated; from notifyListenersIfAnyMessagePresent |
There was a problem hiding this comment.
I'm confused; what's the expected and desired behavior about notifying listeners here?
There was a problem hiding this comment.
The expected behavior here is that the message move itself in this case shouldn't cause notifying listeners, because messages are moved from "topic" to "ToPiC" in the same channel, which means they're still in the same narrow and in the same position compared to other messages in the narrow. But here, the notification comes from notifyListenersIfAnyMessagePresent.
This test is about stream/channel narrow and compares parts of the test behavior with the combined feed, not the stream/channel narrow itself.
Previously, when a UserTopicEvent muted a topic, only messages whose topic matched the event topic exactly (case-sensitive) were removed. Now, messages from all topics whose casing is different from the event topic, are removed.
Prior to this, when focusing on a topic narrow (e.g. for topic "t"),
message moves would be handled in the following way:
- Messages moved from narrow: If messages were moved from topic "T",
they wouldn't be removed from the current message list.
- Messages moved into narrow: If messages were moved into topic "T",
they wouldn't show up in the current message list.
- Messages moved inside narrow:
- If messages were moved from topic "t" into topic "T", they would
be removed from the current message list.
- If messages were moved into topic "t" from topic "T", there
would be an unnecessary refetch.
Prior to this, when focusing on a topic narrow (e.g. for topic "t"), opening a notification for a message of topic "T" would open a new page on top of the current one for the same narrow. Now, it's deduped. Fixes: zulip#2088
sm-sayedi
force-pushed
the
2088-accurate-topic-comparison
branch
from
3f1ae13 to
5005d31
Compare
|
Thanks for the review. Pushed changes, PTAL. |
2 participants
Fixes: #2088
Searched for the occurrences using
topic (!|=)=regex.Notable commits
b35168d msglist: Use
isSameAsfor topic comparison in UserTopicEvent handlingPreviously, when a UserTopicEvent muted a topic, only messages whose
topic matched the event topic exactly (case-sensitive) were removed.
Now, messages from all topics whose casing is different from the event
topic, are removed.
28ecbb2 msglist: Use
isSameAsfor topic comparison in message-move handlingPrior to this, when focusing on a topic narrow (e.g. for topic "t"),
message moves would be handled in the following way:
they wouldn't be removed from the current message list.
they wouldn't show up in the current message list.
be removed from the current message list.
would be an unnecessary refetch.
ec57b6f narrow: Use
isSameAsfor topic comparison in TopicNarrow.==Prior to this, when focusing on a topic narrow (e.g. for topic "t"),
opening a notification for a message of topic "T" would open a new page
on top of the current one for the same narrow. Now, it's deduped.