Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
> On Mai 17, 2016, 2:29 nachm., Wolfgang Bauer wrote:
> > Sorry for coming late, I'm not subscribed to the list (and I'm not a
> > maintainer either), just noticed the commit.
> >
> > I have nothing against the patch per se, and it apparently won't cause the
> > wrong fallback to breeze either as reported in bug#360664.
> >
> > But: Won't we hit the same old problem again that applications won't find
> > their oxygen icons (where they might have installed them in the first
> > place) any more?
> > Which was the reason why the previous patch to change the default to breeze
> > got reverted.
> >
> > See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in
> > https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html.
> >
> > At least kdenlive still seems to be affected according to
> > http://bugzilla.opensuse.org/show_bug.cgi?id=975387...
> >
> > Sorry if I'm misunderstanding something here, just wanted to raise a
> > concern for the sake of good...
>
> Aleix Pol Gonzalez wrote:
> Then maybe Breeze should inherit Oxygen? Or just be extended to have such
> icons. KIconThemes it's the wrong place to enforce these.
>
> Wolfgang Bauer wrote:
> It's not about Breeze. AFAIK Breeze does inherit from Oxygen.
> The problem is that users can select other themes that don't inherit from
> Oxygen.
>
> I do agree that KIconThemes is the wrong place for this, but the problem
> won't be solved by ignoring it either
>
> Albert Astals Cid wrote:
> If apps install the icon into oxygen, the apps need to be fixed to
> install their icon to hicolor
Yes, of course.
But ideally the applications should be fixed first IMHO, before the icon loader
behavior (that has been established for years) is changed.
And in the case of kdenlive, it does install its own icons to hicolor.
Still, many icons are missing (like new/open/save) if the oxygen icon theme is
not installed. (The strange thing is that it actually uses breeze icons, but
they are missing if *oxygen* is not installed.)
Although, I actually tested this patch now, and kdenlive still is fine. And
actually this patch *fixes* the problem with missing icons in kdenlive if the
oxygen icon theme is not installed.
So, sorry for the noise.
Other applications I tried are missing icons when run outside of Plasma, but
that's nothing new and unrelated to this patch.
I will do some more testing *inside* a Plasma session though, with an
("incomplete") icon theme other than breeze or oxygen configured...
There's just one thing I still don't understand really:
the patch changes the default icon theme to hicolor AFAICS. Shouldn't this be
breeze instead then?
- Wolfgang
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127817/#review95528
---
On Mai 17, 2016, 12:23 nachm., Aleix Pol Gonzalez wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127817/
> ---
>
> (Updated Mai 17, 2016, 12:23 nachm.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kiconthemes
>
>
> Description
> ---
>
> We were using oxygen as the default theme, this meant that oxygen was
> processed by every application. This was fixed in the past by changing this
> setting to `breeze`, but this only brought new kinds of problems (e.g. review
> 127232).
>
> See bug 336739, bug 360664.
>
> By just using hicolor, we don't make the application fetch `oxygen` for
> little reason.
>
>
> Diffs
> -
>
> src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4
> src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c
>
> Diff: https://git.reviewboard.kde.org/r/127817/diff/
>
>
> Testing
> ---
>
> On dolphin:
> This has an improvement on callgrind 1556M/1185M instructions (30%
> improvement).
> KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by:
> 535K/509K
>
>
> Thanks,
>
> Aleix Pol Gonzalez
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
> On May 17, 2016, 12:29 p.m., Wolfgang Bauer wrote: > > Sorry for coming late, I'm not subscribed to the list (and I'm not a > > maintainer either), just noticed the commit. > > > > I have nothing against the patch per se, and it apparently won't cause the > > wrong fallback to breeze either as reported in bug#360664. > > > > But: Won't we hit the same old problem again that applications won't find > > their oxygen icons (where they might have installed them in the first > > place) any more? > > Which was the reason why the previous patch to change the default to breeze > > got reverted. > > > > See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in > > https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html. > > > > At least kdenlive still seems to be affected according to > > http://bugzilla.opensuse.org/show_bug.cgi?id=975387... > > > > Sorry if I'm misunderstanding something here, just wanted to raise a > > concern for the sake of good... > > Aleix Pol Gonzalez wrote: > Then maybe Breeze should inherit Oxygen? Or just be extended to have such > icons. KIconThemes it's the wrong place to enforce these. > > Wolfgang Bauer wrote: > It's not about Breeze. AFAIK Breeze does inherit from Oxygen. > The problem is that users can select other themes that don't inherit from > Oxygen. > > I do agree that KIconThemes is the wrong place for this, but the problem > won't be solved by ignoring it either If apps install the icon into oxygen, the apps need to be fixed to install their icon to hicolor - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95528 --- On May 17, 2016, 10:23 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated May 17, 2016, 10:23 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
> On Mai 17, 2016, 2:29 nachm., Wolfgang Bauer wrote: > > Sorry for coming late, I'm not subscribed to the list (and I'm not a > > maintainer either), just noticed the commit. > > > > I have nothing against the patch per se, and it apparently won't cause the > > wrong fallback to breeze either as reported in bug#360664. > > > > But: Won't we hit the same old problem again that applications won't find > > their oxygen icons (where they might have installed them in the first > > place) any more? > > Which was the reason why the previous patch to change the default to breeze > > got reverted. > > > > See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in > > https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html. > > > > At least kdenlive still seems to be affected according to > > http://bugzilla.opensuse.org/show_bug.cgi?id=975387... > > > > Sorry if I'm misunderstanding something here, just wanted to raise a > > concern for the sake of good... > > Aleix Pol Gonzalez wrote: > Then maybe Breeze should inherit Oxygen? Or just be extended to have such > icons. KIconThemes it's the wrong place to enforce these. It's not about Breeze. AFAIK Breeze does inherit from Oxygen. The problem is that users can select other themes that don't inherit from Oxygen. I do agree that KIconThemes is the wrong place for this, but the problem won't be solved by ignoring it either - Wolfgang --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95528 --- On Mai 17, 2016, 12:23 nachm., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated Mai 17, 2016, 12:23 nachm.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
> On May 17, 2016, 2:29 p.m., Wolfgang Bauer wrote: > > Sorry for coming late, I'm not subscribed to the list (and I'm not a > > maintainer either), just noticed the commit. > > > > I have nothing against the patch per se, and it apparently won't cause the > > wrong fallback to breeze either as reported in bug#360664. > > > > But: Won't we hit the same old problem again that applications won't find > > their oxygen icons (where they might have installed them in the first > > place) any more? > > Which was the reason why the previous patch to change the default to breeze > > got reverted. > > > > See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in > > https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html. > > > > At least kdenlive still seems to be affected according to > > http://bugzilla.opensuse.org/show_bug.cgi?id=975387... > > > > Sorry if I'm misunderstanding something here, just wanted to raise a > > concern for the sake of good... Then maybe Breeze should inherit Oxygen? Or just be extended to have such icons. KIconThemes it's the wrong place to enforce these. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95528 --- On May 17, 2016, 12:23 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated May 17, 2016, 12:23 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95528 --- Sorry for coming late, I'm not subscribed to the list (and I'm not a maintainer either), just noticed the commit. I have nothing against the patch per se, and it apparently won't cause the wrong fallback to breeze either as reported in bug#360664. But: Won't we hit the same old problem again that applications won't find their oxygen icons (where they might have installed them in the first place) any more? Which was the reason why the previous patch to change the default to breeze got reverted. See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html. At least kdenlive still seems to be affected according to http://bugzilla.opensuse.org/show_bug.cgi?id=975387... Sorry if I'm misunderstanding something here, just wanted to raise a concern for the sake of good... - Wolfgang Bauer On Mai 17, 2016, 12:23 nachm., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated Mai 17, 2016, 12:23 nachm.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/ --- (Updated May 17, 2016, 10:23 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit ad1ead84ee2c3a14363afdb048b3579f3d01fa18 by Aleix Pol to branch master. Repository: kiconthemes Description --- We were using oxygen as the default theme, this meant that oxygen was processed by every application. This was fixed in the past by changing this setting to `breeze`, but this only brought new kinds of problems (e.g. review 127232). See bug 336739, bug 360664. By just using hicolor, we don't make the application fetch `oxygen` for little reason. Diffs - src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c Diff: https://git.reviewboard.kde.org/r/127817/diff/ Testing --- On dolphin: This has an improvement on callgrind 1556M/1185M instructions (30% improvement). KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: 535K/509K Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95436 --- Ship it! Ok, if noone else wants to review, ship it (wait for 5.22 to be out) and we'll see if it makes people unhappy :D - Albert Astals Cid On May 2, 2016, 11:09 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated May 2, 2016, 11:09 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
> On May 4, 2016, 11:45 p.m., Albert Astals Cid wrote: > > +1 would i'd like someone else opinion. Ping? - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95188 --- On May 3, 2016, 1:09 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated May 3, 2016, 1:09 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127817: Don't make KIconThemes depend on Oxygen
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127817/#review95188 --- +1 would i'd like someone else opinion. - Albert Astals Cid On May 2, 2016, 11:09 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127817/ > --- > > (Updated May 2, 2016, 11:09 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kiconthemes > > > Description > --- > > We were using oxygen as the default theme, this meant that oxygen was > processed by every application. This was fixed in the past by changing this > setting to `breeze`, but this only brought new kinds of problems (e.g. review > 127232). > > See bug 336739, bug 360664. > > By just using hicolor, we don't make the application fetch `oxygen` for > little reason. > > > Diffs > - > > src/kiconloader.cpp 6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4 > src/kicontheme.cpp f8c0a37754848a74e42bba5866aa4ce0998bce7c > > Diff: https://git.reviewboard.kde.org/r/127817/diff/ > > > Testing > --- > > On dolphin: > This has an improvement on callgrind 1556M/1185M instructions (30% > improvement). > KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: > 535K/509K > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
