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.

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

refactor: Message #4915

Merged
merged 9 commits into from
Nov 2, 2023
Merged

refactor: Message #4915

merged 9 commits into from
Nov 2, 2023

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented Oct 24, 2023 •
edited
Loading

Description

this PR was cherry-picked into smaller PRs

With the last remaining changes to Message & some other small changes staying here

Message: Remove empty anon namespace
Message: Remove else after return
Message: Avoid repeating type in return
Message: Remove unused includes (there were a few)
Helix: Remove unneccesary static for already-static variables
AttachedWindow: Remove unused ForwardDecl.hpp include

github-actions[bot]
github-actions bot reviewed Oct 24, 2023
View reviewed changes
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 48. Check the log or trigger a new build to see more.

src/controllers/commands/CommandController.hpp Outdated
@@ -33,8 +33,8 @@ class CommandController final : public Singleton
bool dryRun);
QStringList getDefaultChatterinoCommandList();

virtual void initialize(Settings &, Paths &paths) override;
virtual void save() override;
void initialize(Settings &, Paths &paths) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void initialize(Settings &, Paths &paths) override;
void initialize(Settings & /*settings*/, Paths &paths) override;

src/providers/twitch/IrcMessageHandler.cpp Outdated
}

if (chan->isBroadcaster())
// get username, duration and message of the timed out user
QString username = message->parameter(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'username' of type 'QString' can be declared 'const' [misc-const-correctness]

Suggested change
QString username = message->parameter(1);
QString const username = message->parameter(1);

src/providers/twitch/IrcMessageHandler.cpp Outdated
// get username, duration and message of the timed out user
QString username = message->parameter(1);
QString durationInSeconds;
QVariant v = message->tag("ban-duration");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'v' of type 'QVariant' can be declared 'const' [misc-const-correctness]

Suggested change
QVariant v = message->tag("ban-duration");
QVariant const v = message->tag("ban-duration");

src/providers/twitch/IrcMessageHandler.cpp Outdated
{
std::vector<MessagePtr> builtMessage;

QString remainingTime =
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'remainingTime' of type 'QString' can be declared 'const' [misc-const-correctness]

Suggested change
QString remainingTime =
QString const remainingTime =

src/providers/twitch/IrcMessageHandler.cpp Outdated

QString remainingTime =
formatTime(message->content().split(" ").value(5));
QString formattedMessage =
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'formattedMessage' of type 'QString' can be declared 'const' [misc-const-correctness]

Suggested change
QString formattedMessage =
QString const formattedMessage =

src/widgets/BaseWindow.hpp Outdated
@@ -72,20 +72,20 @@ class BaseWindow : public BaseWidget
bool nativeEvent(const QByteArray &eventType, void *message,
long *result) override;
#endif
virtual void scaleChangedEvent(float) override;
void scaleChangedEvent(float) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void scaleChangedEvent(float) override;
void scaleChangedEvent(float /*newScale*/) override;

src/widgets/BaseWindow.hpp Outdated

virtual void paintEvent(QPaintEvent *) override;
void paintEvent(QPaintEvent *) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void paintEvent(QPaintEvent *) override;
void paintEvent(QPaintEvent * /*event*/) override;

src/widgets/BaseWindow.hpp Outdated
virtual void moveEvent(QMoveEvent *) override;
virtual void closeEvent(QCloseEvent *) override;
virtual void showEvent(QShowEvent *) override;
void changeEvent(QEvent *) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void changeEvent(QEvent *) override;
void changeEvent(QEvent * /*unused*/) override;

src/widgets/BaseWindow.hpp Outdated
virtual void closeEvent(QCloseEvent *) override;
virtual void showEvent(QShowEvent *) override;
void changeEvent(QEvent *) override;
void leaveEvent(QEvent *) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void leaveEvent(QEvent *) override;
void leaveEvent(QEvent * /*event*/) override;

src/widgets/BaseWindow.hpp Outdated
virtual void showEvent(QShowEvent *) override;
void changeEvent(QEvent *) override;
void leaveEvent(QEvent *) override;
void resizeEvent(QResizeEvent *) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
void resizeEvent(QResizeEvent *) override;
void resizeEvent(QResizeEvent * /*event*/) override;

@Mm2PL
Copy link
Collaborator

Mm2PL commented Oct 24, 2023

image
lgtm

@pajlada pajlada mentioned this pull request Oct 24, 2023
Merged
@pajlada pajlada changed the title big refactor big refactor - do not merge 😠 Oct 25, 2023
github-actions[bot]
github-actions bot reviewed Oct 25, 2023
View reviewed changes
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/widgets/helper/ChannelView.cpp Outdated
}

// Link copy
QString url = link.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'url' of type 'QString' can be declared 'const' [misc-const-correctness]

Suggested change
QString url = link.value;
QString const url = link.value;

src/widgets/helper/ChannelView.cpp Outdated
{
openTwitchUsercard(this->channel_->getName(),
hoverLayoutElement->getLink().value);
return;
}
else if (hoverLayoutElement->getLink().isUrl() == false)
if (hoverLayoutElement->getLink().isUrl() == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]

Suggested change
if (hoverLayoutElement->getLink().isUrl() == false)
if (!hoverLayoutElement->getLink().isUrl())

src/widgets/helper/ChannelView.cpp Outdated
@@ -2742,7 +2814,9 @@
int ChannelView::getLayoutWidth() const
{
if (this->scrollBar_->isVisible())
return int(this->width() - scrollbarPadding * this->scale());
{
return int(this->width() - SCROLLBAR_PADDING * this->scale());
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'int' to 'float' [bugprone-narrowing-conversions]

        return int(this->width() - SCROLLBAR_PADDING * this->scale());
                   ^

@pajlada pajlada force-pushed the chore/refactor4 branch 2 times, most recently from 0609034 to 11f2704 Compare October 31, 2023 13:55
@pajlada pajlada changed the title big refactor - do not merge 😠 refactor: Message Oct 31, 2023
@pajlada pajlada marked this pull request as ready for review October 31, 2023 20:32
Nerixyz
Nerixyz reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

For a future refactoring: There are a few #pragma comment(lib, "my.lib") in the code (e.g. in AttachedWindow.cpp). These can be replaced by target_link_libraries(<target> PRIVATE my.lib) (maybe there's even a better way of doing this in CMake).

src/messages/Message.cpp
Comment on lines +64 to +69
return {
ColorProvider::instance().color(ColorType::FirstMessageHighlight),
SBHighlight::Default, false, true);
ScrollbarHighlight::Default,
false,
true,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Not from this PR: The ScrollbarHighlight needs some refactoring. The arguments isRedeemedHighlight,
isFirstMessageHighlight, and isElevatedMessageHighlight are mutually exclusive - shouldn't this be refactored to an enum? Even if they're not mutually exclusive, a bit field would be more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely should be enums yeah

pajlada
pajlada commented Nov 2, 2023
View reviewed changes
Copy link
Member Author

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

image

@pajlada pajlada enabled auto-merge (squash) November 2, 2023 14:28
@pajlada pajlada merged commit 4e63a1b into master Nov 2, 2023
15 checks passed
@pajlada pajlada deleted the chore/refactor4 branch November 2, 2023 14:48
@ilya-zlobintsev ilya-zlobintsev mentioned this pull request Jan 30, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@Nerixyz Nerixyz Nerixyz left review comments

Assignees
No one assigned
Labels
no changelog entry needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pajlada @Mm2PL @Nerixyz