Differential D14856

Search: Add workaround for missing icons in Gnome icon-theme
ClosedPublic
Actions

Authored by sars on Aug 15 2018, 8:43 AM.

Details

Reviewers
dhaumann
broulik
Group Reviewers
Kate
Commits
R39:e63efbdb8e4c: Search: Add workaround for missing icons in Gnome icon-theme
Test Plan

Install Gnome-icon-theme and start kate using that theme. Open the search bar and see that the icons are displayed.

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sars created this revision.Aug 15 2018, 8:43 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 15 2018, 8:43 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
sars requested review of this revision.Aug 15 2018, 8:43 AM
sars edited the test plan for this revision. (Show Details)Aug 15 2018, 8:47 AM
sars added a reviewer: Kate.
dhaumann accepted this revision.Aug 15 2018, 8:53 AM
dhaumann added a subscriber: dhaumann.

Albeit a bi hacky, if that helps Kate users on Gnome, I am fine with this.

This revision is now accepted and ready to land.Aug 15 2018, 8:53 AM
broulik added a subscriber: broulik.Aug 15 2018, 8:54 AM
broulik added inline comments.
src/search/katesearchbar.cpp
1332

hasThemeIcon)foo) just does !QIcon::fromTheme(foo).isNull() so you might as well just do the same and safe a lookup

dhaumann added inline comments.Aug 15 2018, 8:57 AM
src/search/katesearchbar.cpp
1332

What would also be possible:

QIcon mutateIcon = QIcon::fromTheme(QStringLiteral("games-config-options"), QIcon::fromTheme(QStringLiteral("preferences-system")));

The 2nd parameter is the fallback Icon. This would be much shorter.

sars updated this revision to Diff 39772.Aug 15 2018, 9:15 AM

use fromTheme("foo", fromTheme("fall-back"));

sars marked an inline comment as done.Aug 15 2018, 9:15 AM
broulik accepted this revision.Aug 15 2018, 9:23 AM
Closed by commit R39:e63efbdb8e4c: Search: Add workaround for missing icons in Gnome icon-theme (authored by sars). · Explain WhyAug 15 2018, 9:26 AM
This revision was automatically updated to reflect the committed changes.
bruns added a subscriber: bruns.Aug 15 2018, 4:07 PM
bruns added inline comments.
src/search/katesearchbar.cpp
1332

Doesn't this evaluate the fallback always, even if not needed?
IIRC QIcon::fromTheme is not really lightweight ...

dhaumann added a comment.Aug 15 2018, 6:05 PM

True... Is this a performance issue? ;)

dfaure added a subscriber: dfaure.Aug 15 2018, 7:03 PM

I don't think so, because QIcon::fromTheme just creates an icon engine. The actual lookup happens when the first call to isNull or paint (etc) is made. None of which are going to be done on the fallback icon if the main icon can be found.

So this looks fine to me, unless a profiler or strace says otherwise.

bruns added a comment.Aug 16 2018, 12:30 AM
In D14856#309714, @dfaure wrote:

I don't think so, because QIcon::fromTheme just creates an icon engine. The actual lookup happens when the first call to isNull or paint (etc) is made. None of which are going to be done on the fallback icon if the main icon can be found.

So this looks fine to me, unless a profiler or strace says otherwise.

strace confirms you are correct

Revision Contents
Changeset List

PathPackages
Msrc/search/katesearchbar.cpp (14 lines)

Diff 39777

View Options

src/search/katesearchbar.cpp

Loading...