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

refactor: make a single MessageBuilder #5548

Merged
merged 16 commits into from
Aug 24, 2024
Merged

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented Aug 17, 2024

This removes TwitchMessageBuilder and SharedMessageBuilder, and moves all relevant functionality into MessageBuilder

MessageBuilder is chunky, but it's at least in one file now instead of spread out with little reason.

There's a few more things that could probably be done here, like making sure we make better use of static functions for message makers so consumers don't have to set message flags themselves.

A common pattern I still see is:

MessageBuilder builder(foo, bar);
builder.flags = asd;
auto msg = builder.release();

where we could just have a static function instead

auto msg = MessageBuilder::buildFooBar(foo, bar);

with an alternative parameter adding extra flags for ones that are a bit more dynamic

auto msg = MessageBuilder::buildFooBar(foo, bar, {ExtraFlag});

@pajlada pajlada force-pushed the refactor/message-builder branch from d99b081 to 471a55f Compare August 18, 2024 10:25
pajlada added 13 commits August 18, 2024 14:05
The function is now called `makeLiveMessage` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The function is now called `makeLiveSystemMessage` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The function is now called `makeOfflineSystemMessage` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The function is now called `makeHostingSystemMessage` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The function is now called `makeListOfUsersSystemMEssage` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The Collapsed usage from `Chatters` is now part of the params
The function is now called `makeDeletionMessageFromIRC` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
The function is now called `makeDeletionMessageFromPubSub` and it creates the
MessageBuilder itself, and returns a complete MessagePtr
It's been made into a non-static function, with an accompanying static function to make an empty MessageBuilder & use the non-static function on it.
@pajlada pajlada force-pushed the refactor/message-builder branch from c601dd6 to 4fc7389 Compare August 18, 2024 12:05
@pajlada pajlada marked this pull request as ready for review August 18, 2024 12:05
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

There were too many comments to post at once. Showing the first 25 out of 32. Check the log or trigger a new build to see more.


const QColor AUTOMOD_USER_COLOR{"blue"};

const QString regexHelpString("(\\w+)[.,!?;:]*?$");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'regexHelpString' [readability-identifier-naming]

Suggested change
const QString regexHelpString("(\\w+)[.,!?;:]*?$");
const QString REGEX_HELP_STRING("(\\w+)[.,!?;:]*?$");

src/messages/MessageBuilder.cpp:66:

- const QRegularExpression mentionRegex("^@" + regexHelpString);
+ const QRegularExpression mentionRegex("^@" + REGEX_HELP_STRING);

src/messages/MessageBuilder.cpp:69:

- const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
+ const QRegularExpression allUsernamesMentionRegex("^" + REGEX_HELP_STRING);

const QString regexHelpString("(\\w+)[.,!?;:]*?$");

// matches a mention with punctuation at the end, like "@username," or "@username!!!" where capture group would return "username"
const QRegularExpression mentionRegex("^@" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'mentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression mentionRegex("^@" + regexHelpString);
const QRegularExpression MENTION_REGEX("^@" + regexHelpString);

src/messages/MessageBuilder.cpp:2441:

-     {
+     {MENTION_REGEX

const QRegularExpression mentionRegex("^@" + regexHelpString);

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'allUsernamesMentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
const QRegularExpression ALL_USERNAMES_MENTION_REGEX("^" + regexHelpString);

src/messages/MessageBuilder.cpp:2476:

-     {
+     {ALL_USERNAMES_MENTION_REGEX

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);

const QSet<QString> zeroWidthEmotes{
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'zeroWidthEmotes' [readability-identifier-naming]

Suggested change
const QSet<QString> zeroWidthEmotes{
const QSet<QString> ZERO_WIDTH_EMOTES{

src/messages/MessageBuilder.cpp:2902:

- mote;
+ mote;ZERO_WIDTH_EMOTES

return QUrl::fromLocalFile(path);
}

return QUrl("qrc:/sounds/ping2.wav");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list]

    return QUrl("qrc:/sounds/ping2.wav");
           ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

el &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2885: if it should be an assignment, move it out of the 'if' condition

el &&
                          ^

src/messages/MessageBuilder.cpp:2885: if it is meant to be an equality check, change '=' to '=='

el &&
                          ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion 'TwitchChannel *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change
(emote = this->twitchChannel->bttvEmote(name)))
}
el &&( != nullptr)

flags = MessageElementFlag::BttvEmote;
}
else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

tr &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2890: if it should be an assignment, move it out of the 'if' condition

tr &&
                          ^

src/messages/MessageBuilder.cpp:2890: if it is meant to be an equality check, change '=' to '=='

tr &&
                          ^

else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
{
flags = MessageElementFlag::SevenTVEmote;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: repeated branch body in conditional chain [bugprone-branch-clone]

    {
    ^
Additional context

src/messages/MessageBuilder.cpp:2895: end of the original

    }
     ^

src/messages/MessageBuilder.cpp:2906: clone 1 starts here

    {
    ^

flags = MessageElementFlag::SevenTVEmote;
zeroWidth = emote.value()->zeroWidth;
}
else if ((emote = globalFfzEmotes->emote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

    }
                          ^
Additional context

src/messages/MessageBuilder.cpp:2895: if it should be an assignment, move it out of the 'if' condition

    }
                          ^

src/messages/MessageBuilder.cpp:2895: if it is meant to be an equality check, change '=' to '=='

    }
                          ^

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

There were too many comments to post at once. Showing the first 25 out of 33. Check the log or trigger a new build to see more.

@@ -16,15 +16,16 @@

using namespace chatterino;

class BenchmarkMessageBuilder : public SharedMessageBuilder
class BenchmarkMessageBuilder : public MessageBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: destructor of 'BenchmarkMessageBuilder' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

class BenchmarkMessageBuilder : public MessageBuilder
      ^
Additional context

benchmarks/src/Highlights.cpp:18: make it public and virtual

class BenchmarkMessageBuilder : public MessageBuilder
      ^


const QColor AUTOMOD_USER_COLOR{"blue"};

const QString regexHelpString("(\\w+)[.,!?;:]*?$");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'regexHelpString' [readability-identifier-naming]

Suggested change
const QString regexHelpString("(\\w+)[.,!?;:]*?$");
const QString REGEX_HELP_STRING("(\\w+)[.,!?;:]*?$");

src/messages/MessageBuilder.cpp:66:

- const QRegularExpression mentionRegex("^@" + regexHelpString);
+ const QRegularExpression mentionRegex("^@" + REGEX_HELP_STRING);

src/messages/MessageBuilder.cpp:69:

- const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
+ const QRegularExpression allUsernamesMentionRegex("^" + REGEX_HELP_STRING);

const QString regexHelpString("(\\w+)[.,!?;:]*?$");

// matches a mention with punctuation at the end, like "@username," or "@username!!!" where capture group would return "username"
const QRegularExpression mentionRegex("^@" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'mentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression mentionRegex("^@" + regexHelpString);
const QRegularExpression MENTION_REGEX("^@" + regexHelpString);

src/messages/MessageBuilder.cpp:2441:

-     {
+     {MENTION_REGEX

const QRegularExpression mentionRegex("^@" + regexHelpString);

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'allUsernamesMentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
const QRegularExpression ALL_USERNAMES_MENTION_REGEX("^" + regexHelpString);

src/messages/MessageBuilder.cpp:2476:

-     {
+     {ALL_USERNAMES_MENTION_REGEX

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);

const QSet<QString> zeroWidthEmotes{
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'zeroWidthEmotes' [readability-identifier-naming]

Suggested change
const QSet<QString> zeroWidthEmotes{
const QSet<QString> ZERO_WIDTH_EMOTES{

src/messages/MessageBuilder.cpp:2902:

- mote;
+ mote;ZERO_WIDTH_EMOTES

// - 7TV Global
if (this->twitchChannel && (emote = this->twitchChannel->ffzEmote(name)))
{
flags = MessageElementFlag::FfzEmote;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: repeated branch body in conditional chain [bugprone-branch-clone]

    {
    ^
Additional context

src/messages/MessageBuilder.cpp:2884: end of the original

    }
     ^

src/messages/MessageBuilder.cpp:2897: clone 1 starts here

    {
    ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

el &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2885: if it should be an assignment, move it out of the 'if' condition

el &&
                          ^

src/messages/MessageBuilder.cpp:2885: if it is meant to be an equality check, change '=' to '=='

el &&
                          ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion 'TwitchChannel *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change
(emote = this->twitchChannel->bttvEmote(name)))
}
el &&( != nullptr)

flags = MessageElementFlag::BttvEmote;
}
else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

tr &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2890: if it should be an assignment, move it out of the 'if' condition

tr &&
                          ^

src/messages/MessageBuilder.cpp:2890: if it is meant to be an equality check, change '=' to '=='

tr &&
                          ^

else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
{
flags = MessageElementFlag::SevenTVEmote;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: repeated branch body in conditional chain [bugprone-branch-clone]

    {
    ^
Additional context

src/messages/MessageBuilder.cpp:2895: end of the original

    }
     ^

src/messages/MessageBuilder.cpp:2906: clone 1 starts here

    {
    ^

@pajlada
Copy link
Member Author

pajlada commented Aug 24, 2024

image

@pajlada pajlada enabled auto-merge (squash) August 24, 2024 09:44
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

There were too many comments to post at once. Showing the first 25 out of 33. Check the log or trigger a new build to see more.

@@ -16,15 +16,16 @@

using namespace chatterino;

class BenchmarkMessageBuilder : public SharedMessageBuilder
class BenchmarkMessageBuilder : public MessageBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: destructor of 'BenchmarkMessageBuilder' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

class BenchmarkMessageBuilder : public MessageBuilder
      ^
Additional context

benchmarks/src/Highlights.cpp:18: make it public and virtual

class BenchmarkMessageBuilder : public MessageBuilder
      ^


const QColor AUTOMOD_USER_COLOR{"blue"};

const QString regexHelpString("(\\w+)[.,!?;:]*?$");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'regexHelpString' [readability-identifier-naming]

Suggested change
const QString regexHelpString("(\\w+)[.,!?;:]*?$");
const QString REGEX_HELP_STRING("(\\w+)[.,!?;:]*?$");

src/messages/MessageBuilder.cpp:66:

- const QRegularExpression mentionRegex("^@" + regexHelpString);
+ const QRegularExpression mentionRegex("^@" + REGEX_HELP_STRING);

src/messages/MessageBuilder.cpp:69:

- const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
+ const QRegularExpression allUsernamesMentionRegex("^" + REGEX_HELP_STRING);

const QString regexHelpString("(\\w+)[.,!?;:]*?$");

// matches a mention with punctuation at the end, like "@username," or "@username!!!" where capture group would return "username"
const QRegularExpression mentionRegex("^@" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'mentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression mentionRegex("^@" + regexHelpString);
const QRegularExpression MENTION_REGEX("^@" + regexHelpString);

src/messages/MessageBuilder.cpp:2441:

-     {
+     {MENTION_REGEX

const QRegularExpression mentionRegex("^@" + regexHelpString);

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'allUsernamesMentionRegex' [readability-identifier-naming]

Suggested change
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);
const QRegularExpression ALL_USERNAMES_MENTION_REGEX("^" + regexHelpString);

src/messages/MessageBuilder.cpp:2476:

-     {
+     {ALL_USERNAMES_MENTION_REGEX

// if findAllUsernames setting is enabled, matches strings like in the examples above, but without @ symbol at the beginning
const QRegularExpression allUsernamesMentionRegex("^" + regexHelpString);

const QSet<QString> zeroWidthEmotes{
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global constant 'zeroWidthEmotes' [readability-identifier-naming]

Suggested change
const QSet<QString> zeroWidthEmotes{
const QSet<QString> ZERO_WIDTH_EMOTES{

src/messages/MessageBuilder.cpp:2902:

- mote;
+ mote;ZERO_WIDTH_EMOTES

// - 7TV Global
if (this->twitchChannel && (emote = this->twitchChannel->ffzEmote(name)))
{
flags = MessageElementFlag::FfzEmote;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: repeated branch body in conditional chain [bugprone-branch-clone]

    {
    ^
Additional context

src/messages/MessageBuilder.cpp:2884: end of the original

    }
     ^

src/messages/MessageBuilder.cpp:2897: clone 1 starts here

    {
    ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

el &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2885: if it should be an assignment, move it out of the 'if' condition

el &&
                          ^

src/messages/MessageBuilder.cpp:2885: if it is meant to be an equality check, change '=' to '=='

el &&
                          ^

flags = MessageElementFlag::FfzEmote;
}
else if (this->twitchChannel &&
(emote = this->twitchChannel->bttvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion 'TwitchChannel *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change
(emote = this->twitchChannel->bttvEmote(name)))
}
el &&( != nullptr)

flags = MessageElementFlag::BttvEmote;
}
else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

tr &&
                          ^
Additional context

src/messages/MessageBuilder.cpp:2890: if it should be an assignment, move it out of the 'if' condition

tr &&
                          ^

src/messages/MessageBuilder.cpp:2890: if it is meant to be an equality check, change '=' to '=='

tr &&
                          ^

else if (this->twitchChannel != nullptr &&
(emote = this->twitchChannel->seventvEmote(name)))
{
flags = MessageElementFlag::SevenTVEmote;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: repeated branch body in conditional chain [bugprone-branch-clone]

    {
    ^
Additional context

src/messages/MessageBuilder.cpp:2895: end of the original

    }
     ^

src/messages/MessageBuilder.cpp:2906: clone 1 starts here

    {
    ^

@pajlada pajlada merged commit 175afa8 into master Aug 24, 2024
17 checks passed
@pajlada pajlada deleted the refactor/message-builder branch August 24, 2024 10:18
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.

1 participant