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

Fix #5075: Revert "Fix #5072 : Remove condition of hex color for drawables from regex" #5081

Closed
wants to merge 5 commits into from

Conversation

adhiamboperes
Copy link
Collaborator

Fixes #5075 :Reverts #5076

I took another look at the regex checks and noticed that they actually generally prohibit use of color hex in any drawables other than Vector files(which were the files that caused me to request this change in #4945), so the check is valid.(Sorry!)

@adhiamboperes adhiamboperes requested a review from a team as a code owner July 8, 2023 23:32
@adhiamboperes adhiamboperes self-assigned this Jul 8, 2023
@oppiabot
Copy link

oppiabot bot commented Jul 15, 2023

Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 15, 2023
@adhiamboperes adhiamboperes removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 18, 2023
@adhiamboperes adhiamboperes changed the title Fix #5075: Revert "Fix #5072 : Remove condition of hex color for drawables from regex" [Blocked] Fix #5075: Revert "Fix #5072 : Remove condition of hex color for drawables from regex" Aug 1, 2023
@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Aug 1, 2023

Hi @rt4914, as per @MohitGupta121's comment, PTAL and let us know if this sufficiently addresses #5114.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concerns since this is a clean revert.

@oppiabot oppiabot bot unassigned seanlip Aug 2, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 2, 2023

Unassigning @seanlip since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Aug 2, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 2, 2023

Hi @adhiamboperes, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@MohitGupta121 MohitGupta121 removed their assignment Aug 2, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 2, 2023

Hi @adhiamboperes, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Aug 3, 2023

The check (to be reintroduced by this revert) is failing because of the following files introduced in #4881:

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.
89app/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.
90app/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.
91Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks#regexpatternvalidation-check for more details on how to fix this.
92Exception in thread "main" java.lang.Exception: REGEX PATTERN CHECKS FAILED
93
94	at org.oppia.android.scripts.regex.RegexPatternValidationCheckKt.main(RegexPatternValidationCheck.kt:64)

After replacing the highlighted lines with component_colors:

Screenshot 2023-08-04 at 00 32 54

The project fails to build with error:

Cause 1: org.gradle.workers.internal.DefaultWorkerExecutor$WorkExecutionException: A failure occurred while executing com.android.build.gradle.internal.tasks.Workers$ActionFacade
	at org.gradle.workers.internal.DefaultWorkerExecutor$WorkItemExecution.waitForCompletion(DefaultWorkerExecutor.java:336)
Caused by: java.lang.RuntimeException: Error while processing /Users/adhiambooyier/opensource/oppia-android/app/src/main/res/drawable/survey_nps_radio_unselected_color.xml : Can't process attribute android:fillColor="@color/component_color_shared_white_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.
	at com.android.ide.common.resources.MergedResourceWriter$FileGenerationWorkAction.run(MergedResourceWriter.java:420)
	at com.android.build.gradle.internal.tasks.Workers$ActionFacade.run(Workers.kt:348)

Which is the original cause for the reversion in #5076.

Essentially, this issue is still not fixed, so I will hold off merging this PR. It appears that the CI check is actually not entirely correct, and should be modified in order to actually fix #5075 and #5114.

@adhiamboperes adhiamboperes removed their assignment Aug 3, 2023
@adhiamboperes adhiamboperes added PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged PR: don't merge - Contains Problems The PR has unresolved issues and removed PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged labels Aug 3, 2023
@adhiamboperes
Copy link
Collaborator Author

Closing in favor of #5123

BenHenning added a commit that referenced this pull request Sep 19, 2023
## Explanation
Fixes #5075. 
This PR adds back the color hex regex check removed in #5076. This
requires a static check exemption for two vectors in the `drawable`
directory, `survey_nps_radio_selected_color.xml` and
`survey_nps_radio_unselected_color.xml`.

The check `file_path_regex: "app/src/main/res/drawable.*?/.+?\\.xml"`
blanket checks all drawables, including vectors. Since these cannot
fully be changed to use component colors yet, I think that the best
approach is to exempt them from the check.

This is a replacement of PR #5081, since #5081 would nolonger be a clean
revert.

## 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

---------

Co-authored-by: Ben Henning <[email protected]>
@adhiamboperes adhiamboperes deleted the revert-5076-ci_checks_fail branch December 5, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add Regex Check That Prohibits ColorHex Usage in Drawables Post Kitkat
4 participants