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

Fm::IconInfo, scale factor and symbolic icons: not a bug but details #788

Closed
tsujan opened this issue Feb 28, 2022 · 21 comments · Fixed by #1015
Closed

Fm::IconInfo, scale factor and symbolic icons: not a bug but details #788

tsujan opened this issue Feb 28, 2022 · 21 comments · Fixed by #1015
Labels

Comments

@tsujan
Copy link
Member

tsujan commented Feb 28, 2022

Fm::IconInfo correctly scales icons with scale factors > 1; actually, it lets Qt do it. It's responsible for icons inside pcmanfm-qt's (as well as LXQt file dialog's) view and side-pane. Other icons are controlled by Qt, whether directly or through an icon engine (like that of LXQt).

But, for example, with a scale factor of 2, LXQt's icon engine will choose symbolic icons for the sizes 16×2, 22×2 and 24×2 if the icon theme supports them, while Fm::IconInfo shows ordinary icons. The end result is sharp icons in both cases but Fm::IconInfo doesn't use symbolic icons with a scale factors of 2.

You could check that by launching QT_SCALE_FACTOR=2 pcmanfm-qt (after stopping pcmanfm-qt from LXQt session settings). If your icon theme supports symbolic icons, the toolbar icons will be symbolic but the side-pane and view icons won't.

Since Fm::IconInfo takes the icon name from GIcon and uses QIconEngine to find the icon, there doesn't seem to be a place in it for dealing with scale factors directly. So, I'm not sure this could be changed in future. However, IMO, it's better to have an open issue for it.

EDIT: See #788 (comment) → 4, for why you might get confused by the Breeze icon set when testing this.

tsujan added a commit that referenced this issue Feb 28, 2022
It could interfere with the active icon engine.

Fixes #788
@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

Also, see @palinek's comment on why we need an icon engine: #789 (comment)

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

Can you, please, exactly describe icon names, themes etc. for me to replicate it. Maybe we can find a solution/workaround.

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

Any icon set with SVG symbolic icons, like Breeze.

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

What does "symbolic" mean here?

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

Let me show you with an image:

symbolic

The symbolic SVG icons change color, depending on their backgrounds (our icon engine started to support that years ago, with your and my codes).

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

What does "symbolic" mean here?

They have simple forms and are usually monochrome. For coders, I should have said, "with stylesheets".

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

Finally, and for the sake of completeness, this screenshot is taken with a scale factor of 2:

scaled

The toolbar icons are still symbolic, but the side-pane and view icons aren't because libfm-qt's icon engine interferes with that of LXQt.

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

but Fm::IconInfo doesn't use symbolic icons with a scale factors of 2.

It is using what is provided by the icon theme. E.g. the desktop folder -> icon user-desktop, this is what is provided by breeze:

$ find /usr/share/icons/breeze -name user-desktop\* -exec ls -l {} \; | sort -k8,8
-rw-r--r-- 1 root root 452 2022-01-01 13:10 /usr/share/icons/breeze/places/16/user-desktop.svg
-rw-r--r-- 1 root root 427 2022-01-01 13:10 /usr/share/icons/breeze/places/22/user-desktop.svg
-rw-r--r-- 1 root root 6069 2022-01-01 13:10 /usr/share/icons/breeze/places/32/user-desktop.svg
-rw-r--r-- 1 root root 3733 2022-01-01 13:10 /usr/share/icons/breeze/places/48/user-desktop.svg
-rw-r--r-- 1 root root 8415 2022-01-01 13:10 /usr/share/icons/breeze/places/64/user-desktop.svg
-rw-r--r-- 1 root root 7003 2022-01-01 13:10 /usr/share/icons/breeze/places/96/user-desktop.svg
lrwxrwxrwx 1 root root 26 2022-02-11 23:46 /usr/share/icons/breeze/places/symbolic/user-desktop-symbolic.svg -> ../16/folder-documents.svg

screenshot
screenshot
screenshot
screenshot
screenshot
screenshot
screenshot

So the theme icon engine just follows the decision made by icon theme. (with the scale factor > 1 there is never requested icon smaller than 32)

@tsujan
Copy link
Member Author

tsujan commented Mar 2, 2022

No! Breeze also has 16@2x, 22@2x and 24@2x folders. They are for being used with scaling. If you remove Fm::IconEngine (→ #789), LXQt icon engine will show them correctly.

@palinek
Copy link
Contributor

palinek commented Mar 2, 2022

Oh sorry... missed that.
Then the cause is is in IconEngine::virtual_hook... I see it now.

I have something in my mind, need to test if it can solve this problem.

@palinek
Copy link
Contributor

palinek commented Mar 3, 2022

Please, what icon theme are you using and which showed icons are you considering to be shown wrongly?

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

Please, what icon theme are you using

It makes no difference; the "problem" appears with all icon themes that have symbolic icons. I use https://github.com/tsujan/Plataro but, IMO, Breeze is better for testing

This is a screenshot of LXQt file dialog with a scale factor of 2 and Breeze:

breeze

Like in my other screenshot, no view/side-pane icon is symbolic, but all would be symbolic if Fm::IconEngine was removed.

@palinek
Copy link
Contributor

palinek commented Mar 3, 2022

Yes, but it depends what sizes you configured

  • size of small icons 22px
    screenshot
  • size of small icons 28px
    screenshot

...and based on some tests, icons in left pane have different size for pcmanfm-qt & lxqt file dialog, I'll check this further.

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

  • Side-pane icons are 24-pix.
  • Fusion isn't self-consistent with 22-pix (and the Breeze widget style seems to inherit that). Either use 24-pix view icons to see the problem in the view or forget about the view and focus on the side-pane. (Kvantum is self-consistent.)
  • There's no 28-pix symbolic icon in Breeze (and in most icon sets).
  • In a nutshell, where you see symbolic icons without a scale factor, you should also see symbolic icons with a scale factor of 2, provided that you use the Breeze set.

@palinek
Copy link
Contributor

palinek commented Mar 3, 2022

Ok... here is some debug output (with the lxqt/libqtxdg#275, but it probably doesn't make difference) how the icon is searched (user-desktop):

  • 24px (scale=1) - best match (distance = (4 - 0) / 2 = 2) -> places/22/user-desktop.svg
entryForSize enter 0x7fc8e4005f80 "user-desktop" QSize(24, 24) 1
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 144 "/usr/share/icons/breeze/places/96/user-desktop.svg"
entryForSize 0x7fc8e4005f80 better match
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 80 "/usr/share/icons/breeze/places/64/user-desktop.svg"
entryForSize 0x7fc8e4005f80 better match
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 48 "/usr/share/icons/breeze/places/48/user-desktop.svg"
entryForSize 0x7fc8e4005f80 better match
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 16 "/usr/share/icons/breeze/places/32/user-desktop.svg"
entryForSize 0x7fc8e4005f80 better match
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 85 "/usr/share/icons/breeze/places/22@3x/user-desktop.svg"
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 41 "/usr/share/icons/breeze/places/22@2x/user-desktop.svg"
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 4 "/usr/share/icons/breeze/places/22/user-desktop.svg"
entryForSize 0x7fc8e4005f80 better match
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 49 "/usr/share/icons/breeze/places/16@3x/user-desktop.svg"
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 17 "/usr/share/icons/breeze/places/16@2x/user-desktop.svg"
entryForSize 0x7fc8e4005f80 QSize(24, 24) 1 distance 16 "/usr/share/icons/breeze/places/16/user-desktop.svg"
  • 48px (scale=2) - best match (distance = (1 - 1) / 2 = 0) -> places/48/user-desktop.svg, places/16@3x/user-desktop.svg
entryForSize enter 0x55c0b31419d0 "user-desktop" QSize(48, 48) 2
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 97 "/usr/share/icons/breeze/places/96/user-desktop.svg"
entryForSize 0x55c0b31419d0 better match
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 33 "/usr/share/icons/breeze/places/64/user-desktop.svg"
entryForSize 0x55c0b31419d0 better match
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 1 "/usr/share/icons/breeze/places/48/user-desktop.svg"
entryForSize 0x55c0b31419d0 better match
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 33 "/usr/share/icons/breeze/places/32/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 37 "/usr/share/icons/breeze/places/22@3x/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 8 "/usr/share/icons/breeze/places/22@2x/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 53 "/usr/share/icons/breeze/places/22/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 1 "/usr/share/icons/breeze/places/16@3x/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 32 "/usr/share/icons/breeze/places/16@2x/user-desktop.svg"
entryForSize 0x55c0b31419d0 QSize(48, 48) 2 distance 65 "/usr/share/icons/breeze/places/16/user-desktop.svg"

So for this example, how/why should we determine the 16@3x better match than the 48 ?
(I don't know how the QSettings allKeys work, but I would determine the order of traversing the entries in theme rather unspecified)

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

So for this example, how/why should we determine the 16@3x better match than the 48 ?

I feel we may be talking of different things, but 16@... icons should match only 16-pix icons, not 32/48-pix ones. With a scale factor of 2, the pixmap of a 16-pix icon has a device pixel ratio of 2.

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

This screenshot is taken with the Breeze set and a scale factor of 3 and, again, shows that LXQt icon engine has no problem:

scaled_3

The toolbar icons are 16-pix (in Kvantum), their pixmaps have a device pixel ration of 3, and all of them are symbolic (except for the rightmost one, which doesn't have a symbolic icon at all).

@palinek
Copy link
Contributor

palinek commented Mar 3, 2022

Can you have a look in #792?

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

Can you have a look in #792?

I did. Please also read my comment about the Breeze set there. Problem inside problem...

@tsujan
Copy link
Member Author

tsujan commented Mar 3, 2022

Let's summarize it again, for the sake of removing confusions (if you don't agree with what follows, please tell me):

  1. With the current internal icon engine of libfm-qt (namely, Fm::IconEngine), scaled symbolic icons aren't picked up by the side-pane or view.
  2. If we remove Fm::IconEngine, scaled symbolic icons reappear (because LXQt's icon engine works fine).
  3. With something like IconEngine: Forward API to child icon engine #792, scaled symbolic icons reappear too.
  4. The Breeze icon set can be very confusing in this regard because it doesn't include places/24@2x in its index.theme. If you want to test what's explained in the first comment of this report, use an icon set that really has scaled, symbolic, places icons.

tsujan added a commit that referenced this issue Oct 11, 2024
I found no reason for using `IconEngine`. Moreover, its removal closes #788
tsujan added a commit that referenced this issue Oct 15, 2024
Scaling is taken into account separately for Qt < 6.8 and Qt ≥ 6.8.

Closes #788
@tsujan
Copy link
Member Author

tsujan commented Oct 15, 2024

Problem solved, at last: #1015. It was caused by a bad scaled pixmap. But it couldn't be fixed with Qt5, because a needed QIcon method didn't exist there.

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