Question about commit

[Łukasz Wojniłowicz]

Dnia poniedziałek, 12 lutego 2018 08:09:37 CET Pino Toscano pisze:
> Hi,
> 
> In data sabato 10 febbraio 2018 21:18:19 CET, Thomas Baumgart ha scritto:
> > this commit on the 5.0 branch
> > 
> > commit 9b27c432ef80c9d4a35bc9dff37619cde9673343
> > Author: Pino Toscano <pino at kde.org>
> > Date:   Thu Feb 8 22:16:47 2018 +0100
> > 
> >     cmake: install local icons in the local datadir
> >     
> >     Install all the local actions icons to the kmymoney data directory, to
> >     avoid polluting the global icon themes.
> >     
> >     Fixes commit d9cf5550c478d95d4faf312c1149f30829829516.
> > 
> > apparently causes that the icons are not shown anymore in the application.
> > Any idea of what to look for?
> 
> I see, the icon loading code uses QIcon, which does not know about
> local paths for icons. Can you please try the attached patch, which
> switches to KIconLoader?
> 
> Since kmymoney has a lot of own icons, I'd really avoid to install them
> in the global icon themes, with the risk of clashes with the themes
> themselves. Also, the above commit makes the situation as it was in
> kmymoney 4.x.
> 
> Thanks,

Hi Pino,

either something is not working on my setup or you didn't test this patch. 
It breaks whole icon system. In $HOME/.local/share/icons/ I've got custom icon 
theme which I don't use with KMyMoney, but after your patch it's loaded for it 
although I choose different icon theme for it.

I believe it's because you switched QIcon::fromTheme to KDE::icon which is not 
equal to each other.

Small hint No. 1: do not change code without knowing what it does.

In general I confirm Thomas' observation about icon deficiencies.

Small hint No. 2: please don't commit changes "5 minutes before app's release" 
without consulting it with anyone actively involved in the development

> Since kmymoney has a lot of own icons, I'd really avoid to install them
> in the global icon themes, with the risk of clashes with the themes
> themselves. 

Although I understand concept of clash, I don't understand why it should 
happen here or why it should ever bring bad outcome.

1) icons we ship don't exist in themes, hence we ship them, so what it should 
clash with?

2) if other app (e.g. Skrooge) ships icons with the same name as ours and a 
clash occurs then it's nothing bad either, because we don't need the exact 
icon, that we ship, and nothing bad will happen if we use the icon, which e.g. 
Skrooge has overwritten.

3) installing icons in local directory is making them available only to single 
user on the computer, which in my opinion is bad for multi-user setups.

I would like to revert your patch and install missing icons globally unless 
you come up with better solution.

Cheers
Łukasz