Fix loading button icons from qrc
AbandonedPublic

Authored by nicolasfella on Mar 22 2020, 1:04 AM.

Details

Reviewers
mart
ervin
Group Reviewers
Plasma
Summary

Allow to use icon.source: "qrc:/testicon.png" in buttons.

QIcon only seems to accept :/foo.png, not qrc:/foo.png, whereas I can't pass :/foo.png from QML and need to use qrc:/foo.png and hack away the prefix.

Diff Detail

Repository
R858 Qt Quick Controls 2: Desktop Style
Branch
qrcicons
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26477
Build 26495: arc lint + arc unit
nicolasfella created this revision.Mar 22 2020, 1:04 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 22 2020, 1:04 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
nicolasfella requested review of this revision.Mar 22 2020, 1:04 AM
apol added a subscriber: apol.Mar 23 2020, 12:57 AM

Fixing QIcon would make sense but I'd say getting this in is not the worst thing either.

mart added inline comments.Mar 23 2020, 11:45 AM
plugin/kquickstyleitem.cpp
187

should it also catch the ":/foo" case?

  • Also handle ':/' prefix
nicolasfella retitled this revision from [WIP] Fix loading button icons from qrc to Fix loading button icons from qrc.May 6 2020, 7:53 PM
nicolasfella edited the summary of this revision. (Show Details)
nicolasfella added a reviewer: ervin.
nicolasfella marked an inline comment as done.May 6 2020, 8:06 PM
ervin added a comment.May 6 2020, 9:39 PM
In D28194#632629, @apol wrote:

Fixing QIcon would make sense but I'd say getting this in is not the worst thing either.

I don't think it's QIcon's fault to be honest. The pattern used in several places of (icon.name || icon.source) doesn't help, it makes the lower layer loose the information of an icon specified by name (thus going through theme loading) or by URL (thus going through direct loading). Due to that the lower layer ends up having code trying to guess in which situation it is.

plugin/kquickstyleitem.cpp
182

Note that this construct is duplicated three times in that file. It really calls for having a findIcon like that:

QIcon KQuickStyleItem::findIcon(const QVariant &iconProp) const
{
    if (iconProp.canConvert<QIcon>()) {
        return iconProp.value<QIcon>();
    } else if (iconProp.canConvert<QString>()) {
        const auto iconId = iconProp.toString();
        if (iconId.startsWith(QLatin1Char('/')) || iconId.startsWith(QStringLiteral(":/"))) {
            return QIcon(iconId);
        } else if (iconId.startsWith(QStringLiteral("file:"))) {
            return QIcon(QUrl(iconId).toLocalFile());
        } else if (iconId.startsWith(QStringLiteral("qrc:"))) {
            return QIcon(QLatin1Char(':') + QUrl(iconId).path());
        } else {
            return m_theme->iconFromTheme(iconId, m_properties[QStringLiteral("iconColor")].value<QColor>());
        }
    } else {
        return QIcon();
    }
}

And then using it for setting opt->icon and opt->currentIcon (combobox case). Yes, I did that locally and that definitely helped.