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

[BUG]: CI Check for color usage in vectors will never pass until minSdk is updated #5072

Closed
adhiamboperes opened this issue Jun 27, 2023 · 8 comments · Fixed by #5076
Closed
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@adhiamboperes
Copy link
Collaborator

Describe the bug

File Content Validation CI check that prohibits usages of colors other than component_colors in drawable files fails correctly on CI when violated, but the pattern that is being enforced cannot be supported by our current codebase, since we have not yet removed support for kitkat.

The CI error is:

app/src/main/res/drawable/rounded_primary_button_grey_shadow_color.xml:5: Only colors from component_colors.xml may be used in drawables except vector assets.
app/src/main/res/drawable/survey_nps_radio_selected_color.xml:4: Only colors from component_colors.xml may be used in drawables except vector assets.
app/src/main/res/drawable/survey_nps_radio_selected_color.xml:8: Only colors from component_colors.xml may be used in drawables except vector assets.
app/src/main/res/drawable/survey_nps_radio_unselected_color.xml:4: Only colors from component_colors.xml may be used in drawables except vector assets.
app/src/main/res/drawable/survey_nps_radio_unselected_color.xml:8: Only colors from component_colors.xml may be used in drawables except vector assets.
app/src/main/res/values/color_palette.xml:256: All color declarations in component_color.xml and color_palette.xml should end with _color suffix following snake_case naming convention.

Upon applying the recommended fix, the project build fails with error:

Cause 1: org.gradle.workers.internal.DefaultWorkerExecutor$WorkExecutionException: A failure occurred while executing com.android.build.gradle.internal.tasks.Workers$ActionFacade
	...
Caused by: java.lang.RuntimeException: Error while processing /Users/adhiambooyier/opensource/oppia-android/app/src/main/res/drawable/survey_nps_radio_selected_color.xml : Can't process attribute android:fillColor="@color/component_color_survey_primary_button_background_color": references to other resources are not supported by build-time PNG generation.
File was preprocessed as vector drawable support was added in Android 5.0 (API level 21)
See http://developer.android.com/tools/help/vector-asset-studio.html for details.
.android.ide.common.resources.MergedResourceWriter$FileGenerationWorkAction.run(MergedResourceWriter.java:412)
	... 34 more

Steps To Reproduce

In any vector file in res/drawable, replace a <path> tag fillColor attribute color hex (e.g. #ffffff) with a color resource (e.g. @color/component_color_shared_screen_secondary_background_color) and then rebuild the app.

Expected Behavior

I expect the project to build without errors.

Screenshots/Videos

Screenshot 2023-06-27 at 05 24 29

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

This check was introduced in #4960

@adhiamboperes adhiamboperes added bug End user-perceivable behaviors which are not desirable. triage needed labels Jun 27, 2023
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. and removed triage needed labels Jun 27, 2023
@adhiamboperes
Copy link
Collaborator Author

Hi @MohitGupta121, could you please take a look at disabling this CI check as it blocks my PR? Additionally, could you please also file a seperate issue that tracks adding that check back once we deprecate support for kitkat?

@MohitGupta121
Copy link
Member

@adhiamboperes one solution is to add those failing files in the exempted test. After that KitKat thing is done we can remove that. Can I do this in the new PR or are you doing it in your PR?

@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Jun 27, 2023

I don't know whether exempting files is a robust solution, since I cannot say when KitKat deprecation will be prioritized. Could you give me a rough estimate of how much work it would be to disable the check (are tests involved, are there potential breakages) vs the exemption solution. I think it is fine to go with the simplest solution here.
In case of adding the files to exemptions, I will do it in my PR since they are new resources being introduced in that PR.

@MohitGupta121
Copy link
Member

@adhiamboperes disabling the tests means removing the condition regex for drawables.

@adhiamboperes
Copy link
Collaborator Author

Do you estimate that it will involve a lot of work to remove it?

@MohitGupta121
Copy link
Member

No it take few lines change

@adhiamboperes
Copy link
Collaborator Author

I would prefer that we go with that solution. Could you please put up a PR for that?

@MohitGupta121
Copy link
Member

@adhiamboperes okay sure today I'll make PR.

@MohitGupta121 MohitGupta121 added Impact: High High perceived user impact (breaks a critical feature or blocks a release). and removed Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). labels Jun 27, 2023
adhiamboperes pushed a commit that referenced this issue Jun 28, 2023
…5076)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fix #5072 : Remove
condition of hex color for drawables from regex

<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing
adhiamboperes added a commit that referenced this issue Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Development

Successfully merging a pull request may close this issue.

2 participants