Support symbolic SVG icons

[tsujan]

Fixes https://github.com/lxde/lxqt/issues/1181.

That means changing their colors so that they always have a high contrast with their backgrounds. Symbolic icons should have SVG stylesheets (as the Breeze icons do) and the style engine should support them (as the Breeze and Kvantum styles do).

The code is stolen from KDE's kiconloader.cpp (see the comments).

Currently Panel and those LXQt apps that use stylesheets will show the wrong icon color if their stylesheets have a high contrast with the active Qt style. This issue should be fixed in their codes before this PR is merged.

[palinek]

1. the SVG processing should be done only in case when the theme states it (FollowsColorScheme=true)
2. we should allow the end-user to choose, if the "FollowsColorScheme" processing should be done

[palinek]

@tsujan I can have a look in those 2 mentioned points... if you wish

[tsujan]

I can have a look in those 2 mentioned points... if you wish

That would be great. Thanks!

the SVG processing should be done only in case when the theme states it

I disagree and think it's a redundant KDE check. KDE has the concept of "color scheme"; we don't (I remember that I explained that somewhere a year ago).

Yes, we stole the code from them but that doesn't mean we should follow them in each step. On the one hand, this isn't a CPU-intensive process for user to have an option about it. On the other hand, most KDE users don't know about "FollowsColorScheme" and If an SVG icon set doesn't have SVG stylesheets, it won't follow the app palette (the so-called KDE "color scheme") automatically.

[tsujan]

A long story made short, I mean that "FollowsColorScheme" is a KDE concept.

[palinek]

If an SVG icon set doesn't have SVG stylesheets, it won't follow the app palette (the so-called KDE "color scheme") automatically.

But the whole XML processing is needless. Therefore IMO it is good to follow the "FollowsColorScheme"... even if it is a KDE extension.

[tsujan]

xmlReader.qualifiedName() == QLatin1String("style") and xmlReader.attributes().value(QLatin1String("id")) == QLatin1String("current-color-scheme") would be false without SVG stylesheet and I saw no difference with my own SVG icons, which don't have stylesheets. The code is really fast.

[tsujan]

As a matter of fact, the process never reaches these lines, not even with svg icons without stylesheet:

    }
    else
        icn = svgIcon;

So, there's no redundant calculation.

[palinek]

The code is really fast.

It may be in global scope, but

1. It does depend on the structure of the XML
2. This (needless) processing (XML DOM parsing) gives our engine performance penalty. (I'm sure you agree, that checking one bool variable must be faster than parsing whole XML and checking some DOM nodes)

[tsujan]

Please read my last comment above yours. We commented simultaneously ;)

[tsujan]

In other words, this is needed without stylesheet:

                else if (xmlReader.tokenType() != QXmlStreamReader::Invalid)
                    writer.writeCurrentToken(xmlReader);

No redundant processing!

[tsujan]

checking one bool variable must be faster than parsing whole XML and checking some DOM nodes

Even without this patch, the svg was processed as an xml file. The detail just wasn't in our code.

[palinek]

checking one bool variable must be faster than parsing whole XML and checking some DOM nodes

Even without this patch, the svg was processed as an xml file. The detail just wasn't in our code.

Of course, but now there is one XML parsing more.

[tsujan]

Which parsing do you mean?

[palinek]

In other words, this is needed without stylesheet:

else if (xmlReader.tokenType() != QXmlStreamReader::Invalid)
writer.writeCurrentToken(xmlReader);

No redundant processing!

And this is completely wrong assumption -> the XML parser must have been read/parsed the file

[tsujan]

@palinek Remove icn = svgIcon; and you'll see svg icons without stylesheet will be shown correctly. Then also remove

                else if (xmlReader.tokenType() != QXmlStreamReader::Invalid)
                    writer.writeCurrentToken(xmlReader);

and you'll see no icon.

[palinek]

Remove icn = svgIcon; and you'll see svg icons without stylesheet will be shown correctly. Then also remove

I'm not saying, that it's not working. I'm just saying, that the processing is needles for icons from themes, that do not "FollowsColorScheme"

[tsujan]

the XML parser must have been read/parsed the file

Where?

[palinek]

... just had a look into the doc. The QXmlStreamReader is an SAX parser (not DOM)... so better but the processing is still there.

[tsujan]

QXmlStreamReader is a faster and more convenient replacement for Qt's own SAX parser.... In some cases it might also be a faster and more convenient alternative for use in applications that would otherwise use a DOM tree ....

[tsujan]

There's one thing. I think this line

     if (svgIcon.isNull())
          svgIcon = QIcon(filename);

might be moved and become

     if (svgIcon.isNull())
          svgIcon = icn;

[tsujan]

More specifically, these lines before return:

    if (!icn.isNull())
        svgIcon = icn;
    else {
        if (svgIcon.isNull())
            svgIcon = QIcon(filename);
        icn = svgIcon;
    }

EDITED!

[tsujan]

Or:

    QPixmap px;
    if (!img.isNull() && px.convertFromImage(img)) {
        // Do not return the pixmap now but get the icon from it
        // for QIcon::pixmap() to handle states and modes,
        // especially the disabled mode.
        svgIcon = QIcon(px);
    }

    if (svgIcon.isNull())
        svgIcon = QIcon(filename);

    return svgIcon.pixmap(size, mode, state);

This means that there's a redundant reading for icons without stylesheet because those SVG files are read again when svgIcon isn't null, while there's no need to read them again because their color won't change. In this sense, the code should be changed. FollowsColorScheme would be the last resort.

[tsujan]

OK! You're right. My hypotheses were wrong. svgIcon is parsed before. I'll try to add FollowsColorScheme.

[palinek]

OK! You're right. My hypotheses were wrong. svgIcon is parsed before. I'll try to add FollowsColorScheme.

I'm allready on it... but found some problem outside this PR.

[tsujan]

I'm allready on it... but found some problem outside this PR.

Great! Let's work on it independently and see what comes out.

[tsujan]

@palinek I think the code should be placed inside XdgIconLoaderEngine::pixmap(). Then, FollowsColorScheme could be easily respected. I'll add a commit after testing.

EDIT: No, wasn't that easy.

[tsujan]

All right. I succeed in considering FollowsColorScheme by placing the code inside XdgIconLoaderEngine::pixmap() and using XdgIconLoader::instance()->theme().

However, there's a more important matter. If FollowsColorScheme is true, the code shouldn't create svgIcon and then correct it. It seems that the structure of xdgiconloader.cpp should be changed, although I prefer to be wrong in this.

My brain's temperature has got high. Will think about it later, hoping that @palinek might find a simple solution.

EDIT: Actually I found three ways of doing it but the above-mentioned one seemed to be the best.

[tsujan]

Superseded by https://github.com/lxde/libqtxdg/pull/121