[about-distro] run absolute paths through qicon intead of qpixmap
ClosedPublic

Authored by sitter on Feb 15 2019, 12:23 PM.

Details

Summary

qicon::fromtheme internally skips resolution of absolute paths and instead
simply opens it by path. so, simplify the code by only calling fromTheme.

this has two additional advantages:

  • the icon size is now always capped at 128x128 (smaller icons are smaller but that beats upscaling) preventing problems where a distribution might use too large a resolution so the kcm looks wonky on scaling factor 1
  • the LogoPath config may now also be an icon name for resolution through the icon theme instead of an absolute path
Test Plan
  • huge logos are scaled into shape

Diff Detail

Repository
R102 KInfoCenter
Branch
Plasma/5.15
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8323
Build 8341: arc lint + arc unit
sitter created this revision.Feb 15 2019, 12:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 15 2019, 12:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sitter requested review of this revision.Feb 15 2019, 12:23 PM
sitter edited the summary of this revision. (Show Details)Feb 15 2019, 12:24 PM
sitter added a reviewer: Plasma.
cfeck added a subscriber: cfeck.Feb 15 2019, 12:32 PM

The comment is confusing. If QIcon does not resolve absolute paths, but I can stil pass absolute paths, the comment could say that it ignores the path? Shouldn't this comment rather be part of the README, instead of the code?

Well, it won't resolve the path, it will simply load it. I do see the ambiguity though. ;)

sitter updated this revision to Diff 51741.Feb 15 2019, 12:35 PM

throw away the comment. upon reflection I actually find it unnecessary

isn't QIcon::fromTheme buggy when passed an absolute path to an SVG file?

works fine here

The issue Kai is worried about is https://bugreports.qt.io/browse/QTBUG-63187. If we're not hitting that bug here, we need to understand why, or else there's a chance that the logo will not appear.

If I understand it correctly, the bug would only show its ugly head with QIcon::fromTheme("foo.svg") but not QIcon::fromTheme("foo"). So, if that seems a concern we can just add a comment in the README. Distribution devs would set LogoPath's value manually, so we can just tell them to use a valid value :) Does that seem agreeable @broulik?

@ngraham On a side-note: foo.svg is not supported by the specs, so Qt not working with it properly is not even a defect TBH - it'd be an additional convenience feature in Qt. Specifically the desktop entry spec defines Icon= as either absolute or it will be run through the lookup described in the icon theme spec, and the icon theme spec sets forth a lookup which explicitly assumes filename = directory/$(themename)/subdir/iconname.extension which by extension means a string handed to the theme lookup may only ever be a basename of the icon. It may not be absolute, nor relative, nor contain extensions. So, while it'd be nice if Qt handled sanitation of relative strings, our software is in fact wrong if it allows users to set a (relative) icon with file extension and then runs that through QIcon::fromTheme without adjusting it. The input must either be absolute or properly sanitized to meet the requirements of the icon theme spec.

The SVG issue is only relevant in QIcon::fromTheme(name, fallback) because it detects

if (icon.isNull() || icon.availableSizes().isEmpty())
    return fallback;

and availableSizes is problematic for the SVG QIcon backend.

You're not using that, so it's fine.

davidedmundson accepted this revision.Feb 19 2019, 3:07 PM
This revision is now accepted and ready to land.Feb 19 2019, 3:07 PM
This revision was automatically updated to reflect the committed changes.