Issues with SVG icon themes

[ghost]

Hello, when we use dnfdragora-updater while having an icon theme including SVG files, we can't see any notification. No problems with PNG icon themes. dnfdragora devs are thinking that it's a bug with libyui.

-> dnfdragora issue #82

Thanks :)

[hellcp]

Does that happen regardless of GTK or Qt client, or does it happen in both?

[ghost]

Only tried with GTK client.

[Conan-Kudo]

It happens with both Plasma and GNOME.

[hellcp]

That's fun 🙄, I will take a look at it later.

[shundhammer]

Do we have a minimalistic ("hello world"-style) test program that shows the problem?

Since this works just fine in libyui-qt, I find it hard to believe that the problem is in libyui.

[anaselli]

@shundhammer i think you are right, but i would like to focus here (or please siggest the right place) how icons are loaded. I mean which is the right way to manage them? maybe the order should be if icon (for instance using YItem) has full path name then use it to load icon, otherwise try to load it from theme. I will provide an example soon, but if we use a Selection Box for Qt that doesn't work if full path is given, while for gtk works only with full path. Can we have one policy to be shared? So that we can align all the icon to be loaded behavior. What do you think?

[shundhammer]

I thought the icon loader already takes absolute paths into account. Let me check.

[anaselli]

Great thanks, but to have same behaviour in Gtk it would be nice to know which way to follow for instance with:
new YItem("label",

• "fullpath/icon/icon.png" : load icon and empty icon if not exists
• "icon theme name": get from theme and empty iconif not exists
• "icon.png" or "icon.svg" without path what to do? remove extension and try with theme? because i think ./icon.png is non sense...

WDYT?

[shundhammer]

Without path IMHO only an icon from the icon theme makes sense; otherwise you'd have to rely on the process's current directory or silently add a magic path. We did that in the past, and IMHO it didn't really work well (even less when people started creating magic symlinks to the current theme which sometimes were broken).

If you absolutely want the current directory, using "./icon.png" or "./icon.svg" would still be possible.

I am not 100% sure about "icon.png" vs. "icon.svg". Should we really only use the desktop's icon theme if there isn't even a filename extension, i.e. only if it's "icon", not "icon.png" or "icon.svg"? Can we rely on icon themes automagically selecting the right one? Are there icon themes that offer both a .svg and a .png?

I don't know. Maybe @hellcp can offer some expertise here.

[anaselli]

I Would follow freedesktop directive there, i.e. no extensions :)

BTW I don't know if that is the right way though in mga-qt i checked if icon is in theme first here or loaded from path. As said maybe if full path is given that should be forced to be loaded without caring if it is also into a theme...

[shundhammer]

Sure, if we can avoid them, no extensions.

But the same mechanism (or at least a very similar one) is also used for loading other images; think about the world map for the time zone selector, to name just one.

[anaselli]

yes, i think i have to fix that in yui-gtk since it does not work so well if you refer to createImage :) But can that loaded from theme too? I'm pretty ignorant on themes

[anaselli]

As promised here is an example from SelexionBox1 one into git.
I assumed to have "/usr/share/icons/hicolor/32x32/actions/document-share.png" on the system change it eventually.

Qt outupt:

Gtk:

As you can see they work differently and both are wrong in something :D

[shundhammer]

https://github.com/libyui/libyui-qt/blob/master/src/YQUI.cc#L708

QIcon YQUI::loadIcon( const string & iconName ) const
{
    QIcon icon;
    const QString resource = ":/";

    if ( QIcon::hasThemeIcon( iconName.c_str() ) )
    {
	yuiDebug() << "Trying theme icon from: " << iconName << endl;
	icon = QIcon::fromTheme( iconName.c_str() );
    }

    if ( icon.isNull() )
    {
	yuiDebug() << "Trying icon from resource: " << iconName << endl;
	icon = QIcon( resource + iconName.c_str() );
    }

    if ( icon.isNull() )
    {
	yuiDebug() << "Trying icon from path: " << iconName << endl;
	icon = QIcon( iconName.c_str() );
    }

    if ( icon.isNull() )
	yuiWarning() << "Couldn't load icon: " << iconName << endl;

    return icon;
}

What this does is it tries several locations in this order:

• From the desktop theme
• From compiled-in Qt resources
• From the filesystem

To be successful from the desktop theme, iconName should not include a path and probably also not a filename extension (but that may be different for some themes; I don't know).

If it is not found there, it uses compiled-in Qt resources which is very much like a small filesystem that is (by a special Qt resource compiler) shrink-wrapped into a binary blob and then included (via old C/C++ #include) as a hexdump into the code. This can also specify aliases so a name myicon is translated into :/icons/myicon.svg in the XML description file for that Qt resource file:

https://github.com/libyui/libyui-qt/blob/master/src/qt_icons.qrc

<!DOCTYPE RCC>
<RCC version="1.0">
	<qresource>
		<file alias="checkbox-off">icons/checkbox-off.png</file>
		<file alias="checkbox-on">icons/checkbox-on.png</file>
		<file alias="checkbox-auto-selected">icons/checkbox-auto-selected.png</file>
		<file alias="computer">icons/computer.svg</file>
		<file alias="configure">icons/configure.svg</file>
		...
		...
	</qresource>
</RCC>

I.e. that this may or may not have a filename extension and/or a path. An absolute path for Qt resources would start with :/; this is what resource + iconName() does.

If all that fails, it falls back to the filesystem; of course it makes sense to use an absolute path for that case to ensure that the file can be located, regardless of the current working directory of the process.

[shundhammer]

I know we have a YIconLoader in libyui:

https://github.com/libyui/libyui/blob/master/src/YIconLoader.h

but AFAICs it is no longer used by libyui-qt since @hellcp 's changes to use icons from the current desktop's theme whenever possible.

@hellcp has become our de facto art director for YaST on openSUSE (and thus also very much for SLE products). For any more details about the reasoning and what impact any changes in this area might have, he is the right person to ask.

[shundhammer]

@hellcp , any input from your side?

[anaselli]

Here i've shown an example on how i managed, that doesn't mean doing that but wanted to talk about my thoughts

https://github.com/manatools/libyui-mga-qt/blob/master/src/YMGAQMenuBar.cc#L112-L128

    QItemAction *action = new QItemAction(item, this);
    if (item->hasIconName())
    {
      QString qtIcon = fromUTF8( yitem->iconName() );
      QString icon_name = QFileInfo( qtIcon ).baseName();
      if ( QIcon::hasThemeIcon( icon_name ) )

That is not so right as seem imo, if icon is into icon theme it will be loaded from there also if there is a full path, so them wins even if we wanted to test new icon locally. Note also the fromUTF8 call, i seem to recall i experienced some problems if local directory name has special characters.
Anyway said that I'd expect the code you quoted, reported here below for convenience, falling the same:

    if ( QIcon::hasThemeIcon( iconName.c_str() ) )
    {
	yuiDebug() << "Trying theme icon from: " << iconName << endl;
	icon = QIcon::fromTheme( iconName.c_str() );
    }

Let's see the second part of my mga commit
https://github.com/manatools/libyui-mga-qt/blob/master/src/YMGAQMenuBar.cc#L122-L128

          QPixmap pixmap( qtIcon );

          if ( !pixmap.isNull() )
              action->setIcon( QIcon( pixmap ) );

QPixmat should load the icon by full path name, passing QPixmap to QIcon should solve the absolute path icon load. However that should be performed if the icon name has no extension/path to avoid the above mistake wdyt?

[anaselli]

A possible Qt solution could be adding QFileInfo to include files:

QIcon YQUI::loadIcon( const string & iconName ) const
{
    QIcon icon;
    const QString resource = ":/";
    QString qtIcon = fromUTF8( iconName );
    QFileInfo icon_fi = QFileInfo( qtIcon );

    if (icon_fi.exists())
    {
	QPixmap pixmap( qtIcon );

	if ( !pixmap.isNull() )
	    icon = QIcon( pixmap );
    }

    if ( icon.isNull() && QIcon::hasThemeIcon( qtIcon) )
    {
	yuiDebug() << "Trying theme icon from: " << iconName << endl;
	icon = QIcon::fromTheme( qtIcon );
    }

    if ( icon.isNull() )
    {
	yuiDebug() << "Trying icon from resource: " << iconName << endl;
	icon = QIcon( resource + qtIcon );
    }

    if ( icon.isNull() )
    {
	yuiDebug() << "Trying icon from path: " << iconName << endl;
	icon = QIcon( qtIcon );
    }

    if ( icon.isNull() )
	yuiWarning() << "Couldn't load icon: " << iconName << endl;

    return icon;
}

which produces as output for my example:

If a solution like that can be acceptable i could try to follow a similar way in yui-gtk to have a single point of change for all icons as well...