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

Fixes #4712: Ensure CoordinatorLayout Behaviors are not renamed/removed during binary optimizations #4731

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Nov 17, 2022

Explanation

Fixes #4712

Proguard was renaming CoordinatorLayout behaviors and removing implementation constructors, where both can be referenced via reflection in CoordinatorLayout. We haven't run into this issue before since it seems the new bottom navigation component introduced in #4482 is the first time we actually started using CoordinatorLayout behaviors.

Note that this is inherently difficult to test as it requires verifying runtime functionality in a Proguard-optimized build. Hence, it's only testable with end-to-end tests (and our own end-to-end tests currently are manual tests) so no new unit/integration tests are being added.

Note also that this PR will be cherry-picked for the upcoming beta MR2 release.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

This doesn't seem particularly valuable to fill out here--the video would just show the options menu opening as it should (and does for developer builds). I've verified it works locally, and #4712 will require reverifying once the PR is merged,

@BenHenning BenHenning changed the title Fixes #4712: Ensures CoordinatorLayout Behaviors are not renamed/removed during binary optimizations Fixes #4712: Ensure CoordinatorLayout Behaviors are not renamed/removed during binary optimizations Nov 17, 2022
@BenHenning BenHenning requested a review from seanlip November 17, 2022 02:58
@BenHenning BenHenning marked this pull request as ready for review November 17, 2022 02:58
@BenHenning
Copy link
Member Author

Sending this into review right away--PTAL @seanlip. We don't have CI checks that affect the Proguard rules except for the binary builds, and since I've already verified beta is building it's extremely unlikely we'll have issues with the alpha or GA builds. There's not much value waiting on CI checks to pass before reviewing, but I'll still wait on them prior to merging.

@oppiabot oppiabot bot unassigned seanlip Nov 17, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 17, 2022

Unassigning @seanlip since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Nov 17, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 17, 2022

Hi @BenHenning, 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!

@BenHenning
Copy link
Member Author

Thanks @seanlip!

@BenHenning BenHenning merged commit 252c077 into develop Nov 17, 2022
@BenHenning BenHenning deleted the fix-runtime-proguard-issue branch November 17, 2022 17:27
@BenHenning BenHenning added the PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. label Nov 17, 2022
BenHenning added a commit that referenced this pull request Nov 18, 2022
…ed during binary optimizations (#4731)

## Explanation
Fixes #4712

Proguard was renaming CoordinatorLayout behaviors and removing
implementation constructors, where both can be referenced via reflection
in CoordinatorLayout. We haven't run into this issue before since it
seems the new bottom navigation component introduced in #4482 is the
first time we actually started using CoordinatorLayout behaviors.

Note that this is inherently difficult to test as it requires verifying
runtime functionality in a Proguard-optimized build. Hence, it's only
testable with end-to-end tests (and our own end-to-end tests currently
are manual tests) so no new unit/integration tests are being added.

Note also that this PR will be cherry-picked for the upcoming beta MR2
release.

## Essential Checklist
- [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
This doesn't seem particularly valuable to fill out here--the video
would just show the options menu opening as it should (and does for
developer builds). I've verified it works locally, and #4712 will
require reverifying once the PR is merged,
@BenHenning BenHenning added the PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. label Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. PR: LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App crashes when trying to access In-lesson options
2 participants