Qt UI: Correctly load icon from absolute path [SLE-15-SP4]

[shundhammer]

Target Release

This is for SLE-15 SP4. See below for the merge PRs to SLE-15-SP5 and to master / Factory / TW.

Bugzilla

https://bugzilla.suse.com/show_bug.cgi?id=1210591

Problem

Loading icons from a fixed path didn't work: There was never an icon on the screen.

It worked with icons from the desktop theme and with compiled-in icons, but not with anything else; in particular not with icons from a full path.

Cause

The fallback cascade that tried to load icons relied on checking QIcon::isNull(), assuming that a failed attempt to load an icon would result in that function returning true as one would expect.

libyui/libyui-qt/src/YQUI.cc

Lines 694 to 720 in 9cc9049

	QIcon YQUI::loadIcon( const string & iconName ) const
	{
	QIcon icon;
	const QString resource = ":/";
	if ( QIcon::hasThemeIcon( iconName.c_str() ) )
	{
	yuiDebug() << "Trying theme icon from: " << iconName << std::endl;
	icon = QIcon::fromTheme( iconName.c_str(), QIcon( resource + iconName.c_str() ) );
	}
	if ( icon.isNull() )
	{
	yuiDebug() << "Trying icon from resource: " << iconName << std::endl;
	icon = QIcon( resource + iconName.c_str() );
	}
	if ( icon.isNull() )
	{
	yuiDebug() << "Trying icon from path: " << iconName << std::endl;
	icon = QIcon( iconName.c_str() );
	}
	if ( icon.isNull() )
	yuiWarning() << "Couldn't load icon: " << iconName << std::endl;
	return icon;
	}

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;
}

But that assumption is wrong. In reality, that QIcon::isNull() has a really bizarre interpretation what it means for a QIcon to be null:

https://doc.qt.io/qt-5/qicon.html#isNull

bool QIcon::isNull() const

Returns true if the icon is empty; otherwise returns false.

An icon is empty if it has neither a pixmap nor a filename.

Note: Even a non-null icon might not be able to create valid pixmaps, eg. if the file does not exist or cannot be read.

Fix

Don't rely on that QIcon::isNull() function; it almost never returns a useful result.

After some experimentation, it turned out that QIcon::availableSizes() which returns a list of sizes for that icon that the loaded file(s) can generate is an empty list (size 0) if the icon could not be loaded, and non-empty (size > 0) if it was successful.

Added a function with a speaking name for that check so it becomes clear what's going on.

Test

Manual test with the Table-icons.rb UI example (after adapting the path to use one that actually exists and has icons) from yast2-ycp-bindings:

https://github.com/yast/yast-ycp-ui-bindings/blob/master/examples/Table-icons.rb

diff --git a/examples/Table-icons.rb b/examples/Table-icons.rb
index 22b2994..1140ca3 100644
--- a/examples/Table-icons.rb
+++ b/examples/Table-icons.rb
@@ -8,7 +8,7 @@ module Yast
 
 
 
-      @iconBasePath = "/usr/share/YaST2/theme/current/icons"
+      @iconBasePath = "/usr/share/icons/hicolor"
 
       UI.OpenDialog(
         VBox(
@@ -52,9 +52,9 @@ module Yast
         item = Item(
           Id(iconName),
           iconName,
-          term(:cell, term(:icon, Ops.add("22x22/apps/", iconName))),
-          term(:cell, term(:icon, Ops.add("32x32/apps/", iconName))),
-          term(:cell, term(:icon, Ops.add("48x48/apps/", iconName)))
+          term(:cell, term(:icon, @iconBasePath + "/22x22/apps/" + iconName)),
+          term(:cell, term(:icon, @iconBasePath + "/32x32/apps/" + iconName)),
+          term(:cell, term(:icon, @iconBasePath + "/48x48/apps/" + iconName))
         )
         # y2debug( "Item: %1", item );
         @itemList = Builtins.add(@itemList, item)

Related PRs

• Merge to SLE-15-SP5: Qt UI: Correctly load icon from absolute path [SLE-15-SP5] #95
• Merge to master: Qt UI: Correctly load icon from absolute path [master]  #96

[lslezak]

LGTM