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: Fix crashes by ensuring layout ID is set before inflating Camera View stub + raise AGP for Kotlin 1.7.0 compatibility #4794

Merged
merged 9 commits into from
Jul 19, 2022

Conversation

chris-hatton
Copy link
Contributor

@chris-hatton chris-hatton commented Jul 7, 2022

Description

Code was attempting to set the visibility of the View Camera stub before its source layout XML id was set.
This is an error because setting a View visible causes inflate() to be run in case it is not already inflated.
With no XML id set; there is nothing to inflate and so subsequent code that assumes a view to be present fails.

Simply switching around the order of operations here...

  1. Setting the layout ID
  2. Inflating
  3. Setting visibility

... resolves the issue.

Related issues

Fixes #4685, #4686

Related PRs

Unblocks testing of #4777

Screenshots

N/A

Link to the automatically generated build APK

N/A

@chris-hatton chris-hatton requested a review from a team as a code owner July 7, 2022 14:20
@teolemon teolemon changed the title Fix crashes by inflating Camera View stub before attempting to set visibility fix: Fix crashes by inflating Camera View stub before attempting to set visibility Jul 7, 2022
@chris-hatton
Copy link
Contributor Author

The CI linter is failing this build with errors in the pattern:

Error: Calling new methods on older versions: Call requires API level 24 (current min is 21): `<some kotlin stdlib method>`

...given this error is unrelated to the changes in the PR; it seems likely this is caused by an environmental change.
Has the linter been updated or reconfigured independently of the source code?

A direct answer to this error seems to be raising the min API level to 24, but I'm not sure that's expected or desirable. Other maintainers please advise.

@chris-hatton chris-hatton changed the title fix: Fix crashes by inflating Camera View stub before attempting to set visibility fix: Fix crashes by ensuring layout ID is set before inflating Camera View stub + raise min API to 24 Jul 9, 2022
@chris-hatton
Copy link
Contributor Author

chris-hatton commented Jul 9, 2022

I've added raising min API level to 24 for expedience. Apparently, combining API 21 with JDK 8 stdlib (as current config implies) is a genuine error and probably should have been addressed before.

If it's important to keep API 21 compatibility then I can edit this PR again, but we'd need to explicitly use stdlib-jdk7 and probably revisit the use of some JDK 8 stdlib functions throughout the code.

@VaiTon
Copy link
Member

VaiTon commented Jul 9, 2022

I've added raising min API level to 24 for expedience. Apparently, combining API 21 with JDK 8 stdlib (as current config implies) is a genuine error and probably should have been addressed before.

If it's important to keep API 21 compatibility then I can edit this PR again, but we'd need to explicitly use stdlib-jdk7 and probably revisit the use of some JDK 8 stdlib functions throughout the code.

I think that we'd like to keep API 21 as min for now. If you could do the changes described to keep the minSdk to 21 it would be great!

@chris-hatton chris-hatton changed the title fix: Fix crashes by ensuring layout ID is set before inflating Camera View stub + raise min API to 24 fix: Fix crashes by ensuring layout ID is set before inflating Camera View stub + raise AGP for Kotlin 1.7.0 compatibility Jul 10, 2022
@chris-hatton
Copy link
Contributor Author

chris-hatton commented Jul 10, 2022

Thanks @VaiTon ; looking again at the errors, I discovered they actually stem from incompatibility between the linter in Android Gradle Plugin 7.0.4 and Kotlin 1.7. The linter thinks the source is trying to use JDK 8 Stream operators instead of those included in the Kotlin stdlib. I've applied a fix to upgrade to the latest AGP 7.2.1, which is probably advisable anyway.

@NikNoori
Copy link

Any update on this issue? Thanks.

@chris-hatton
Copy link
Contributor Author

chris-hatton commented Jul 17, 2022

Any update on this issue? Thanks.

The crash fix itself was straightforward.
The complication with having it pass in CI stems from bit-rot: The minimally fixed code wouldn't build in CI because of (apparently) some unrelated change in the build environment. That can be resolved by upgrading to Gradle 7.2.1 which entailed at least one further breaking change in the project. Still working on it, just time poor.

@VaiTon I have applied a fix for the most recent run failure: un-mergeable Android Manifest of androidTestScreenshots module due to incorrect package name. It may work next time if you can kindly re-run it.

app/build.gradle.kts Outdated Show resolved Hide resolved
@VaiTon
Copy link
Member

VaiTon commented Jul 19, 2022

@chris-hatton I've reverted the minSdk change

@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@VaiTon VaiTon merged commit a39d366 into openfoodfacts:develop Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
4 participants