Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed internal icon engine #789

Closed
wants to merge 1 commit into from
Closed

Removed internal icon engine #789

wants to merge 1 commit into from

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented Feb 28, 2022

It could interfere with the active icon engine.

Fixes #788

It could interfere with the active icon engine.

Fixes #788
@tsujan
Copy link
Member Author

tsujan commented Feb 28, 2022

@palinek, do you know of a reason for having an internal icon engine? Have I missed something obvious?

@palinek
Copy link
Contributor

palinek commented Feb 28, 2022

If I'm not mistaken, it is here exactly for being a bridge for using glib's icon resolving. I didn't look into the code thoroughly now...how do icons work with the engine removed?

@tsujan
Copy link
Member Author

tsujan commented Feb 28, 2022

If I'm not mistaken, it is here exactly for being a bridge for using glib's icon resolving.

The bridging is done by Fm::IconInfo (especially, by IconInfo::internalQicon). Fm::IconEngine just used it.

how do icons work with the engine removed?

They're OK. Moreover, the problem described in #788 is resolved because, now, LXQt's icon engine does its job without any interference.

I'll use the patch for a while. If you find a reason for having an internal icon engine, please tell me!

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

The bridging is done by Fm::IconInfo (especially, by IconInfo::internalQicon). Fm::IconEngine just used it.

Yes, but the themed GIcon supports multiple (prioritized) names, whereas the QIcon does not. So the Fm::IconEngine here is used to make the bridge for it.
Without the IconEngine if you put most precise QIcon to any qt-world component (by using IconInfo::qicon()), that qt component lost track of the "parent" IconInfo object -> now if there would an icon theme change occur, the qt component (with its QIcon object) may show wrong icon (low priority).

E.g.:

  • you need icon themed "AAA"
  • current theme T1 doesn't have such icon, so the first one found of fallback ("unknown", "application-octet-stream", "application-x-generic", "text-x-generic") will be used and returned as QIcon q
  • icon theme is changed
  • current theme T2 does have icon "AAA"
  • the qt component with QIcon will still show the wrong fallback icon image (if any in the T2 theme) or even some QIconEngine fallback (if the T2 doesn't have such icon)

Where if there is the QIcon with Fm::IconEngine used, upon the theme change the most suitable icon is shown for that theme.

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

Very good argument! Yes, it's all about icon set change. Thank you very much!

Now, we have a reason to tolerate #788.

Yes, but the themed GIcon supports multiple (prioritized) names, whereas the QIcon does not.

Using IconEngine doesn't make a difference in that respect. IconEngine::virtual_hook calls IconInfo::internalQicon(), as is the case without IconEngine: in both cases, the first icon is picked up.

However, that doesn't change the value of your main argument about changing the icon set.

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

Using IconEngine doesn't make a difference in that respect. IconEngine::virtual_hook calls IconInfo::internalQicon(), as is the case without IconEngine: in both cases, the first icon is picked up.

No. The first no-null/valid icon is taken in internalQicon() and the validity of each one could have been changed after icon theme update.

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

No. The first no-null/valid icon is taken in internalQicon() and the validity of each one could have been changed after icon theme update.

Yes, I meant the first existing icon, and it's picked up with or without an internal icon engine. The whole reason for an internal icon engine is the case of icon theme change, whether a theme is updated or it's switched.

I might add a comment to the code later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fm::IconInfo, scale factor and symbolic icons: not a bug but details
2 participants