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

2018-08-15 Thread Stefan Brüns 
bruns added a comment.


  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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann, broulik
Cc: dfaure, bruns, broulik, dhaumann, kwrite-devel, kde-frameworks-devel, 
michaelh, kevinapavew, ngraham, demsking, cullmann, sars

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

2018-08-15 Thread David Faure 
dfaure added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann, broulik
Cc: dfaure, bruns, broulik, dhaumann, kwrite-devel, kde-frameworks-devel, 
michaelh, kevinapavew, ngraham, demsking, cullmann, sars

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

2018-08-15 Thread Dominik Haumann 
dhaumann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann, broulik
Cc: bruns, broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, demsking, cullmann, sars

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

2018-08-15 Thread Stefan Brüns 
bruns added inline comments.

INLINE COMMENTS

> katesearchbar.cpp:1332
> +// Gnome does not seem to have all icons we want, so we use 
> fall-back icons for those that are missing.
> +QIcon mutateIcon = 
> QIcon::fromTheme(QStringLiteral("games-config-options"), 
> QIcon::fromTheme(QStringLiteral("preferences-system")));
> +QIcon matchCaseIcon = 
> QIcon::fromTheme(QStringLiteral("format-text-superscript"), 
> QIcon::fromTheme(QStringLiteral("format-text-bold")));

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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann, broulik
Cc: bruns, broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, demsking, cullmann, sars

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

2018-08-15 Thread Kåre Särs 
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:e63efbdb8e4c: Search: Add workaround for missing icons in 
Gnome icon-theme (authored by sars).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14856?vs=39772&id=39777

REVISION DETAIL
  https://phabricator.kde.org/D14856

AFFECTED FILES
  src/search/katesearchbar.cpp

To: sars, #kate, dhaumann, broulik
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Kai Uwe Broulik 
broulik accepted this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann, broulik
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Kåre Särs 
sars marked an inline comment as done.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Kåre Särs 
sars updated this revision to Diff 39772.
sars added a comment.


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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14856?vs=39766&id=39772

REVISION DETAIL
  https://phabricator.kde.org/D14856

AFFECTED FILES
  src/search/katesearchbar.cpp

To: sars, #kate, dhaumann
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Dominik Haumann 
dhaumann added inline comments.

INLINE COMMENTS

> broulik wrote in katesearchbar.cpp:1332
> `hasThemeIcon)foo)` just does `!QIcon::fromTheme(foo).isNull()` so you might 
> as well just do the same and safe a lookup

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.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Kai Uwe Broulik 
broulik added inline comments.

INLINE COMMENTS

> katesearchbar.cpp:1332
> +// Gnome does not seem to have all icons we want, so we use 
> fall-back icons for those that are missing.
> +QIcon mutateIcon = 
> QIcon::hasThemeIcon(QStringLiteral("games-config-options")) ? 
> QIcon::fromTheme(QStringLiteral("games-config-options")) : 
> QIcon::fromTheme(QStringLiteral("preferences-system"));
> +QIcon matchCaseIcon = 
> QIcon::hasThemeIcon(QStringLiteral("format-text-superscript")) ? 
> QIcon::fromTheme(QStringLiteral("format-text-superscript")) : 
> QIcon::fromTheme(QStringLiteral("format-text-bold"));

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

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann
Cc: broulik, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Dominik Haumann 
dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

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

2018-08-15 Thread Kåre Särs 
sars added a reviewer: Kate.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars, #kate
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

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

2018-08-15 Thread Kåre Särs 
sars edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14856

To: sars
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

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

2018-08-15 Thread Kåre Särs 
sars created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
sars requested review of this revision.

TEST PLAN
  Install Gnome-icon-theme and start kate using that theme

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14856

AFFECTED FILES
  src/search/katesearchbar.cpp

To: sars
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann