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.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 48 additions & 0 deletions lib/model/channel.dart
Original file line number Diff line number Diff line change
Expand Up @@ -361,3 +361,51 @@ class ChannelStoreImpl with ChannelStore {
}
}
}

class TopicMap<T> {
final Map<String, _KeyValue<T>> _topicMessageMap = {};

int get size => _topicMessageMap.length;

T? get(TopicName key) {
return _topicMessageMap[_munge(key)]?.v;
}

void set(TopicName key, T value) {
_topicMessageMap[_munge(key)] = _KeyValue(k: key, v: value);
}

bool containsKey(TopicName key) => _topicMessageMap.containsKey(_munge(key));

bool remove(TopicName key) => _topicMessageMap.remove(_munge(key)) != null;

Iterable<TopicName> keys() => _topicMessageMap.values.map((kv) => kv.k);

Iterable<T> values() => _topicMessageMap.values.map((kv) => kv.v);

Iterable<MapEntry<TopicName, T>> get entries =>
_topicMessageMap.entries.map((kv) => MapEntry(kv.value.k, kv.value.v));

void clear() => _topicMessageMap.clear();

String _munge(TopicName key) => key.toString().toLowerCase();
}

extension TopicMapExtensions<T> on TopicMap<T> {
// Converts this `TopicMap<T>` into a plain `Map<TopicName, T>`,
// making it easier to do standard deep equality checks in tests.
Map<TopicName, T> toMap() {
final result = <TopicName, T>{};
for (final entry in entries) {
result[entry.key] = entry.value;
}
return result;
}
}

class _KeyValue<T> {
final TopicName k;
final T v;

_KeyValue({required this.k, required this.v});
}
40 changes: 23 additions & 17 deletions lib/model/unreads.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ class Unreads extends ChangeNotifier {
required int selfUserId,
required ChannelStore channelStore,
}) {
final streams = <int, Map<TopicName, QueueList<int>>>{};
final streams = <int, TopicMap<QueueList<int>>>{};
final dms = <DmNarrow, QueueList<int>>{};
final mentions = Set.of(initial.mentions);

for (final unreadChannelSnapshot in initial.channels) {
final streamId = unreadChannelSnapshot.streamId;
final topic = unreadChannelSnapshot.topic;
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
(streams[streamId] ??= TopicMap<QueueList<int>>()).set(topic,QueueList.from(unreadChannelSnapshot.unreadMessageIds));
}

for (final unreadDmSnapshot in initial.dms) {
Expand Down Expand Up @@ -86,7 +86,7 @@ class Unreads extends ChangeNotifier {
// int count;

/// Unread stream messages, as: stream ID → topic → message IDs (sorted).
final Map<int, Map<TopicName, QueueList<int>>> streams;
final Map<int, TopicMap<QueueList<int>>> streams;

/// Unread DM messages, as: DM narrow → message IDs (sorted).
final Map<DmNarrow, QueueList<int>> dms;
Expand Down Expand Up @@ -187,7 +187,9 @@ class Unreads extends ChangeNotifier {

int countInTopicNarrow(int streamId, TopicName topic) {
final topics = streams[streamId];
return topics?[topic]?.length ?? 0;
if (topics == null) return 0;
final qlist = topics.get(topic);
return qlist?.length ?? 0;
}

int countInDmNarrow(DmNarrow narrow) => dms[narrow]?.length ?? 0;
Expand Down Expand Up @@ -443,26 +445,30 @@ class Unreads extends ChangeNotifier {
// TODO use efficient lookups
bool _slowIsPresentInStreams(int messageId) {
return streams.values.any(
(topics) => topics.values.any(
(topics) => topics.values().any(
(messageIds) => messageIds.contains(messageId),
),
);
}

void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) {
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
if ((streams[streamId] ??= TopicMap<QueueList<int>>()).get(topic) == null) {
streams[streamId]?.set(topic, QueueList<int>());
}
(streams[streamId] ??= TopicMap<QueueList<int>>()).get(topic)?.addLast(messageId);
}

// [messageIds] must be sorted ascending and without duplicates.
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, TopicName topic) {
final topics = streams[streamId] ??= {};
topics.update(topic,
ifAbsent: () => messageIds,
// setUnion dedupes existing and incoming unread IDs,
// so we tolerate zulip/zulip#22164, fixed in 6.0
// TODO(server-6) remove 6.0 comment
(existing) => setUnion(existing, messageIds),
);
final topics = streams[streamId] ??= TopicMap<QueueList<int>>();
if (topics.containsKey(topic)) {
// If the topic already exists, update its message ids with setUnion
topics.set(topic, setUnion(topics.get(topic) ?? QueueList<int>(), messageIds));
} else {
// If the topic does not exist, insert the new messageIds list
topics.set(topic, messageIds);
}

}

// TODO use efficient model lookups
Expand All @@ -479,7 +485,7 @@ class Unreads extends ChangeNotifier {
for (final topic in newlyEmptyTopics) {
topics.remove(topic);
}
if (topics.isEmpty) {
if (topics.size==0) {
newlyEmptyStreams.add(streamId);
}
}
Expand All @@ -491,14 +497,14 @@ class Unreads extends ChangeNotifier {
void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, TopicName topic) {
final topics = streams[streamId];
if (topics == null) return;
final messageIds = topics[topic];
final messageIds = topics.get(topic);
if (messageIds == null) return;

// ([QueueList] doesn't have a `removeAll`)
messageIds.removeWhere((id) => incomingMessageIds.contains(id));
if (messageIds.isEmpty) {
topics.remove(topic);
if (topics.isEmpty) {
if (topics.size==0) {
streams.remove(streamId);
}
}
Expand Down
20 changes: 10 additions & 10 deletions pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -291,10 +291,10 @@ packages:
dependency: "direct dev"
description:
name: fake_async
sha256: "6a95e56b2449df2273fd8c45a662d6947ce1ebb7aafe80e550a3f68297f3cacc"
sha256: "5368f224a74523e8d2e7399ea1638b37aecfca824a3cc4dfdf77bf1fa905ac44"
url: "https://pub.dev"
source: hosted
version: "1.3.2"
version: "1.3.3"
ffi:
dependency: transitive
description:
Expand Down Expand Up @@ -607,10 +607,10 @@ packages:
dependency: "direct main"
description:
name: intl
sha256: d6f56758b7d3014a48af9701c085700aac781a92a87a62b1333b46d8879661cf
sha256: "3df61194eb431efc39c4ceba583b95633a403f46c9fd341e550ce0bfa50e9aa5"
url: "https://pub.dev"
source: hosted
version: "0.19.0"
version: "0.20.2"
io:
dependency: transitive
description:
Expand Down Expand Up @@ -647,10 +647,10 @@ packages:
dependency: transitive
description:
name: leak_tracker
sha256: c35baad643ba394b40aac41080300150a4f08fd0fd6a10378f8f7c6bc161acec
sha256: "6bb818ecbdffe216e81182c2f0714a2e62b593f4a4f13098713ff1685dfb6ab0"
url: "https://pub.dev"
source: hosted
version: "10.0.8"
version: "10.0.9"
leak_tracker_flutter_testing:
dependency: transitive
description:
Expand Down Expand Up @@ -1244,10 +1244,10 @@ packages:
dependency: transitive
description:
name: vm_service
sha256: "0968250880a6c5fe7edc067ed0a13d4bae1577fe2771dcf3010d52c4a9d3ca14"
sha256: ddfa8d30d89985b96407efce8acbdd124701f96741f2d981ca860662f1c0dc02
url: "https://pub.dev"
source: hosted
version: "14.3.1"
version: "15.0.0"
wakelock_plus:
dependency: "direct main"
description:
Expand Down Expand Up @@ -1300,10 +1300,10 @@ packages:
dependency: transitive
description:
name: webdriver
sha256: "3d773670966f02a646319410766d3b5e1037efb7f07cc68f844d5e06cd4d61c8"
sha256: "2f3a14ca026957870cfd9c635b83507e0e51d8091568e90129fbf805aba7cade"
url: "https://pub.dev"
source: hosted
version: "3.0.4"
version: "3.1.0"
webkit_inspection_protocol:
dependency: transitive
description:
Expand Down
116 changes: 116 additions & 0 deletions test/model/channel_test.dart
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@

import 'package:checks/checks.dart';
import 'package:collection/collection.dart';
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/initial_snapshot.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/model/algorithms.dart';
import 'package:zulip/model/store.dart';
import 'package:zulip/model/channel.dart';

Expand Down Expand Up @@ -394,4 +396,118 @@ void main() {
.equals(UserTopicVisibilityPolicy.none);
});
});
group('TopicMap with QueueList<int>', () {
test('initially empty', () {
final topicMap = TopicMap<QueueList<int>>();
check(topicMap.size).equals(0);
check(topicMap.keys()).isEmpty();
check(topicMap.values()).isEmpty();
check(topicMap.entries).isEmpty();
check(topicMap.toMap()).deepEquals({});
});

test('set and get values', () {
final topicMap = TopicMap<QueueList<int>>();
final topic = TopicName('Topic1');
final value = QueueList<int>();
value.addAll([1, 2, 3]);
topicMap.set(topic, value);

check(topicMap.size).equals(1);
check(topicMap.get(topic)).equals(value);
});

test('containsKey and remove', () {
final topicMap = TopicMap<QueueList<int>>();
final topic = TopicName('Math');
final value = QueueList<int>();
value.addAll([3, 1, 4]);
topicMap.set(topic, value);

check(topicMap.containsKey(topic)).isTrue();
// Removing should return true.
check(topicMap.remove(topic)).isTrue();
check(topicMap.containsKey(topic)).isFalse();
check(topicMap.size).equals(0);
// Removing a non-existent key returns false.
check(topicMap.remove(topic)).isFalse();
});

test('keys, values, and entries', () {
final topicMap = TopicMap<QueueList<int>>();
final topicA = TopicName('A');
final topicB = TopicName('B');
final valueA = QueueList<int>();
valueA.addAll([10, 20]);
final valueB = QueueList<int>();
valueB.addAll([30,40]);
topicMap.set(topicA, valueA);
topicMap.set(topicB, valueB);

final keys = topicMap.keys().toList();
check(keys.length).equals(2);
check(keys).contains(topicA);
check(keys).contains(topicB);

final values = topicMap.values().toList();
check(values.length).equals(2);
check(values).contains(valueA);
check(values).contains(valueB);

final entries = topicMap.entries.toList();
check(entries.length).equals(2);
for (final entry in entries) {
if (entry.key == topicA) {
check(entry.value).equals(valueA);
} else if (entry.key == topicB) {
check(entry.value).equals(valueB);
}
}
});

test('clear and toMap', () {
final topicMap = TopicMap<QueueList<int>>();
final topic1 = TopicName('One');
final topic2 = TopicName('Two');
final value1 = QueueList<int>();
final value2 = QueueList<int>();
value1.add(1);
value2.add(2);
topicMap.set(topic1, value1);
topicMap.set(topic2, value2);

check(topicMap.size).equals(2);
check(topicMap.toMap()).deepEquals({
topic1: value1,
topic2: value2,
});

topicMap.clear();
check(topicMap.size).equals(0);
check(topicMap.toMap()).deepEquals({});
});

test('key normalization is case-insensitive', () {
final topicMap = TopicMap<QueueList<int>>();
// Using two TopicName instances that differ only by case.
final topicUpper = TopicName('HELLO');
final topicLower = TopicName('hello');
final valueLower = QueueList<int>();
final valueUpper = QueueList<int>();
valueUpper.add(42);
valueLower.add(32);
topicMap.set(topicLower, valueLower);
topicMap.set(topicUpper, setUnion(topicMap.get(topicLower) as Iterable<int>, valueUpper));
// Since _munge lowercases the key, using a different case should return the same value.
final list1 = topicMap.get(topicUpper);
final list2 = setUnion(valueUpper, valueLower);
check(list1?.length).equals(list2.length);
for ( int i = 0; i < list1!.length; i++ ) {
check(list1[i]).equals(list2[i]);
}

check(topicMap.toMap().containsKey(TopicName("HELLO"))).isTrue();
check(topicMap.toMap().containsKey(TopicName("hello"))).isFalse();
});
});
}
10 changes: 9 additions & 1 deletion test/model/unreads_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,17 @@ import 'package:collection/collection.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/unreads.dart';
import 'package:zulip/model/channel.dart';

extension UnreadsChecks on Subject<Unreads> {
Subject<Map<int, Map<TopicName, QueueList<int>>>> get streams => has((u) => u.streams, 'streams');
Subject<Map<int, Map<TopicName, QueueList<int>>>> get streams => has(
(unreads) {
return unreads.streams.map(
(streamId, topicMap) => MapEntry(streamId, topicMap.toMap()),
);
},
'streams',
);
Subject<Map<DmNarrow, QueueList<int>>> get dms => has((u) => u.dms, 'dms');
Subject<Set<int>> get mentions => has((u) => u.mentions, 'mentions');
Subject<bool> get oldUnreadsMissing => has((u) => u.oldUnreadsMissing, 'oldUnreadsMissing');
Expand Down
Loading