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 better icons for RSS articles #20587

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Conversation

glassez
Copy link
Member

@glassez glassez commented Mar 21, 2024

Closes #20579.

@glassez glassez added RSS RSS-related issues/changes Look and feel Affect UI "Look and feel" only without changing the logic GUI GUI-related issues/changes labels Mar 21, 2024
@glassez glassez added this to the 5.0 milestone Mar 21, 2024
@glassez glassez requested a review from a team March 21, 2024 13:07
@glassez glassez marked this pull request as ready for review March 21, 2024 13:07
xavier2k6
xavier2k6 previously approved these changes Mar 21, 2024
@xavier2k6
Copy link
Member

xavier2k6 commented Mar 21, 2024

@glassez I just noticed....you are keeping the alternative name to just sphere & not sphere/sphere2 like it was in 4.2.x branch.

if (article->isRead()) {
item->setData(Qt::ForegroundRole, QPalette().color(QPalette::Inactive, QPalette::WindowText));
item->setData(Qt::DecorationRole, QIcon(":/icons/sphere.png"));
}
else {
item->setData(Qt::ForegroundRole, QPalette().color(QPalette::Active, QPalette::Link));
item->setData(Qt::DecorationRole, QIcon(":/icons/sphere2.png"));

Is that deliberate?

src/icons/icons.qrc Outdated Show resolved Hide resolved
@glassez
Copy link
Member Author

glassez commented Mar 21, 2024

@glassez I just noticed....you are keeping the alternative name to just sphere & not sphere/sphere2 like it was in 4.2.x branch.

if (article->isRead()) {
item->setData(Qt::ForegroundRole, QPalette().color(QPalette::Inactive, QPalette::WindowText));
item->setData(Qt::DecorationRole, QIcon(":/icons/sphere.png"));
}
else {
item->setData(Qt::ForegroundRole, QPalette().color(QPalette::Active, QPalette::Link));
item->setData(Qt::DecorationRole, QIcon(":/icons/sphere2.png"));

Is that deliberate?

Sure.
I've always been in favor of naming icons based on their IDs.

I'm sorry, I didn't understand the essence of the question right away. I get it now.

Now "alternative name" only makes sense for icons from the Linux system theme. I'm not even sure if there's "sphere" icon there. Note that the old code (4.2.x) used only the built-in icons for this purpose even if Linux system theme icons were enabled. Perhaps we could just delete this alternate name but I don't want to touch it now.

@glassez glassez modified the milestones: 5.0, 4.6.4 Mar 22, 2024
@glassez glassez merged commit 845f9a8 into qbittorrent:master Mar 22, 2024
13 checks passed
@glassez glassez deleted the rss-icons branch March 22, 2024 15:47
glassez added a commit to sledgehammer999/qBittorrent that referenced this pull request Mar 22, 2024
@sledgehammer999
Copy link
Member

Please try to give some minimal text/reasoning to a PR you open.
Like what is the source of the new assets? Does it need mentioning in AUTHORS ?

@glassez
Copy link
Member Author

glassez commented Mar 22, 2024

Please try to give some minimal text/reasoning to a PR you open.

I supposed that it makes no sense to duplicate the information that is already in the linked Issue.

Like what is the source of the new assets? Does it need mentioning in AUTHORS ?

They were extracted from previous qBittorrent resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI GUI-related issues/changes Look and feel Affect UI "Look and feel" only without changing the logic RSS RSS-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect icons used for RSS articles
4 participants