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

feat: make raid entry username clickable #5651

Merged

Conversation

jupjohn
Copy link
Contributor

@jupjohn jupjohn commented Oct 16, 2024

This PR aims to make the username of entering raid messages clickable using the `MentionElement". Did a bit of cleanup on the duplicated emplacement code which was shared by the path used by other system messages types.

I'm not sure how I feel about colouring the usernames with the user color - maybe it's better to colour them as SystemMessage instead?

Example from a historical message:
image

@jupjohn jupjohn marked this pull request as ready for review October 16, 2024 00:45
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Some small nits, overall looks good

Comment on lines 515 to 525
auto buildAndEmplaceMessage = [&builtMessages,
mirrored](MessageBuilder &builder) {
builder->flags.set(MessageFlag::Subscription);
if (mirrored)
{
builder->flags.set(MessageFlag::SharedMessage);
}

auto newMessage = builder.release();
builtMessages.emplace_back(newMessage);
};
Copy link
Member

Choose a reason for hiding this comment

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

Undo this change - leave the duplicate code.

The code, as is, would flag the raid message as a subscription message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you wanting it to be duplicated so that the raid message isn't flagged as Subscription? I believe it should be as that's (currently) what dictates the background colour & highlighting.

If you'd rather it wasn't tagged as subscription, I'm more than happy to introduce some "event" (for lack of a better name) message type that has the same behaviour, but isn't tied to subscription bits (and isn't connected to subscription-related settings).

Otherwise, happy to revert this 👍

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 it's still best to revert so we don't end up thinking it's correct. If it still requires the sub flag, leave the sub flag but just in its own place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mint, will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-inlined in afe3487

Comment on lines 548 to 549
auto loginTag = tags.find("login");
auto displayNameTag = tags.find("msg-param-displayName");
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't care to make different implementation depending on if the value is unset, or if the value is set, or if the value is set but empty, instead of doing the v = tags.find("..."); v != tags.end(); dance, use tags.value("...").toString(), then check if the string is valid/non-empty instead.
Same goes for the color code, that way we don't need to blindly trust that it's a valid color, and keep the SystemColor if the color is invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c620474 and 61f77a1

@pajlada
Copy link
Member

pajlada commented Oct 18, 2024

I'm not sure how I feel about colouring the usernames with the user color - maybe it's better to colour them as SystemMessage instead?

We should keep the username colored, as the current implementation does.

@jupjohn
Copy link
Contributor Author

jupjohn commented Oct 19, 2024

Retested on a92b5ee:
image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Currently only works for recent messages, have talked offline about solution

@jupjohn
Copy link
Contributor Author

jupjohn commented Oct 19, 2024

I've duplicated raid message building logic to the non-RM path in d083c42

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Show resolved Hide resolved
src/messages/MessageBuilder.hpp Show resolved Hide resolved
@pajlada pajlada merged commit fb787b5 into Chatterino:master Oct 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants