-
Notifications
You must be signed in to change notification settings - Fork 528
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
App crashes when trying to access In-lesson options #4712
Comments
Can I work on this please ? |
Thanks @iampradeep-hr. While this would normally be a great issue to start working on, it's time sensitive since it's blocking a real that's about to go out so unfortunately it may not be the best problem to work on right now. I suggest looking at other issues which aren't part of a release milestone (which you can see on the right side of the issue's details under "Milestone"). |
Okay, so it seems this only repros on Proguarded-builds. Stack trace:
I think this is the same issue the monkey tests found on our pre-launch report. It's definitely tied to the new options bottom sheet--I suspect something is relying on a direct name being present & Proguard is obfuscating it. |
This makes me sad: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:coordinatorlayout/coordinatorlayout/src/main/java/androidx/coordinatorlayout/widget/CoordinatorLayout.java;l=678?q=coordinatorlayout. It seems Jetpack relies on reflection for Behavior subclasses. :( |
Confirming that keeping subclasses of androidx.coordinatorlayout.widget.CoordinatorLayout/Behavior from being renamed fixes the problem (and ensuring subclass constructors are never removed). Unfortunately, we can't test this without end-to-end tests, so we'll need rely on manual testing for verification & continued regression testing. |
…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,
…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,
Describe the bug
In-Lesson options are not working
To Reproduce
Steps to reproduce the behavior:
Expected behavior
Reading text size and Default Audio options must be seen
Actual behavior:
App crashes or reopens the lesson page
Environment
app.crash.mp4
The text was updated successfully, but these errors were encountered: