D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein abandoned this revision.
hein added a comment.


  Marco explained privately he doesn't want this so I'll move it to my codebase 
for now until the alternative better thing appears.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: nicolasfella, kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, 
bencreasy, michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: nicolasfella, kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, 
bencreasy, michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Nicolas Fella]

nicolasfella added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: nicolasfella, kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, 
bencreasy, michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Marco Martin]

mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ECMAddAppIcon.cmake:157
> +else()
> +install(FILES ${icon_full} DESTINATION 
> ${KDE_INSTALL_QMLDIR}/org/kde/kirigami.2/icons/ RENAME "${name}.${ext}")
> +endif()

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> apol wrote in ECMAddAppIcon.cmake:154
> this if could go up to where we're checking the icon name in an elseif up at 
> line 146.

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 ... :-)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Aleix Pol Gonzalez]

apol added a comment.


  The patch makes sense. +1

INLINE COMMENTS

> ECMAddAppIcon.cmake:154
> +set(ext   "${CMAKE_MATCH_4}")
> +if(NOT ${icon_type} STREQUAL ".svg" OR NOT ${icon_type} 
> STREQUAL ".svgz")
> +message(AUTHOR_WARNING "${icon_full} is not a SVG file - 
> ignoring")

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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein added a comment.


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

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22958

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22958: Add initial Android support to ecm_add_app_icon

[Eike Hein]

hein created this revision.
hein added reviewers: Frameworks, mart, leinir, apol.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
hein requested review of this revision.

REVISION 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.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22958

AFFECTED FILES
  modules/ECMAddAppIcon.cmake

To: hein, #frameworks, mart, leinir, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns