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 four new icons to the View menu #20406

Closed
wants to merge 1 commit into from

Conversation

buckmelanoma
Copy link

This pull request adds four new icons to actions in the View menu that are currently missing icons:

  • RSS Reader
  • Filters Sidebar
  • Speed in Title Bar
  • Search Engine

Before:
image

After:
image

@luzpaz
Copy link
Contributor

luzpaz commented Feb 11, 2024

LGTM, untested

Comment on lines +176 to +179
m_ui->actionRSSReader->setIcon(UIThemeManager::instance()->getIcon(u"rss"_s));
m_ui->actionShowFiltersSidebar->setIcon(UIThemeManager::instance()->getIcon(u"view-sidetree"_s));
m_ui->actionSpeedInTitleBar->setIcon(UIThemeManager::instance()->getIcon(u"speedometer"_s));
m_ui->actionSearchWidget->setIcon(UIThemeManager::instance()->getIcon(u"search"_s));
Copy link
Member

@glassez glassez Feb 15, 2024

Choose a reason for hiding this comment

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

IIRC, there are no such icons in qBittorrent sources (except "speedometer" one).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've just tested this on windows & here are the results:

Before This PR:
checkboxes/icons are all aligned vertically
Screenshot 2024-02-16 112209

With this PR:
Only the speedometer is added & it takes the place of the checkbox!
Screenshot 2024-02-16 112243

speedometer - clicked/enabled
Screenshot 2024-02-16 112310

Copy link
Contributor

Choose a reason for hiding this comment

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

@buckmelanoma why are the results different than yours ?

Copy link
Member

Choose a reason for hiding this comment

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

@buckmelanoma why are the results different than yours ?

Because they use icons from Linux theme.

@sledgehammer999
Copy link
Member

From the screens provided by @xavier2k6 it is clear that the checkboxes are replaced by the icons (when available by the icon theme). IMO, a checkbox provides a much better indication to the user that the item is toggleable and the state of the toggle.
On that grounds, I propose to not merge this PR.

@glassez
Copy link
Member

glassez commented Mar 3, 2024

On that grounds, I propose to not merge this PR.

I wouldn't approve it anyway. #20406 (comment)

Copy link

github-actions bot commented May 3, 2024

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label May 3, 2024
Copy link

This PR was closed because it has been stalled for some time with no activity.

@github-actions github-actions bot closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants