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.

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

Mark as resolved/unresolved prep 1 #1242

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
9 changes: 1 addition & 8 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:json_annotation/json_annotation.dart';

import '../../model/algorithms.dart';
import '../route/messages.dart';
import 'json.dart';
import 'model.dart';
import 'submessage.dart';
Expand Down Expand Up @@ -766,14 +767,6 @@ class UpdateMessageEvent extends Event {
Map<String, dynamic> toJson() => _$UpdateMessageEventToJson(this);
}

/// As in [UpdateMessageEvent.propagateMode].
@JsonEnum(fieldRename: FieldRename.snake)
enum PropagateMode {
Comment on lines -769 to -771
Copy link
Member

Choose a reason for hiding this comment

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

And this can move from lib/api/model/events.dart to lib/api/model/model.dart, since it's no longer specific to an event.

changeOne,
changeLater,
changeAll;
}

/// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message
@JsonSerializable(fieldRename: FieldRename.snake)
class DeleteMessageEvent extends Event {
Expand Down
2 changes: 1 addition & 1 deletion lib/api/model/events.g.dart

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

21 changes: 3 additions & 18 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'package:json_annotation/json_annotation.dart';

import '../route/messages.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't like having lib/api/model/ import from lib/api/route/. If it's needed for the model, that feels like a sign it's part of the model and not part of how particular routes interact with the model.

I think these can just stay in this file.

Copy link
Member

Choose a reason for hiding this comment

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

(When this file gets to feel unwieldy, I think the thing to do is to split pieces out by subject area: like a lib/ap/model/message.dart, akin to the narrow.dart and submessage.dart and so on that we already have.)

import 'events.dart';
import 'initial_snapshot.dart';
import 'reaction.dart';
Expand Down Expand Up @@ -806,30 +807,14 @@ enum MessageEditState {
edited,
moved;

// Adapted from the shared code:
// https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts
// The canonical resolved-topic prefix.
static const String _resolvedTopicPrefix = '✔ ';

/// Whether the given topic move reflected either a "resolve topic"
/// or "unresolve topic" operation.
///
/// The Zulip "resolved topics" feature is implemented by renaming the topic;
/// but for purposes of [Message.editState], we want to ignore such renames.
/// This method identifies topic moves that should be ignored in that context.
static bool topicMoveWasResolveOrUnresolve(String topic, String prevTopic) {
if (topic.startsWith(_resolvedTopicPrefix)
&& topic.substring(_resolvedTopicPrefix.length) == prevTopic) {
return true;
}

if (prevTopic.startsWith(_resolvedTopicPrefix)
&& prevTopic.substring(_resolvedTopicPrefix.length) == topic) {
return true;
}

return false;
}
static bool topicMoveWasResolveOrUnresolve(String topic, String prevTopic) =>
topic != prevTopic && topicsMatchModuloResolvePrefix(topic, prevTopic);

static MessageEditState _readFromMessage(Map<dynamic, dynamic> json, String key) {
// Adapted from `analyze_edit_history` in the web app:
Expand Down
56 changes: 56 additions & 0 deletions lib/api/route/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,19 @@ class GetMessagesResult {
// https://zulip.com/api/send-message#parameter-topic
const int kMaxTopicLength = 60;

/// The canonical resolved-topic prefix.
// Adapted from the shared code:
// https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts
const String kResolvedTopicPrefix = '✔ ';

String stripResolvePrefixIfPresent(String topic) =>
topic.startsWith(kResolvedTopicPrefix)
? topic.substring(kResolvedTopicPrefix.length)
: topic;
Comment on lines +166 to +169
Copy link
Member

Choose a reason for hiding this comment

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

How does this end up getting used later in your branch?

Its behavior differs from unresolve_name in the shared code:
https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts#L22-L29

which I'm guessing (haven't checked) is what gets applied when the user unresolves a topic in web or the legacy mobile app.


bool topicsMatchModuloResolvePrefix(String topicA, String topicB) =>
stripResolvePrefixIfPresent(topicA) == stripResolvePrefixIfPresent(topicB);

// https://zulip.com/api/send-message#parameter-content
const int kMaxMessageLengthCodePoints = 10000;

Expand Down Expand Up @@ -258,6 +271,49 @@ class SendMessageResult {
Map<String, dynamic> toJson() => _$SendMessageResultToJson(this);
}

/// https://zulip.com/api/update-message
Future<UpdateMessageResult> updateMessage(
ApiConnection connection, {
required int messageId,
String? topic,
PropagateMode? propagateMode,
bool? sendNotificationToOldThread,
bool? sendNotificationToNewThread,
String? content,
int? streamId,
}) {
return connection.patch('updateMessage', UpdateMessageResult.fromJson, 'messages/$messageId', {
if (topic != null) 'topic': RawParameter(topic),
if (propagateMode != null) 'propagate_mode': RawParameter(propagateMode.toJson()),
if (sendNotificationToOldThread != null) 'send_notification_to_old_thread': sendNotificationToOldThread,
if (sendNotificationToNewThread != null) 'send_notification_to_new_thread': sendNotificationToNewThread,
if (content != null) 'content': RawParameter(content),
if (streamId != null) 'stream_id': streamId,
});
}

/// As in [updateMessage] or [UpdateMessageEvent.propagateMode].
@JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true)
enum PropagateMode {
changeOne,
changeLater,
changeAll;

String toJson() =>_$PropagateModeEnumMap[this]!;
}

@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageResult {
// final List<DetachedUpload> detachedUploads; // TODO handle

UpdateMessageResult();

factory UpdateMessageResult.fromJson(Map<String, dynamic> json) =>
_$UpdateMessageResultFromJson(json);

Map<String, dynamic> toJson() => _$UpdateMessageResultToJson(this);
}

/// https://zulip.com/api/upload-file
Future<UploadFileResult> uploadFile(
ApiConnection connection, {
Expand Down
13 changes: 13 additions & 0 deletions lib/api/route/messages.g.dart

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

10 changes: 10 additions & 0 deletions lib/widgets/action_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ void _showActionSheet(
});
}

/// A button in an action sheet.
///
/// When built from server data, the action sheet ignores changes in that data;
/// we intentionally don't live-update the buttons on events.
/// If a button's label, action, or position changes suddenly,
/// it can be confusing and make the on-tap behavior unexpected.
/// Better to let the user decide to tap
Comment on lines +69 to +73
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah.

Looking for PerAccountStoreWidget.of calls in this file, I think there's one place we don't currently follow this: the hasSelfVote in ReactionButtons.

That one shouldn't cause this problem in practice, though, because it changes by the user's own action. (Or by the message getting deleted, but then it doesn't matter whether the user tries to add a reaction or remove one.)

/// based on information that's comfortably in their working memory,
/// even if we sometimes have to explain (where we handle the tap)
/// that that information has changed and they need to decide again.
Comment on lines +73 to +76
Copy link
Member

Choose a reason for hiding this comment

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

A key point here is that these races are always possible anyway — the change may have happened on the server but not reached us yet.

abstract class ActionSheetMenuItemButton extends StatelessWidget {
const ActionSheetMenuItemButton({super.key, required this.pageContext});

Expand Down
50 changes: 29 additions & 21 deletions lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -132,23 +132,28 @@ class _InboxPageState extends State<InboxPageBody> with PerAccountStoreAwareStat
});

for (final MapEntry(key: streamId, value: topics) in sortedUnreadStreams) {
final topicItems = <(String, int, bool, int)>[];
final topicItems = <_StreamSectionTopicData>[];
int countInStream = 0;
bool streamHasMention = false;
for (final MapEntry(key: topic, value: messageIds) in topics.entries) {
if (!store.isTopicVisible(streamId, topic)) continue;
final countInTopic = messageIds.length;
final hasMention = messageIds.any((messageId) => unreadsModel!.mentions.contains(messageId));
if (hasMention) streamHasMention = true;
topicItems.add((topic, countInTopic, hasMention, messageIds.last));
topicItems.add(_StreamSectionTopicData(
topic: topic,
count: countInTopic,
hasMention: hasMention,
lastUnreadId: messageIds.last,
));
countInStream += countInTopic;
}
if (countInStream == 0) {
continue;
}
topicItems.sort((a, b) {
final (_, _, _, aLastUnreadId) = a;
final (_, _, _, bLastUnreadId) = b;
final aLastUnreadId = a.lastUnreadId;
final bLastUnreadId = b.lastUnreadId;
return bLastUnreadId.compareTo(aLastUnreadId);
});
sections.add(_StreamSectionData(streamId, countInStream, streamHasMention, topicItems));
Expand Down Expand Up @@ -192,11 +197,25 @@ class _StreamSectionData extends _InboxSectionData {
final int streamId;
final int count;
final bool hasMention;
final List<(String, int, bool, int)> items;
final List<_StreamSectionTopicData> items;

const _StreamSectionData(this.streamId, this.count, this.hasMention, this.items);
}

class _StreamSectionTopicData {
final String topic;
final int count;
final bool hasMention;
final int lastUnreadId;

const _StreamSectionTopicData({
required this.topic,
required this.count,
required this.hasMention,
required this.lastUnreadId,
});
}

abstract class _HeaderItem extends StatelessWidget {
final bool collapsed;
final _InboxPageState pageState;
Expand Down Expand Up @@ -466,33 +485,22 @@ class _StreamSection extends StatelessWidget {
child: Column(children: [
header,
if (!collapsed) ...data.items.map((item) {
final (topic, count, hasMention, _) = item;
return _TopicItem(
streamId: data.streamId,
topic: topic,
count: count,
hasMention: hasMention,
);
return _TopicItem(streamId: data.streamId, data: item);
}),
]));
}
}

class _TopicItem extends StatelessWidget {
const _TopicItem({
required this.streamId,
required this.topic,
required this.count,
required this.hasMention,
});
const _TopicItem({required this.streamId, required this.data});

final int streamId;
final String topic;
final int count;
final bool hasMention;
final _StreamSectionTopicData data;

@override
Widget build(BuildContext context) {
final _StreamSectionTopicData(:topic, :count, :hasMention) = data;

final store = PerAccountStoreWidget.of(context);
final subscription = store.subscriptions[streamId]!;

Expand Down
19 changes: 16 additions & 3 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,14 @@ abstract class MessageListPageState {
Narrow get narrow;

/// The controller for this [MessageListPage]'s compose box,
/// if this [MessageListPage] offers a compose box.
/// if this [MessageListPage] offers a compose box and it has mounted,
/// else null.
ComposeBoxController? get composeBoxController;

/// The active [MessageListView].
///
/// This is null if [MessageList] has not mounted yet.
MessageListView? get model;
}

class MessageListPage extends StatefulWidget {
Expand Down Expand Up @@ -214,9 +220,12 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis

@override
ComposeBoxController? get composeBoxController => _composeBoxKey.currentState?.controller;

final GlobalKey<ComposeBoxState> _composeBoxKey = GlobalKey();

@override
MessageListView? get model => _messageListKey.currentState?.model;
final GlobalKey<_MessageListState> _messageListKey = GlobalKey();

@override
void initState() {
super.initState();
Expand Down Expand Up @@ -302,7 +311,11 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
removeBottom: ComposeBox.hasComposeBox(narrow),

child: Expanded(
child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))),
child: MessageList(
key: _messageListKey,
narrow: narrow,
onNarrowChanged: _narrowChanged,
))),
if (ComposeBox.hasComposeBox(narrow))
ComposeBox(key: _composeBoxKey, narrow: narrow)
]))));
Expand Down
1 change: 1 addition & 0 deletions test/api/model/events_checks.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:checks/checks.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/api/route/messages.dart';

extension EventChecks on Subject<Event> {
Subject<int> get id => has((e) => e.id, 'id');
Expand Down
Loading