Skip to content

content: Handle @-topic mentions #1071

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

Jump to bottom
Merged
merged 1 commit into from
Dec 19, 2024
Merged

content: Handle @-topic mentions #1071

merged 1 commit into from
Dec 19, 2024

Conversation

rajveermalviya
Copy link
Member

Fixes: #892

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Nov 22, 2024
@PIG208 PIG208 self-assigned this Dec 3, 2024
@rajveermalviya
Copy link
Member Author

(Rebased to main)

PIG208
PIG208 reviewed Dec 12, 2024
View reviewed changes
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Left some comments. This looks good to me!

lib/model/content.dart Outdated
enum UserMentionType { user, userGroup }

class UserMentionNode extends InlineContainerNode {
class UserMentionNode extends MentionNode {
const UserMentionNode({
super.debugHtmlNode,
required super.nodes,
Copy link
Member

Choose a reason for hiding this comment

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

nit: the comments below and in the body should move with MentionNode

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure how the comment about silent mentions would apply to this generic MentionNode, since it's being introduced for @topic mention and not sure how a silent @topic would work. I feel this comment is specific to UserMentionNode.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see! Yeah that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

A silent @-topic mention might not be super useful, but it does exist:
https://chat.zulip.org/#narrow/channel/7-test-here/topic/mention/near/2007548

test/widgets/content_test.dart Outdated
@@ -684,6 +684,47 @@ void main() {
// TODO(#647):
// testFontWeight('non-silent self-user mention in bold context',
// expectedWght: 800, // [etc.]

testContentSmoke(ContentExample.topicMentionPlain);
testContentSmoke(ContentExample.topicMentionSilent);
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like the other smoke tests are up top in this group. Let's move these there too.

test/model/content_test.dart
@@ -180,6 +180,26 @@ class ContentExample {
expectedText: 'all',
'<p><span class="silent user-mention" data-user-id="*">all</span></p>',
const UserMentionNode(nodes: [TextNode('all')]));
static final topicMentionPlain = ContentExample.inline(
Copy link
Member

Choose a reason for hiding this comment

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

nit: blank line before this

@rajveermalviya
Copy link
Member Author

Thanks for the review @PIG208!
Pushed a new revison, also a have question, PTAL.

@PIG208 PIG208 assigned gnprice and unassigned PIG208 Dec 13, 2024
@PIG208 PIG208 requested review from gnprice and removed request for PIG208 December 13, 2024 18:55
@PIG208 PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 13, 2024
@PIG208
Copy link
Member

PIG208 commented Dec 13, 2024

Looks good to me! Marking this for Greg's review.

gnprice
gnprice reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @rajveermalviya, and thanks @PIG208 for the previous review!

Let's actually handle this within the existing class UserMentionNode. It's meant to encompass not only mentions of individual users, but of sets of users either via user groups, the current channel, the current topic, or any other such features we might introduce in the future. And it's already how we currently handle @-channel mentions.

To distinguish @-topic mentions from mentions of individual users, we can use the mentionType field that's sketched in a comment, and the currently-unused UserMentionType enum. That can happen in a separate commit, either before or after the commit that adds @-topic mentions in the first place; if we introduce @-topic mentions first (and just give them the same background as direct user mentions), their treatment isn't any worse than what we have now for @-channel.

lib/model/content.dart Outdated
enum UserMentionType { user, userGroup }

class UserMentionNode extends InlineContainerNode {
class UserMentionNode extends MentionNode {
const UserMentionNode({
super.debugHtmlNode,
required super.nodes,
Copy link
Member

Choose a reason for hiding this comment

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

A silent @-topic mention might not be super useful, but it does exist:
https://chat.zulip.org/#narrow/channel/7-test-here/topic/mention/near/2007548

lib/model/content.dart Outdated
@@ -717,6 +721,10 @@ class UserMentionNode extends InlineContainerNode {
// final bool isSilent; // TODO(#647)
}

class TopicMentionNode extends MentionNode {
Copy link
Member

Choose a reason for hiding this comment

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

I think the name "topic mention" for this, with no further context, will be confusing — it sounds like it might refer to the #**channel > topic** feature, which is unrelated.

lib/widgets/content.dart Outdated
Comment on lines 1142 to 1145
// TODO(#646) different for wildcard mentions
color: contentTheme.colorDirectMentionBackground,
color: switch (node) {
UserMentionNode() => contentTheme.colorDirectMentionBackground,
TopicMentionNode() => contentTheme.colorTopicMentionBackground,
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is beginning to implement #646 — we're now giving some wildcard mentions (namely for topics) a different background vs. direct mentions.

Should @-channel mentions get the same background as @-topic?

Copy link
Collaborator

@alya alya Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah! But personal mentions trump channel/topic/group mentions if a message has both kinds.

Original issue for reference (we've probably tweaked colors since then): zulip/zulip#22059
Corner case that needed fixing, in case this helps avoid an issue: zulip/zulip#27654

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the references!

To possibly clarify, though: the immediate code here is about how the mention is formatted within the message, rather than how the message as a whole gets highlighted when it is/has a mention. The latter is tracked under #647. I think that part will probably call for a separate PR from this one, but the formatting of the mention elements themselves might be easy to fold in here.

Copy link
Member Author

@rajveermalviya rajveermalviya Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to remove this part, will open separate PR for #646 later as it's a post-launch issue.

@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed a new revision, PTAL.

@gnprice
Copy link
Member

gnprice commented Dec 19, 2024

Thanks! Looks good; merging.

@gnprice gnprice merged commit 0edabbf into zulip:main Dec 19, 2024
1 check passed
@rajveermalviya rajveermalviya deleted the topic-mention branch December 19, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alya alya alya left review comments

@PIG208 PIG208 PIG208 left review comments

@gnprice gnprice Awaiting requested review from gnprice

Assignees

@gnprice gnprice

Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parse @-topic mentions
4 participants
@rajveermalviya @PIG208 @gnprice @alya