msglist: Support data model for edited/moved marker

[PIG208]

#171

TODOs:

• Handle message update events
• Support in UI (deferred to a separate PR)

[gnprice]

Thanks!

Quick comments on test style. Both of these are key points for my general approach to writing tests, so I figure they're good to comment on up front.

I haven't read this in detail beyond those two high-level points (and beyond what we talked about in our pairing call today); LMK when it's ready for review or if there are other aspects you'd like feedback on early.

[PIG208]

This PR is rebased on top of #753 with a commit from there edited (to apply these changes: #753 (comment))

[gnprice]

Cool. If this is ready for review (or once it's ready), please mark it in GitHub as "ready for review" i.e. no longer a draft PR. Then @chrisbobbe would you do the first round of review on this PR?

[chrisbobbe]

Thanks, @PIG208! Comments below, including one about the failing test.

[gnprice]

@PIG208 Useful background reading on some of the comments Chris made above about doc comments:
https://dart.dev/effective-dart/documentation#doc-comments
https://github.com/flutter/flutter/blob/d4cffa25402/docs/contributing/Style-guide-for-Flutter-repo.md#documentation-dartdocs-javadocs-etc

Generally we follow the Flutter style guide for docs, with the exception that because the audience for the docs is smaller (because we're writing an app and not a library), we skip the really high-effort things like sample code and diagrams, and don't hold ourselves to documenting everything. We do generally document things that carry significant non-obvious semantics in them — so MessageEditState.areSameTopic in this PR is a good example of where it's quite helpful to be writing docs.

[chrisbobbe]

#753 is now merged, so Zixuan please rebase onto current main.

[PIG208]

I have rebased the branch and addressed the comments. Thanks for the reviews and the link on dartdoc!

[chrisbobbe]

Thanks! Comments below, including one or two things I seem to have missed last time. 🙂

[PIG208]

Thanks for the comments! I have updated the PR accordingly.

[chrisbobbe]

Thanks! A small whitespace nit below, otherwise LGTM. Over to @gnprice for his review.

[gnprice]

Thanks @PIG208, and thanks @chrisbobbe for the previous reviews!

Comments below. I've read through all but the last commit, and all but its tests.

Oh, and a commit-message comment:

message: Handle UpdateMessageEvent for moving/editing.

That's broader than what this commit does — that description would cover actually moving the messages too, i.e. #150. Instead, could say something like:
message: Update editState on events

[gnprice]

Thanks for the revision! (And the revert on that enum-vs-string question.) A few comments.

[gnprice]

This seems to be different from the logic that's used in analyze_edit_history at that very helpful link you added:
https://github.com/zulip/zulip/blob/c31cebbf68a93927d41e9947427c2dd4d46503e3/web/src/message_list_view.js#L68-L118

Let's adjust this code to have the same behavior as what's in web — the question of "was this message moved or not" is basically part of the Zulip model, so it's good for different clients to agree on that question even in edge cases.

[PIG208]

Interestingly, the test

      test('Unresolving topic with a weird prefix -> none', () {
          checkEditState(MessageEditState.none,
            [{'prev_topic': '✔ ✔old_topic', 'topic': 'old_topic'}]);
      });

fails with the implementation change. This definitely aligns a previously different behavior with the web app. I will change the expected value to MessageEditState.moved.

[PIG208]

The PR has been updated to address the reviews. Thanks!

[gnprice]

Thanks for the revision! I think all the behavior looks correct now. Various small comments.

[PIG208]

Thanks for the review @gnprice ! The PR has been updated to address the comments.

[gnprice]

Thanks! Small comments below.

One is actually a latent bug ­— the sameTopicModuloResolve (or previously areSameTopic) method doesn't do what its name and dartdoc say it does.

[gnprice]

Hmm, but is this specific method something we'd want as part of "mark as resolve"? I'm not seeing why it would be.

I think we can probably just leave this TODO comment out; when we go to implement that feature, I think we'll remember that this area of code is here and we'll naturally take care of that.

[gnprice]

Rereading this summary line (and the new name I suggested at #750 (comment) to match it), and then the implementation, I realize that this isn't actually accurate!

If we'd implemented a method that met this description, then it would be reflexive — sameTopicModuloResolve(topic, topic) would be true for any topic. But it isn't; in fact that's always false.

This is actually good because it also helps resolve the concern that I was otherwise thinking of here (and was going to say should be a TODO comment): in general Zulip topics are case-insensitive, so if this really meant a version of "these topics are equal" or "same topic", then it should be case-insensitive too.

I think what this method actually is about is: does this topic move reflect a resolve or unresolve operation. That explains the lack of case insensitivity: a resolve or unresolve wouldn't have changed case. And it explains the irreflexivity: a resolve or unresolve always changes the topic.

[PIG208]

Hmm, this difference is quite subtle. We can rewrite this as "Whether the two topics differs and only differs by whether one of them is resolved or not." And do you think a name like topicsDifferByResolvednessOnly is objectively accurate?

[PIG208]

The previous push addresses the review comments and updates the name of the resolved topic helper.

[gnprice]

Thanks for the revision! This looks good except that the method's name and docs didn't quite get to what I was thinking of with #750 (comment) above; details below.

I'll apply that change, and merge.

[gnprice]

This name and documentation describe how the function is implemented; but the way I'd like to think of what it means is a bit higher-level. From #750 (comment) :

I think what this method actually is about is: does this topic move reflect a resolve or unresolve operation.

So here's a version based on that:

Suggested change

	/// Whether the two topics differs and only differs by whether one of them is
	/// resolved or not.
	///
	/// When a topic is resolved, the clients agree on adding a ✔ prefix to the
	/// topic string. Topics whose only difference is the ✔ prefix are considered
	/// the same. This helper can be helpful when checking if a message has been
	/// moved.
	static bool topicsDifferByResolvednessOnly(String topic, String prevTopic) {
	/// 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) {

The name is kind of wordy but I couldn't think of a shorter one that was accurate. And topicsDifferByResolvednessOnly happens to be the exact same number of characters 🙂

later accepted, just not in the metadata