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

Consider resources in flavor sources sets #452

Merged
merged 5 commits into from
May 20, 2023

Conversation

kkris
Copy link
Contributor

@kkris kkris commented May 17, 2023

When a product flavor overrides resources, they are currently not picked up by the plugin.

This PR includes the source set names of all defined product flavors and also extends samle/example-custom with some overriden icons to test.

Steps to reproduce before the fix: build via example-custom:installStagingAaaDebug and observe that the app icon used is from the main source set which has lower priority than staging

After the fix, the correct icons from staging/res are picked up.

Note: I had to use --rerun-tasks at times since the task does not always run, depending on the changes made since the last execution.

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #452 (e84a449) into master (d5bc16d) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #452      +/-   ##
============================================
+ Coverage     77.81%   77.88%   +0.07%     
  Complexity      112      112              
============================================
  Files            16       16              
  Lines           631      633       +2     
  Branches         99       99              
============================================
+ Hits            491      493       +2     
  Misses           63       63              
  Partials         77       77              
Impacted Files Coverage Δ
.../starter/easylauncher/plugin/EasyLauncherPlugin.kt 65.26% <100.00%> (+0.74%) ⬆️

@mateuszkwiecinski
Copy link
Member

mateuszkwiecinski commented May 17, 2023

Thanks! I'll try to take a look soon :) (I can also record new screenshots if they fail, since that's unconvenient for most people:/ )

Note: I had to use --rerun-tasks at times since the task does not always run, depending on the changes made since the last execution.

Can you clarify which tasks did you have problems with? Which tasks you expected to run and which were up-to-date (or from-cache?)?

Also, I wanted to let you know to ignore failing optional checks - I'm still working on restoring build's configuration-cache compatibility :)

@kkris kkris force-pushed the consider-flavor-resources branch from 6296e62 to ccd161d Compare May 18, 2023 08:09
@kkris
Copy link
Contributor Author

kkris commented May 18, 2023

Thanks! I'll try to take a look soon :) (I can also record new screenshots if they fail, since that's unconvenient for most people:/ )

Thanks! I tried to run the recordAll task, but it failed with this error: Failed to create the directory /sdcard/screenshots/com.example.flavor.productionAaaDebug.test/screenshots-default for screenshots. Is your sdcard directory read-only?. Are there any other preconditions I need to fulfill?

Note: I had to use --rerun-tasks at times since the task does not always run, depending on the changes made since the last execution.

Can you clarify which tasks did you have problems with? Which tasks you expected to run and which were up-to-date (or from-cache?)?

My bad, I messed up the example and thought it was some caching issue. To showcase the problem, I created a simple sample: sample/example-flavor. It has two flavors in the environment dimension and the staging flavor overrides the launcher icons. Try example-flavor:installStagingAaaDebug without the fix (second commit) and the app icon will be the green one from the main source set. After the fix, the correct icon from the staging source set is taken.

My understanding of how the different ways of specifying app icons work together was too weak to integrate it into the example-custom sample.

@mateuszkwiecinski
Copy link
Member

Thanks! I tried to run the recordAll task, but it failed with this error: Failed to create the directory /sdcard/screenshots/com.example.flavor.productionAaaDebug.test/screenshots-default for screenshots. Is your sdcard directory read-only?. Are there any other preconditions I need to fulfill?

Screenshots require very specific setup to be recorded:

  1. They require python with specific version of Pillow installed:
    python -m pip install 'Pillow==9.1.1'
  2. They require specific device to be installed: or https://github.com/usefulness/easylauncher-gradle-plugin/blob/d5bc16d6836e598efe1d87eff49542d8fa620bd5/sample/emulator_config.ini

Your error suggests you tried to run it on api >28 device which is well... not yet supported :/ I have a plan to migrate to paparazzi at some point, but last time I checked it was not yet stable.

My understanding of how the different ways of specifying app icons work together was too weak to integrate it into the example-custom sample.

I think the only missing part was the fact all the pngs you added are used on api <26 devices only, and you were missing an override for adaptive icons. I merged example-flavor with example-custom and added missing color resource override. I hope it will pass the test.

Regarding the fix itself, it was an obvious bug, thanks for fixing it 👍
I believe taking a variant.flavorName into account could also be considered a bug, since such setup is too specific and the generated icons will have lower priority than the "real" ones. I decided to keep it as it is, until someone complains 🤷

@mateuszkwiecinski mateuszkwiecinski merged commit da457b6 into usefulness:master May 20, 2023
@mtrakal
Copy link

mtrakal commented May 29, 2023

Hi, @mateuszkwiecinski do you plan new release soon? 🙏 I just today faces to this merged (not released yet) issue :).
Thanks a lot for your work on this plugin 👍

@mateuszkwiecinski
Copy link
Member

Hey 👋 right, sorry for the delay 😞
I will try to trigger a release today. I need first to verify if build configuration changes I had to make recently didn't break release pipeline.

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

Successfully merging this pull request may close these issues.

3 participants