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

Add settings tooltips #3437

Merged
merged 12 commits into from Oct 30, 2022
Merged

Add settings tooltips #3437

merged 12 commits into from Oct 30, 2022

Conversation

acdvs
Copy link
Contributor

@acdvs acdvs commented Dec 28, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Resolves #2146

Displays a tooltip when hovering over a setting. This adds another parameter to each GeneralPageView::add[Widget] function for easy definition within GeneralPage. Uses the built-in Qt tooltip with a modified stylesheet to match the settings page colors.

Example

Initially, this PR adds tooltips for the settings listed in #2146 at the time of writing this.

Not sure about the tooltips for badge visibility. Should those stay on the labels?

@zneix zneix self-requested a review December 28, 2021 22:23
@Mm2PL Mm2PL self-requested a review December 28, 2021 22:39
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

I have some code comments. I feel like there isn't much to notice that these elements have a tooltip associated with them. Maybe adding an 🛈 icon would work or as some apps do it: a question mark with a dashed/dotted underline.

src/widgets/settingspages/GeneralPageView.hpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Outdated 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/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPageView.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.

Took me long enough, thank you for doing the heavy lifting! 🙇

@pajlada pajlada enabled auto-merge (squash) October 30, 2022 11:49
@pajlada pajlada merged commit fa93d63 into Chatterino:master Oct 30, 2022
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

{
auto check = new QCheckBox(text);
this->addToolTip(*check, toolTipText);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'toolTipText' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]

src/widgets/settingspages/GeneralPageView.cpp:12:

+ #include <utility>
Suggested change
this->addToolTip(*check, toolTipText);
this->addToolTip(*check, std::move(toolTipText));

@@ -362,4 +393,23 @@
}
}

void GeneralPageView::addToolTip(QWidget &widget, QString text) const
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'addToolTip' can be made static [readability-convert-member-functions-to-static]

Suggested change
void GeneralPageView::addToolTip(QWidget &widget, QString text) const
void GeneralPageView::addToolTip(QWidget &widget, QString text)

src/widgets/settingspages/GeneralPageView.hpp:205:

-     void addToolTip(QWidget &widget, QString text) const;
+     static void addToolTip(QWidget &widget, QString text) ;

bool inverse = false);
ComboBox *addDropdown(const QString &text, const QStringList &items);
bool inverse = false, QString toolTipText = {});
ComboBox *addDropdown(const QString &text, const QStringList &items,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'chatterino::GeneralPageView::addDropdown' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

    ComboBox *addDropdown(const QString &text, const QStringList &items,
              ^

src/widgets/settingspages/GeneralPageView.cpp:135: the definition seen here

ComboBox *GeneralPageView::addDropdown(const QString &text,
                           ^

src/widgets/settingspages/GeneralPageView.hpp:102: differing parameters are named here: ('items'), in definition: ('list')

    ComboBox *addDropdown(const QString &text, const QStringList &items,
              ^

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.

Description when you hover over a setting
6 participants