Skip to content

Remove unknown from UserTopicVisibilityPolicy #2095

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

Open
Cairo09 wants to merge 1 commit into
base: main
Choose a base branch
from
+217 −80
Open

Remove unknown from UserTopicVisibilityPolicy #2095

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
3 changes: 2 additions & 1 deletion lib/api/model/events.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,7 +1051,8 @@ class UserTopicEvent extends Event {
final int streamId;
final TopicName topicName;
final int lastUpdated;
final UserTopicVisibilityPolicy visibilityPolicy;
@JsonKey(unknownEnumValue: JsonKey.nullForUndefinedEnumValue)
final UserTopicVisibilityPolicy? visibilityPolicy;

UserTopicEvent({
required super.id,
Expand Down
4 changes: 2 additions & 2 deletions lib/api/model/events.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/api/model/initial_snapshot.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ class UserTopicItem {
final int streamId;
final TopicName topicName;
final int lastUpdated;
@JsonKey(unknownEnumValue: UserTopicVisibilityPolicy.unknown)
final UserTopicVisibilityPolicy visibilityPolicy;
@JsonKey(unknownEnumValue: JsonKey.nullForUndefinedEnumValue)
final UserTopicVisibilityPolicy? visibilityPolicy;

UserTopicItem({
required this.streamId,
Expand Down
5 changes: 2 additions & 3 deletions lib/api/model/initial_snapshot.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/api/model/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,8 +899,8 @@ enum UserTopicVisibilityPolicy {
none(apiValue: 0),
muted(apiValue: 1),
unmuted(apiValue: 2), // TODO(server-7) newly added
followed(apiValue: 3), // TODO(server-8) newly added
unknown(apiValue: null); // TODO(#1074) remove this
followed(apiValue: 3); // TODO(server-8) newly added
//Removed unknown (#1074)

const UserTopicVisibilityPolicy({required this.apiValue});

Expand Down
1 change: 0 additions & 1 deletion lib/api/route/channels.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ Future<void> updateUserTopic(ApiConnection connection, {
required TopicName topic,
required UserTopicVisibilityPolicy visibilityPolicy,
}) {
assert(visibilityPolicy != UserTopicVisibilityPolicy.unknown);
assert(connection.zulipFeatureLevel! >= 170);
return connection.post('updateUserTopic', (_) {}, 'user_topics', {
'stream_id': streamId,
Expand Down
63 changes: 38 additions & 25 deletions lib/model/channel.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ mixin ChannelStore on UserStore {
/// All the channel folders, including archived ones, indexed by ID.
Map<int, ChannelFolder> get channelFolders;

static bool _warnInvalidVisibilityPolicy(
UserTopicVisibilityPolicy? visibilityPolicy
) {
if (visibilityPolicy == null) {
// Not a value we expect. Keep it out of our data structures. // TODO(log)
return true;
}
return false;
}

static int compareChannelsByName(ZulipStream a, ZulipStream b) {
// A user gave feedback wanting zulip-flutter to match web in putting
// emoji-prefixed channels first; see #1202.
Expand Down Expand Up @@ -118,9 +128,14 @@ mixin ChannelStore on UserStore {
UserTopicVisibilityEffect willChangeIfTopicVisibleInStream(UserTopicEvent event) {
final streamId = event.streamId;
final topic = event.topicName;
final UserTopicVisibilityPolicy? visibilityPolicy = event.visibilityPolicy;
if (ChannelStore._warnInvalidVisibilityPolicy(visibilityPolicy)) {
return UserTopicVisibilityEffect.none;
}
final UserTopicVisibilityPolicy policy = visibilityPolicy!;
return UserTopicVisibilityEffect._fromBeforeAfter(
_isTopicVisibleInStream(topicVisibilityPolicy(streamId, topic)),
_isTopicVisibleInStream(event.visibilityPolicy));
_isTopicVisibleInStream(policy));
}

static bool _isTopicVisibleInStream(UserTopicVisibilityPolicy policy) {
Expand All @@ -132,9 +147,6 @@ mixin ChannelStore on UserStore {
case UserTopicVisibilityPolicy.unmuted:
case UserTopicVisibilityPolicy.followed:
return true;
case UserTopicVisibilityPolicy.unknown:
assert(false);
return true;
}
}

Expand All @@ -155,9 +167,14 @@ mixin ChannelStore on UserStore {
UserTopicVisibilityEffect willChangeIfTopicVisible(UserTopicEvent event) {
final streamId = event.streamId;
final topic = event.topicName;
final UserTopicVisibilityPolicy? visibilityPolicy = event.visibilityPolicy;
if (ChannelStore._warnInvalidVisibilityPolicy(visibilityPolicy)) {
return UserTopicVisibilityEffect.none;
}
final UserTopicVisibilityPolicy policy = visibilityPolicy!;
return UserTopicVisibilityEffect._fromBeforeAfter(
_isTopicVisible(streamId, topicVisibilityPolicy(streamId, topic)),
_isTopicVisible(streamId, event.visibilityPolicy));
_isTopicVisible(streamId, policy));
}

bool _isTopicVisible(int streamId, UserTopicVisibilityPolicy policy) {
Expand All @@ -173,9 +190,6 @@ mixin ChannelStore on UserStore {
case UserTopicVisibilityPolicy.unmuted:
case UserTopicVisibilityPolicy.followed:
return true;
case UserTopicVisibilityPolicy.unknown:
assert(false);
return true;
}
}

Expand Down Expand Up @@ -332,12 +346,13 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {

final topicVisibility = <int, TopicKeyedMap<UserTopicVisibilityPolicy>>{};
for (final item in initialSnapshot.userTopics) {
if (_warnInvalidVisibilityPolicy(item.visibilityPolicy)) {
// Not a value we expect. Keep it out of our data structures. // TODO(log)
final UserTopicVisibilityPolicy? visibilityPolicy = item.visibilityPolicy;
if (ChannelStore._warnInvalidVisibilityPolicy(visibilityPolicy)) {
continue;
}
final UserTopicVisibilityPolicy policy = visibilityPolicy!;
final forStream = topicVisibility.putIfAbsent(item.streamId, () => makeTopicKeyedMap());
forStream[item.topicName] = item.visibilityPolicy;
forStream[item.topicName] = policy;
}

return ChannelStoreImpl._(
Expand Down Expand Up @@ -378,13 +393,6 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {
return topicVisibility[streamId]?[topic] ?? UserTopicVisibilityPolicy.none;
}

static bool _warnInvalidVisibilityPolicy(UserTopicVisibilityPolicy visibilityPolicy) {
if (visibilityPolicy == UserTopicVisibilityPolicy.unknown) {
// Not a value we expect. Keep it out of our data structures. // TODO(log)
return true;
}
return false;
}

void handleChannelEvent(ChannelEvent event) {
switch (event) {
Expand Down Expand Up @@ -558,13 +566,18 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {
}

void handleUserTopicEvent(UserTopicEvent event) {
UserTopicVisibilityPolicy visibilityPolicy = event.visibilityPolicy;
if (_warnInvalidVisibilityPolicy(visibilityPolicy)) {
visibilityPolicy = UserTopicVisibilityPolicy.none;
final UserTopicVisibilityPolicy? visibilityPolicy = event.visibilityPolicy;
if (ChannelStore._warnInvalidVisibilityPolicy(visibilityPolicy)) {
final forStream = topicVisibility[event.streamId];
if (forStream == null) return;
forStream.remove(event.topicName);
if (forStream.isEmpty) {
topicVisibility.remove(event.streamId);
}
return;
}
if (visibilityPolicy == UserTopicVisibilityPolicy.none) {
// This is the "zero value" for this type, which our data structure
// represents by leaving the topic out entirely.
final UserTopicVisibilityPolicy policy = visibilityPolicy!;
if (policy == UserTopicVisibilityPolicy.none) {
final forStream = topicVisibility[event.streamId];
if (forStream == null) return;
forStream.remove(event.topicName);
Expand All @@ -573,7 +586,7 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {
}
} else {
final forStream = topicVisibility.putIfAbsent(event.streamId, () => makeTopicKeyedMap());
forStream[event.topicName] = visibilityPolicy;
forStream[event.topicName] = policy;
}
}
}
Expand Down
23 changes: 0 additions & 23 deletions lib/widgets/action_sheet.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,10 +725,6 @@ void showTopicActionSheet(BuildContext context, {
if (supportsFollowingTopics) {
visibilityOptions.add(UserTopicVisibilityPolicy.none);
}
case UserTopicVisibilityPolicy.unknown:
// TODO(#1074): This should be unreachable as we keep `unknown` out of
// our data structures.
assert(false);
}
} else {
// Channel is muted.
Expand All @@ -750,10 +746,6 @@ void showTopicActionSheet(BuildContext context, {
if (supportsFollowingTopics) {
visibilityOptions.add(UserTopicVisibilityPolicy.none);
}
case UserTopicVisibilityPolicy.unknown:
// TODO(#1074): This should be unreachable as we keep `unknown` out of
// our data structures.
assert(false);
}
}
}
Expand Down Expand Up @@ -824,11 +816,6 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton {
return ZulipIcons.unmute;
case UserTopicVisibilityPolicy.followed:
return ZulipIcons.follow;
case UserTopicVisibilityPolicy.unknown:
// TODO(#1074): This should be unreachable as we keep `unknown` out of
// our data structures.
assert(false);
return ZulipIcons.inherit;
}
}

Expand All @@ -855,11 +842,6 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton {
assert(false);
return '';

case (_, UserTopicVisibilityPolicy.unknown):
// This case is unreachable (or should be) because we keep `unknown` out
// of our data structures. We plan to remove the `unknown` case in #1074.
assert(false);
return '';
}
}

Expand All @@ -885,11 +867,6 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton {
assert(false);
return '';

case (_, UserTopicVisibilityPolicy.unknown):
// This case is unreachable (or should be) because we keep `unknown` out
// of our data structures. We plan to remove the `unknown` case in #1074.
assert(false);
return '';
}
}

Expand Down
5 changes: 0 additions & 5 deletions lib/widgets/icons.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,5 @@ IconData? iconDataForTopicVisibilityPolicy(UserTopicVisibilityPolicy policy) {
return ZulipIcons.follow;
case UserTopicVisibilityPolicy.none:
return null;
case UserTopicVisibilityPolicy.unknown:
// This case is unreachable (or should be) because we keep `unknown` out
// of our data structures. We plan to remove the `unknown` case in #1074.
assert(false);
return null;
}
}
3 changes: 0 additions & 3 deletions lib/widgets/topic_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,6 @@ class _TopicItem extends StatelessWidget {
case UserTopicVisibilityPolicy.unmuted:
case UserTopicVisibilityPolicy.followed:
opacity = 1;
case UserTopicVisibilityPolicy.unknown:
assert(false);
opacity = 1;
}

final visibilityIcon = iconDataForTopicVisibilityPolicy(visibilityPolicy);
Expand Down
7 changes: 7 additions & 0 deletions test/api/model/events_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,12 @@ extension HeartbeatEventChecks on Subject<HeartbeatEvent> {
// No properties not covered by Event.
}

extension UserTopicEventChecks on Subject<UserTopicEvent> {
Subject<int> get streamId => has((e) => e.streamId, 'streamId');
Subject<TopicName> get topicName => has((e) => e.topicName, 'topicName');
Subject<int> get lastUpdated => has((e) => e.lastUpdated, 'lastUpdated');
Subject<UserTopicVisibilityPolicy?> get visibilityPolicy => has((e) => e.visibilityPolicy, 'visibilityPolicy');
}

// Add more extensions here for more event types as needed.
// Keep them in the same order as the event types' own definitions.
35 changes: 35 additions & 0 deletions test/api/model/events_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,4 +331,39 @@ void main() {
})).recipientIds.isNotNull().deepEquals([1, 2, 4, 8, 10]);
});
});
group('user_topic: visibility_policy null handling', () {
final baseJson = {
'id': 1,
'type': 'user_topic',
'stream_id': 42,
'topic_name': 'test topic',
'last_updated': 1234567890,
};

test('known visibility_policy values deserialize correctly', () {
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': 0}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.none);
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': 1}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.muted);
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': 2}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.unmuted);
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': 3}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.followed);
});

test('unknown visibility_policy value becomes null', () {
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': 999}))
.visibilityPolicy.isNull();
});

test('missing visibility_policy field becomes null', () {
check(UserTopicEvent.fromJson(baseJson))
.visibilityPolicy.isNull();
});

test('explicit null visibility_policy remains null', () {
check(UserTopicEvent.fromJson({...baseJson, 'visibility_policy': null}))
.visibilityPolicy.isNull();
});
});
}
34 changes: 34 additions & 0 deletions test/api/model/initial_snapshot_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:zulip/api/model/model.dart';

import '../../example_data.dart' as eg;
import '../../stdlib_checks.dart';
import 'model_checks.dart';

void main() {
test('UnreadMessagesSnapshot from json recognizes channels as streams', () {
Expand Down Expand Up @@ -72,4 +73,37 @@ void main() {
check(settings.emojiset).equals(Emojiset.unknown);
}
});
group('UserTopicItem: visibility_policy unknown to null conversion', () {
final baseJson = {
'stream_id': 123,
'topic_name': 'test topic',
'last_updated': 1234567890,
};

test('known visibility_policy values deserialize correctly', () {
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': 0}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.none);
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': 1}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.muted);
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': 2}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.unmuted);
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': 3}))
.visibilityPolicy.equals(UserTopicVisibilityPolicy.followed);
});

test('unknown visibility_policy value becomes null', () {
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': 999}))
.visibilityPolicy.isNull();
});

test('missing visibility_policy field becomes null', () {
check(UserTopicItem.fromJson(baseJson))
.visibilityPolicy.isNull();
});

test('explicit null visibility_policy remains null', () {
check(UserTopicItem.fromJson({...baseJson, 'visibility_policy': null}))
.visibilityPolicy.isNull();
});
});
}
Loading