Mark as resolved/unresolved prep 1 #1242

chrisbobbe · 2024-12-30T21:49:48Z

My local branch for #744 has gotten pretty long and I think it'll benefit from being split into multiple PRs. 🙂 Here's one.

gnprice

Thanks! Good idea.

Comments below from an initial review.

gnprice · 2024-12-30T23:07:04Z

lib/api/route/messages.dart

+String stripResolvePrefixIfPresent(String topic) =>
+  topic.startsWith(kResolvedTopicPrefix)
+    ? topic.substring(kResolvedTopicPrefix.length)
+    : topic;


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.

Updating to match web's behavior.

gnprice · 2024-12-30T23:13:04Z

lib/widgets/action_sheet.dart

+/// 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


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

In a quick manual test, I don't even see the hasSelfVote live-updating in the message action sheet as I add or remove one of the popular emojis from the web app.

hasSelfVote uses PerAccountStoreWidget.of to get the self-user's ID. My user ID doesn't change when I add/remove a reaction. And although we're still signing up to refresh whenever the PerAccountStore notifies listeners, it doesn't notify listeners on ReactionEvents. (Any MessageListViews do notify their listeners.)

In a quick manual test, I don't even see the hasSelfVote live-updating in the message action sheet as I add or remove one of the popular emojis from the web app.

Right, but then it's still your own action — you know that you just made that change (even though via a different client). So there isn't a risk of confusion.

gnprice · 2024-12-30T23:15:11Z

lib/widgets/action_sheet.dart

+/// Better to let the user decide to tap
+/// 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.


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.

gnprice · 2024-12-30T23:23:55Z

lib/api/model/model.dart

@@ -1,5 +1,6 @@
 import 'package:json_annotation/json_annotation.dart';

+import '../route/messages.dart';


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.

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

gnprice · 2024-12-30T23:24:52Z

lib/api/model/events.dart

-/// As in [UpdateMessageEvent.propagateMode].
-@JsonEnum(fieldRename: FieldRename.snake)
-enum PropagateMode {


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.

chrisbobbe · 2025-01-16T01:04:55Z

Thanks for the review! Revision pushed.

gnprice · 2025-01-16T02:52:04Z

lib/api/model/model.dart

-    if (topic.apiName.startsWith(_resolvedTopicPrefix)
-        && topic.apiName.substring(_resolvedTopicPrefix.length) == prevTopic.apiName) {
-      return true;
-    }
-
-    if (prevTopic.apiName.startsWith(_resolvedTopicPrefix)
-        && prevTopic.apiName.substring(_resolvedTopicPrefix.length) == topic.apiName) {
+    if (topic.isSameAs(prevTopic, ignoringResolvePrefix: false)) { // TODO(log)
+      // Not a topic move after all… Return `true`
+      // just because `false` would put a marker for `MessageEditState.moved`
+      // on a message in the message list, which would be conspicuously wrong.
      return true;
    }
-
-    return false;
+    return topic.isSameAs(prevTopic, ignoringResolvePrefix: true);


Because this operation is something we do when decoding one of the high-volume types of data we get from the server (namely messages), it's an area where I'd like to stay somewhat paranoid about performance. (Similar to #1156 (comment) and other comments I've made in some PRs about parsing message content.)

Concretely, that means:

Let's have this function call canonicalize only once on each of the topics, not twice.

(Even then the loop in _readFromMessage will end up calling it twice on each of the topics in the middle of the history, once by passing it as prevTopic here and once as topic. Ideally we'd fix that too, but that might be more annoying and I'm content to let it be. That issue compounds with this one, though — it means that in this revision it calls canonicalize four times on each intermediate topic.)

It's a bit regrettable this needs a regexp instead of the startsWith/substring check. We do want to get the semantics right, though. Is that definitely what web does?

It's a bit regrettable this needs a regexp instead of the startsWith/substring check. We do want to get the semantics right, though. Is that definitely what web does?

Oh! Actually this isn't what web does. The code before matches what web does. Compare (zulip-flutter before vs. current web):

static bool topicMoveWasResolveOrUnresolve(TopicName topic, TopicName prevTopic) { if (topic.apiName.startsWith(TopicName.resolvedTopicPrefix) && topic.apiName.substring(TopicName.resolvedTopicPrefix.length) == prevTopic.apiName) { return true; } if (prevTopic.apiName.startsWith(TopicName.resolvedTopicPrefix) && prevTopic.apiName.substring(TopicName.resolvedTopicPrefix.length) == topic.apiName) { return true; } return false; }

// We know it has a topic edit. Now we need to determine if // it was a true move or a resolve/unresolve. if ( resolved_topic.is_resolved(edit_history_event.topic) && edit_history_event.topic.slice(2) === edit_history_event.prev_topic ) { // Resolved. resolve_toggled = true; continue; } if ( resolved_topic.is_resolved(edit_history_event.prev_topic) && edit_history_event.prev_topic.slice(2) === edit_history_event.topic ) { // Unresolved. resolve_toggled = true; continue; } // Otherwise, it is a real topic rename/move. moved = true;

For this "MOVED"-marker logic, there are pros and cons of following web:

Pro: performance, as you mentioned

Pro (I guess): You get a "MOVED" marker if the topic changes from like "✔ ✔ ✔ some topic" to "✔ ✔ some topic"

Con: You still get a "MOVED" marker on an actual resolve/unresolve, if the resolved-topic prefix is weird: "✔ ✔ some topic" to "some topic"

Con: Case-folding is not done

It looks like web uses the regex in just one place: to make an unresolve-topic API request. That seems sensible; as you mentioned in the office, the output of "unresolve topic" shouldn't be a resolved topic or look like one.

Cool. Yeah, let's stick with the simpler existing behavior which matches web, then.

The job of this "was resolve or unresolve" function is really just to paper over the fact that our "resolve topic" feature is implemented under the hood by renaming the topic. So it's fine that if someone edits the topic name only by fixing the letter case, then that will be displayed with a "MOVED" marker — the user who made that change indeed did so through a "move this topic" UI, just like for other times we show a "MOVED" marker.

Oh I see, yeah, that makes sense.

gnprice · 2025-01-16T02:55:21Z

lib/api/model/model.dart

+    if (topic.isSameAs(prevTopic, ignoringResolvePrefix: false)) { // TODO(log)
+      // Not a topic move after all… Return `true`
+      // just because `false` would put a marker for `MessageEditState.moved`
+      // on a message in the message list, which would be conspicuously wrong.
      return true;


I guess further on the point of performance paranoia: is this a situation that the server is actually expected to produce?

I think not — https://zulip.com/api/get-messages at prev_topic says:

Only present if message's topic was edited.

So we don't need to go out of our way trying to detect it.

gnprice · 2025-01-16T03:02:45Z

lib/api/model/model.dart

+  ///
+  /// If [ignoringResolvePrefix] is true,
+  /// the [unresolve]d forms of [this] and [other] are compared instead.
+  bool isSameAs(TopicName other, {required bool ignoringResolvePrefix}) {


This feels to me more like two different methods than one method with a boolean flag.

And then I think the "ignoring" case is basically doing much the same job as its one caller topicMoveWasResolveOrUnresolve, so can be inlined into there.

gnprice · 2025-01-16T03:05:55Z

lib/widgets/action_sheet.dart

+/// 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


In a quick manual test, I don't even see the hasSelfVote live-updating in the message action sheet as I add or remove one of the popular emojis from the web app.

Right, but then it's still your own action — you know that you just made that change (even though via a different client). So there isn't a risk of confusion.

gnprice

Thanks for the revision! Generally this all looks good. A few comments above, and a couple more below.

gnprice · 2025-01-16T03:07:37Z

lib/api/model/model.dart

+  changeLater,
+  changeAll;
+
+  String toJson() =>_$PropagateModeEnumMap[this]!;


nit:

Suggested change

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

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

gnprice · 2025-01-16T03:12:03Z

lib/api/route/messages.dart

+Future<UpdateMessageResult> updateMessage(
+  ApiConnection connection, {
+  required int messageId,
+  String? topic,


Suggested change

String? topic,

TopicName? topic,

🙂

chrisbobbe · 2025-01-16T22:27:36Z

Thanks for the review! Revision pushed, and please see #1242 (comment).

gnprice · 2025-01-16T23:59:37Z

Thanks! All looks good except that open subthread above.

… delay Fixes zulip#1289.

Instead of passing its fields separately. (We'd like to start using lastUnreadId from the data too.)

This will make it available in a long-press handler in the app bar for a topic narrow. That long-press handler brings up the topic action sheet, which will need a message ID to pass in the API request for resolve/unresolve topic. (Not sure the API request should *need* to require a message ID, but it does; shrug.)

And move the PropagateMode definition from events.dart to model.dart, since it's no longer specific to an event.

chrisbobbe · 2025-01-17T23:26:47Z

Thanks for the review! Revision pushed, atop #1290 for the sake of CI.

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Dec 30, 2024

chrisbobbe requested a review from gnprice December 30, 2024 21:49

gnprice reviewed Dec 30, 2024

View reviewed changes

chrisbobbe force-pushed the pr-resolve-unresolve-prep-1 branch from fe11d69 to 68d7cd1 Compare January 16, 2025 01:04

gnprice reviewed Jan 16, 2025

View reviewed changes

chrisbobbe force-pushed the pr-resolve-unresolve-prep-1 branch from 68d7cd1 to ee4f762 Compare January 16, 2025 22:27

chrisbobbe added 10 commits January 17, 2025 13:07

autocomplete test: Add extra pump in new test for new RawAutocomplete…

5e8d0a8

… delay Fixes zulip#1289.

inbox [nfc]: Factor out a data class so things are clearly labeled

b0251d7

inbox [nfc]: Pass new data class to topic-item widget

e7dee29

Instead of passing its fields separately. (We'd like to start using lastUnreadId from the data too.)

msglist [nfc]: Flag wrinkle in MessageListPageState.composeBoxController

64b3116

api [nfc]: Move resolved-topic-prefix constant onto TopicName

1fbe495

api: Implement TopicName.isSameAs(other) and .unresolve()

61a588f

api [nfc]: Comment on why to keep an existing implementation

d8b922f

action_sheet [nfc]: Dartdoc ActionSheetMenuItemButton

dd4715e

api: Add updateMessage route

3ef535f

And move the PropagateMode definition from events.dart to model.dart, since it's no longer specific to an event.

chrisbobbe force-pushed the pr-resolve-unresolve-prep-1 branch from ee4f762 to 3ef535f Compare January 17, 2025 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark as resolved/unresolved prep 1 #1242

Mark as resolved/unresolved prep 1 #1242

chrisbobbe commented Dec 30, 2024

gnprice left a comment

gnprice Dec 30, 2024

chrisbobbe Jan 16, 2025

gnprice Dec 30, 2024

chrisbobbe Jan 16, 2025

gnprice Jan 16, 2025

gnprice Dec 30, 2024

gnprice Dec 30, 2024

gnprice Dec 30, 2024 •

edited

Loading

gnprice Dec 30, 2024

chrisbobbe commented Jan 16, 2025

gnprice Jan 16, 2025

chrisbobbe Jan 16, 2025 •

edited

Loading

chrisbobbe Jan 16, 2025 •

edited

Loading

gnprice Jan 16, 2025

chrisbobbe Jan 17, 2025

gnprice Jan 16, 2025

gnprice Jan 16, 2025

gnprice Jan 16, 2025

gnprice left a comment

gnprice Jan 16, 2025

gnprice Jan 16, 2025

chrisbobbe commented Jan 16, 2025

gnprice commented Jan 16, 2025

chrisbobbe commented Jan 17, 2025

		@@ -1,5 +1,6 @@
		import 'package:json_annotation/json_annotation.dart';

		import '../route/messages.dart';

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

Mark as resolved/unresolved prep 1 #1242

Are you sure you want to change the base?

Mark as resolved/unresolved prep 1 #1242

Conversation

chrisbobbe commented Dec 30, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 16, 2025

Choose a reason for hiding this comment

chrisbobbe Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 16, 2025

gnprice commented Jan 16, 2025

chrisbobbe commented Jan 17, 2025

gnprice Dec 30, 2024 •

edited

Loading

chrisbobbe Jan 16, 2025 •

edited

Loading

chrisbobbe Jan 16, 2025 •

edited

Loading