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

Use palette colors in pieces bars #13256

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

jagannatharjun
Copy link
Member

No description provided.

@jagannatharjun
Copy link
Member Author

jagannatharjun commented Aug 15, 2020

windows
windows-pieces

ubuntu
image

ubuntu theme
ubuntu-bar

@FranciscoPombal FranciscoPombal added the GUI GUI-related issues/changes label Aug 15, 2020
Chocobo1
Chocobo1 previously approved these changes Aug 16, 2020
Chocobo1
Chocobo1 previously approved these changes Aug 16, 2020
Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

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

👍
Good job!
There are some minor comments only...

P.S. Ideally we should get rid from all hardcoded colors, styles, etc.

@@ -181,7 +181,7 @@ void PiecesBar::paintEvent(QPaintEvent *)
QPainter painter(this);
QRect imageRect(borderWidth, borderWidth, width() - 2 * borderWidth, height() - 2 * borderWidth);
if (m_image.isNull()) {
painter.setBrush(Qt::white);
painter.setBrush(palette().base());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you use backgroundColor() here?

@@ -127,7 +127,7 @@ bool PieceAvailabilityBar::updateImage(QImage &image)
}

if (m_pieces.empty()) {
image2.fill(Qt::white);
image2.fill(palette().color(QPalette::Base));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you use backgroundColor() here?

@@ -120,7 +120,7 @@ bool DownloadedPiecesBar::updateImage(QImage &image)
}

if (m_pieces.isEmpty()) {
image2.fill(Qt::white);
image2.fill(palette().color(QPalette::Base));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you use backgroundColor() here?

@@ -158,6 +158,10 @@ void PieceAvailabilityBar::clear()

QString PieceAvailabilityBar::simpleToolTipText() const
{
return tr("White: Unavailable pieces") + '\n'
+ tr("Blue: Available pieces") + '\n';
const QString colorBoxBorderColor = palette().color(QPalette::ToolTipText).name();
Copy link
Member

Choose a reason for hiding this comment

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

This color is common to all PiecesBar subclasses, isn't it? Then I would make it accessible via its interface like other such colors (e.g. backgroundColor()).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is not a property of Pieces Bar so I would like to leave it as it is.

Copy link
Member

Choose a reason for hiding this comment

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

What is the difference with the other colors that are set there?

@Chocobo1 Chocobo1 merged commit 4539c67 into qbittorrent:master Aug 23, 2020
@Chocobo1
Copy link
Member

@jagannatharjun
Thank you!

@jagannatharjun jagannatharjun deleted the pieces-style branch August 23, 2020 05:03
IceCodeNew pushed a commit to IceCodeNew/qBittorrent-Enhanced-Edition that referenced this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI GUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants