Bug 364261 - Only "Breeze" iconset will be used, even with Oxygen configured
Summary: Only "Breeze" iconset will be used, even with Oxygen configured
Status: RESOLVED FIXED
Alias: None
Product: kaffeine
Classification: Applications
Component: general (show other bugs)
Version: 2.0.1
Platform: unspecified Linux
: NOR minor
Target Milestone: ---
Assignee: Mauro Carvalho Chehab
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-12 23:37 UTC by JanKusanagi
Modified: 2016-06-13 22:48 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JanKusanagi 2016-06-12 23:37:19 UTC
Kaffeine 2.0.3 uses Breeze icons in the menus, even when system's configured iconset is Oxygen.

I've found that in src/main.cpp, there is a function, iconThemeFunc() that seems to handle this.

The function starts with this 'if' statement, on line 159 of current master:

if ((QIcon::themeName() != QLatin1String("breeze")
	    && QIcon::themeName() != QLatin1String("oxgen"))
	    || QIcon::themeName().isEmpty()) {

where we can see it looks for "oxgen" theme, instead of "oxygen", so it will always default to "breeze" ;)

Thanks!


P.S.- I understand the decision to force either of these two iconsets, as stated right before that function:
"// The icon Kaffeine needs are either at breeze or oxygen themes"

but I really think this should be optional, or a recommendation, or something like that. I see no reason for people to have this program showing with a completely alien iconset (for them), just because it's assumed other iconsets are not complete enough.

What do you think?
Comment 1 Mauro Carvalho Chehab 2016-06-13 01:09:07 UTC
(In reply to JanKusanagi from comment #0)
> Kaffeine 2.0.3 uses Breeze icons in the menus, even when system's configured
> iconset is Oxygen.
> 
> I've found that in src/main.cpp, there is a function, iconThemeFunc() that
> seems to handle this.
> 
> The function starts with this 'if' statement, on line 159 of current master:
> 
> if ((QIcon::themeName() != QLatin1String("breeze")
> 	    && QIcon::themeName() != QLatin1String("oxgen"))
> 	    || QIcon::themeName().isEmpty()) {
> 
> where we can see it looks for "oxgen" theme, instead of "oxygen", so it will
> always default to "breeze" ;)

Sorry for the typo.

> P.S.- I understand the decision to force either of these two iconsets, as
> stated right before that function:
> "// The icon Kaffeine needs are either at breeze or oxygen themes"
> 
> but I really think this should be optional, or a recommendation, or
> something like that. I see no reason for people to have this program showing
> with a completely alien iconset (for them), just because it's assumed other
> iconsets are not complete enough.
> 
> What do you think?

The problem is that this window really relies on icons:
https://mchehab.fedorapeople.org/kaffeine_screenshots/kaffeine_live_tv.png

For TV configuration, channel scanning,  OPG, record schedule and Instant save:

src/dvb/dvbtab.cpp:     QAction *channelsAction = new QAction(QIcon::fromTheme(QLatin1String("video-television")), i18n("Channels"), this);
src/dvb/dvbtab.cpp:     QAction *epgAction = new QAction(QIcon::fromTheme(QLatin1String("view-list-details")), i18n("Program Guide"), this);
src/dvb/dvbtab.cpp:     QAction *osdAction = new QAction(QIcon::fromTheme(QLatin1String("dialog-information")), i18n("OSD"), this);
src/dvb/dvbtab.cpp:     QAction *recordingsAction = new QAction(QIcon::fromTheme(QLatin1String("view-pim-calendar")),
src/dvb/dvbtab.cpp:     instantRecordAction = new QAction(QIcon::fromTheme(QLatin1String("document-save")), i18n("Instant Record"), this);
src/dvb/dvbtab.cpp:     QAction *configureAction = new QAction(QIcon::fromTheme(QLatin1String("configure")),

If the above icons are not found, it will display names, instead, with reduces a lot the size of the screen view. AFAICT, the above icons are present only on Breeze or Oxygen.

An option would be to, instead, copy all icons that Kaffeine needs from Breeze (or Oxygen) and installing them as hicolor to work as fallback while other themes don't have those icons.

For now, I'll fix the typo.
Comment 2 Mauro Carvalho Chehab 2016-06-13 01:09:36 UTC
Git commit 50e47e91d507785823386ae24aad994e0416c843 by Mauro Carvalho Chehab.
Committed on 13/06/2016 at 01:07.
Pushed by mauroc into branch 'master'.

main: Fix Oxygen theme support

The Oxygen theme support was broken due to a typo. Fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

M  +2    -2    src/main.cpp

http://commits.kde.org/kaffeine/50e47e91d507785823386ae24aad994e0416c843
Comment 3 JanKusanagi 2016-06-13 01:18:51 UTC
Thanks for the superquick fix =)

> An option would be to, instead, copy all icons that Kaffeine needs from Breeze (or Oxygen) and installing them as hicolor to work as fallback while other themes don't have those icons.

That sounds very sensible. I actually use that in my (Qt-only) programs, using something like QIcon::fromtheme("something", QIcon(":/something.png")); and adding these fallback icons in a Qt resource file. This way, only the missing icons are replace by mine, while the rest are up to the user.

Anyway, thanks again! \o/
Comment 4 Mauro Carvalho Chehab 2016-06-13 16:16:46 UTC
(In reply to JanKusanagi from comment #3)
> Thanks for the superquick fix =)

Anytime.

> That sounds very sensible. I actually use that in my (Qt-only) programs,
> using something like QIcon::fromtheme("something",
> QIcon(":/something.png")); and adding these fallback icons in a Qt resource
> file. This way, only the missing icons are replace by mine, while the rest
> are up to the user.

I did that and removed the logic that forces the icons to be from either Breeze or Oxygen.
Now,  all icons have a fallback (the Breeze icon).
Comment 5 JanKusanagi 2016-06-13 22:48:17 UTC
Awesome! Thanks again!