Differential D22958

Add initial Android support to ecm_add_app_icon
AbandonedPublic
Actions

Authored by hein on Aug 5 2019, 9:26 PM.

Details

Reviewers
mart
leinir
apol
Group Reviewers
Frameworks
Summary

Kirigami has a kirigami_package_breeze_icons CMake macro that
KDE Frameworks apps targeting Android use to extract icons from
the Breeze theme into a folder that will get picked up by our
SDK for inclusion in the eventual .apk binary package.

Applications that bundle their own application icon that is not
included in the Breeze theme do not benefit from this, as that
macro relies on making a clone of breeze-icons.git to work from,
and as those apps install their icons into the "hicolor" theme
instead anyway, as is spec-conformant.

This e.g. means that the application icon won't show on the
Kirigami.AboutPage Qt Quick component, which is used to display
the About dialogs in Kirigami apps.

This patch makes ecm_app_icon put app icons into the same
folder, if the constraints of being on Android and being a SVG
icon are satisfied, causing the icon to be picked up into the
.apk.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14780
Build 14798: arc lint + arc unit
hein created this revision.Aug 5 2019, 9:26 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptAug 5 2019, 9:26 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
hein requested review of this revision.Aug 5 2019, 9:26 PM
hein added a comment.Aug 5 2019, 9:27 PM

The code is mostly copy-and-pasted together from ECMAddAppIcon.cmake and ECMInstallIcons.cmake.

hein added a comment.Aug 5 2019, 9:42 PM

This is the culmination of a patch series that makes About dialogs in Kirigami work reasonably across platforms:

apol added a comment.Aug 5 2019, 11:43 PM

The patch makes sense. +1

modules/ECMAddAppIcon.cmake
154

this if could go up to where we're checking the icon name in an elseif up at line 146.

hein added inline comments.Aug 6 2019, 12:40 AM
modules/ECMAddAppIcon.cmake
154

We'd need to do a get_filename_component there and end up either pulling the ext thing out twice (unless CMake doesn't do block scoping) or ahead of the ABSOLUTE one and then possibly unnecessarily. FWIW I copied that entire regex block out of ECMInstallIcons.cmake to play it safe so I'd prefer not touching it ... :-)

mart requested changes to this revision.Aug 6 2019, 7:34 AM
mart added inline comments.
modules/ECMAddAppIcon.cmake
157

I'm not sure i want keep using that weird icon localtion (which is pretty much deprecated and i would remove the support for it if it wasn't for legacy), it was more a result of me not knowing how tings were supposed to work.
for android and static things, icons are supposed to be in the qrc under qrc:/icons/themename/typical icon theme hyerarchy
and that will make QIcon::fromTheme just work(tm)

to have this (and premade theme metadata so one just installs icons) kirigami has a qrc://icons/breeze-internal which is already there (and would need adding icons there for the android case)
always meant making some cmake for it, but then a thousand other things reclaimed the time

This revision now requires changes to proceed.Aug 6 2019, 7:34 AM
nicolasfella added a comment.Aug 6 2019, 7:51 AM

Why do you restrict it to SVGs? Wouldn't you be able to use PNGs as well?

hein added a comment.Aug 6 2019, 8:59 AM
In D22958#507299, @nicolasfella wrote:

Why do you restrict it to SVGs? Wouldn't you be able to use PNGs as well?

Mostly because I don't know what assumptions Kirigami makes. Its own macro is hardcoded to only work with Breeze, which is implicitly SVG, so I didn't want to throw anything else at it in its special folder.

hein abandoned this revision.EditedAug 6 2019, 9:10 AM

Marco explained privately he doesn't want this so I'll move it to my codebase for now until the alternative better thing appears (maybe I'll try to make it appear soon).

Revision Contents
Changeset List

PathPackages
Mmodules/ECMAddAppIcon.cmake (21 lines)

Diff 63151

View Options

modules/ECMAddAppIcon.cmake

Loading...