Correct hierarchy of fallbacks

[tsujan]

This is my idea of a practically correct hierarchy of fallbacks, in contrast to what XDG specification tells us.

The icon is searched in the theme and all of its parents. Then, if it is not found, the search is done for the first "dash fallback" in the theme and all of its parents. And so on for the second, third,... "dash fallbacks".

Anyone has right to hate this but I use it to have acceptable fallback icons under LXQt, in the same way I have them under KDE.

[luis-pereira]

@tsujan It seems OK to me. This change can have unforeseen consequences. I will test it for a couple of days.

[agaida]

@tsujan @luis-pereira i will use this too and go a step further - if ok for me i will add this to the fixes for Siduction after a few days to get more feedback eventually. We don't need to hurry right now but we should finally fix it - if tested enough and considered ok.

[tsujan]

@luis-pereira @agaida Thanks!

[palinek]

I personally have no opinion on this => both approaches are logical.

But we should mention, that we are violating the standard intentionally. At least in the source code like e.g. here

[tsujan]

To be clear as far as possible, I explain this with math symbols and an example. Supposing that X and Y are icon sets and X is active, this is what XDG specification gives us:

(X ⊂ Y) ∧ (gnome-disks ∉ X) ∧ (gnome ∈ X) ∧ (gnome-disks ∈ Y) ⇒ (gnome ∈ X)

And this is what the PR does:

(X ⊂ Y) ∧ (gnome-disks ∉ X) ∧ (gnome ∈ X) ∧ (gnome-disks ∈ Y) ⇒ (gnome-disks ∈ Y)

Now, gnome ∈ X may have the general look of other icons in X but is meaningless in this case. On the other hand, gnome-disks ∈ Y may not have the general look of other X icons but shows what this app really is.

The responsibility of gnome-disks ∉ X is on the shoulders of the creator of X; we shouldn't compensate for the incompleteness of an icon set with a meaningless icon -- that wouldn't be logical -- but try to provide the user with a meaningful one.

[agaida]

ok, we will need a bump - but thats fine i guess

- (c++)"XdgIconLoader::findIconHelper(QString const&, QString const&, QStringList&) const@Base" 2.0.0
+ (c++)"XdgIconLoader::findIconHelper(QString const&, QString const&, QStringList&, bool) const@Base" 2.0.1~

[agaida]

@tsujan - sorry for being so silent the last days - build in all the PRs and have to sort some things out, still not finished

[tsujan]

@agaida Take you time! I'm busy with the latest changes in libfm-qt (and also Dolphin's code, ...).

[luis-pereira]

@tsujan I'm in the process of evaluating this. First of all, sorry for the delay. Can you point an example where KDE does the same thing ?

[tsujan]

@luis-pereira Please read https://github.com/lxde/libqtxdg/pull/116#issuecomment-278041342. It includes a real example.

[luis-pereira]

@tsujan Thanks. What X and Y illustrates KDE behaving that way ?

p.s. I'm making some "practical" tests with kiconfinder and qtxdg-iconfinder.

[tsujan]

@luis-pereira KDE behaves in the way of this PR.

You could: (1) Install gnome-disks; (2) Add an icon named gnome to the icon themes you use; (3) Remove gnome-disks from that theme; (4) Log out and in.

[luis-pereira]

@tsujan Under KDE with the Human icon theme what's the output of: kiconfinder system-suspend ? and under LXQt also with the Human theme and this PR what's the output of: qtxdg-iconfinder system-suspend ?

p.s. You have to rebase this PR to get the qtxdg-iconfinder tool.

[tsujan]

@luis-pereira

kiconfinder correctly finds "/usr/share/icons/Human/48x48/devices/system.png" because Human inherits Tangerine and gnome while none of those themes has "system-suspend".

I work under KDE for a few weeks (to study and patch Dolphin's code,...) and can't go to LXQt for now; so I don't have qtxdg-iconfinder yet but I'd tested this PR in several different ways before making it and it had worked fine. It's logic is clear too. Do you have a real example, where it doesn't work? I don't know about qtxdg-iconfinde yet but remember that icon cache sometimes played tricks, so that, to see the real icons, I had to log out and in.

[tsujan]

I forgot to add that, as in the case of my other PR's that aren't related to pcmanfm-qt, libfm-qt, etc., whatever you or @palinek do with this PR (editing, rebasing adding comments,...) will be OK to me. But please note that LXQt's current way of handling icons is incorrect -- in fact, the so-called "standards" are incorrect in this case -- and this PR is a step toward making it right.

[luis-pereira]

@tsujan I delete all cache files before experimenting. Take a look at qtxdg-iconfinder with this PR applied. It does not behave like the KDE engine.

qtxdg-iconfinder system-suspend
system-suspend:system-suspend
        /usr/share/icons/oxygen/base/128x128/actions/system-suspend.png
        /usr/share/icons/oxygen/base/22x22/actions/system-suspend.png
        /usr/share/icons/oxygen/base/32x32/actions/system-suspend.png
        /usr/share/icons/oxygen/base/64x64/actions/system-suspend.png
        /usr/share/icons/oxygen/base/16x16/actions/system-suspend.png
        /usr/share/icons/oxygen/base/48x48/actions/system-suspend.png

without this PR the output is following:

qtxdg-iconfinder system-suspend
system-suspend:system
        /usr/share/icons/Human/48x48/devices/system.png
        /usr/share/icons/Human/24x24/devices/system.png
        /usr/share/icons/Human/16x16/devices/system.png
        /usr/share/icons/Human/scalable/devices/system.svg

which matches KDE icon engine behavior.

[tsujan]

@luis-pereira That seems impossible to me. A bug in qtxdg-iconfinder? Which icon is really shown on the menu (after log-out/log-in)?

[tsujan]

@luis-pereira You made me log into LXQt and see for myself ;) KDE does it wrong; this PR does it right! Why? Because this is in Tangerine's index.theme:

Inherits=gnome,oxygen

Human inherits Tangerine's.

Why KDE doesn't go deeper, I don't know but this PR takes all parents into account correctly :)

EDIT: Previously I didn't have time to see Tangerine's index.theme; hence my mistake about kiconfinderbeing correct.

[tsujan]

In simple word, finding and showing that oxygen icon was the whole point of the PR: showing a meaningful icon as far as possible. Your test proved that.

I think KDE sets a limit to the number of parents that should be searched (only the first index.theme?).

[luis-pereira]

@tsujan qtxdg-iconfinder is really simple. It's results are trustworthy. I'm trying to have an informed opinion. Before breaking deliberately the standard I want to be in possession of "all" facts.
You stated that KDE also breaks the standard in this regard. While I think that what others do or not do is not that relevant, it may point some issues.

Iterating all the parents of a theme, can bring a performance penalty. Specially when theme creators abuse the Inherits entry.

Can you edit the Tangerine theme to: Inherits=oxygen and run again kiconfinder system-suspend ?

[luis-pereira]

Rebased

[tsujan]

qtxdg-iconfinder is really simple. It's results are trustworthy.

I know. As I wrote above, I wrongly trusted KDE. Everything is OK.

Iterating all the parents of a theme, can bring a performance penalty. Specially when theme creators abuse the Inherits entry.

I vaguely remember that the method in question was protected against infinite loops. Sorry, I'm very busy with another code; would you please check that? Apart from it, there can't be any penalty; we don't have 100 parents ;)

Can you edit the Tangerine theme to:....

Sorry, I can't, for the above reason.

IMHO, you could acquire the certainty you seek by reading the code. All my tests and also your test -- that was supposed to be a counter-example -- have confirmed that the code works fine.

EDIT: No infinite loop! See the line below the comment "// Used to protect against potential recursions"!

[tsujan]

OK. I came back to LXQt from KDE. Everything feels much lighter now :D

I took a look at the code again and found nothing wrong. Iterating over all the parents of a theme is what should be done; otherwise the keyword Inherits would have no meaning. Those who make icon sets use Inherits for a reason. Moreover, iterating over all parents isn't something this PR has introduced. It was already there; the difference is just in the method of iteration.

I think we discussed this too much. I'll listen to any rational criticism or reasoning but I'm afraid that too much of cautiousness is rather a personal matter.

[luis-pereira]

@tsujan your code does what it promises. There's no doubt about that. I'm in a trip. Will come back later today.

[agaida]

@tsujan - i'm nearly fine with the changes - it might be that i'm overly picky or understand something wrong so please bare with me if i'm missing the point - but i have a visual problem with the impact of the changes :) ^^ For the icons this is the outcome i would expect

^^ and it would be nice to have that matching too - in this special case it would point to another problem - the leave menu should use the system theme which is a modified frost in my case.

[tsujan]

@agaida I'll come to my computer soon but until then, could you please explain the problem? I didn't get it.

[tsujan]

@agaida, I'm back. Supposing that you use this PR, with the first screenshot, you've chosen Breeze. Let's take the log-out icon as an example. Its name is "system-log-out". In my system (latest Manjaro Testing) Breeze has this (ugly) icon for "system-log-out":

And it's displayed correctly.

In the second screenshot, you've chosen Breeze Dark and it has this (uglier) icon for "system-log-out":

So, your second screenshot can't be correct ;) In my system, Andromeda has that icon.

the leave menu should use the system theme which is a modified frost in my case

But you've chosen Breeze Dark in your second screenshot. Maybe that's an example of icon cache trick!?

[agaida]

@tsujan - you are right, i would expect the fugly icon - but i get the colorful icons - any ideas where i can search? Hmm, i guess i should work a little bit on a pure LXQt liveiso with latest snapshot packages. :)

[tsujan]

@agaida, even without this PR you shouldn't get those colored icons when you switch to Breeze Dark because it doesn't have them -- at least, not here, with breeze-icons 5.32.0. I know they belong to Andromeda but another theme (inheriting Andromeda) may have them too.

[tsujan]

BTW, Breeze Dark is for dark widget styles. Its icons won't be easily visible on light backgrounds under LXQt. KDE has an interesting feature LXQt lacks: when SVG icons are made appropriately -- as Breeze sets are -- they become white on (some) dark backgrounds and black on light ones. I know the basics of the code but not all of it.

[agaida]

First - the PR is ok, second - i'm getting old and grumpy, third - the expected icons are not in debians dark theme - for what reason ever. And i have no clue, which icons are displayed nor why, had to search it in Numix or in hicolor i guess.

Edit: and it would be cool if we could have some automatic or that it is possible to set icon themes on a application base, Quassel do this and it is really great. Overriding system settings make sense some times.

[tsujan]

i'm getting old and grumpy

Join the club ;)

it would be cool if we could have some automatic

You mean https://github.com/lxde/lxqt/issues/1181 ? It's tricky but quite possible.

or that it is possible to set icon themes on a application base

It isn't impossible.

[agaida]

and here is the reason for the nice coloured icons: the three files that debian deliver:> icons/breeze-dark % find . | grep system-log-out

./apps/16/system-log-out.svg ./apps/22/system-log-out.svg ./apps/32/system-log-out.svg

[tsujan]

and here is the reason for the nice coloured icons:

Oh, that's 5.28.0 (I downloaded it from https://packages.debian.org/sid/breeze-icon-theme right now). The colored icon is replaced by a colorless one in 5.32.0. So, your second screenshot showed a correct icon.

[agaida]

sometimes i really love linux development - it's so predictable ...

BTW: LGTM :)

[agaida]

ok, works fine with the 5.32 icons - and thats a bad thing ;) - we really need to adjust our themes systemwide or provide a mechanism to set different icon themes or something like lxde/lxqt/#1181 (or both).

[tsujan]

we really need to adjust our themes systemwide or provide a mechanism to set different icon themes ...

That's not needed. Breeze is tuned to the feature mentioned at https://github.com/lxde/lxqt/issues/1181. That feature needes 3 things: (1) LXQt code; (2) An icon set like Breeze (whose SVG icons have a special property); and (3) a style engine that supports the feature (currently only Kvantum and Breeze do). We need to do (1).

[agaida]

Indeed: From this point of view it isn't needed - but it would be nice to have.

[tsujan]

This is a screenshot of Dolphin with the Breeze icon set and the Kvantum Style engine (with its theme KvSimplicity) under KDE:

See how breeze icons change color on selected menu-items: they become white because the background is dark blue. The same thing doesn't happen under LXQt because LXQt still lacks this nice feature.

[agaida]

go for it :)

[tsujan]

Sure; just a little more free time. I think I'll have it soon.

[agaida]

Don' hurry - if we solve all problems today, what should we do tomorrow? That would be boring ...

[tsujan]

:)

To prove that it isn't specific to KDE apps, this is pcmanfm-qt with Breeze icons and another Kvantum theme but under KDE:

This time, icons are white on the dark toolbar and selected menu-item but black on the light menu. The default Breeze set is used here.

[agaida]

@tsujan @palinek @luis-pereira - any objections to merge this?

[tsujan]

As the PR maker, I can't have any objection, of course ;)

[palinek]

As I've said, I have no problem with either of the prioritization logic. But we definitely should mention (in doc?, source code comment? ..), that we are violating the standard for a good reason.

[tsujan]

But we definitely should mention (in doc?, source code comment? ..), that we are violating the standard for a good reason.

I can't find a good place in the source for such a comment.

[agaida]

If anything else fails - README seems a good place

[tsujan]

An explanation could contain a statement like this (but, perhaps, not exactly this):

"... The XXX standard gives priority to the current icon set, sometimes at the cost of a meaningless icon, while YYY searches for a meaningful icon first... "

[agaida]

We should put the explanation in a separated Pull Request - such explanations have a great potential for bike shedding.