-
Notifications
You must be signed in to change notification settings - Fork 141
Xr feature #161
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
base: p-and-d
Are you sure you want to change the base?
Xr feature #161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @sashalukin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here to provide a summary of this pull request. This PR, titled "Xr feature", appears to introduce support for Extended Reality (XR) features into the application. Although there's no detailed description, the changes indicate the integration of AndroidX XR libraries, specifically those related to Compose and Material3 components. The implementation includes adding necessary dependencies, updating the Android Manifest to handle potential minSdk conflicts for these new libraries, and conditionally enabling XR component overrides in the main activity based on the Android SDK version.
Highlights
- Add XR Dependency: The
androidx.xr.compose.material3library is added as a dependency to the application module. - Manifest Updates for XR Libraries: Several
androidx.xrandcom.google.arlibraries are added to thetools:overrideLibrarylist in theAndroidManifest.xml. A comment clarifies that this is safe because XR features are intended for SDK level 34 (UPSIDE_DOWN_CAKE) and above. - Conditional XR Feature Enabling: In
MainActivity.kt, the main content composable (Main) is wrapped in a conditional block. If the device's SDK version is 34 or higher,EnableXrComponentOverridesis used to wrap the content, presumably enabling XR-specific behaviors for Compose Material3 components. - Gradle Version Catalog Update: The
gradle/libs.versions.tomlfile is updated to define the version forandroidx.xr.compose.material3and add an alias for the library.
Changelog
Click here to see the changelog
- app/build.gradle.kts
- Added
implementation(libs.androidx.xr.material3)dependency (line 90).
- Added
- app/src/main/AndroidManifest.xml
- Added a comment explaining the minSdk override for XR features on SDK >= 34 (lines 37-38).
- Expanded the
tools:overrideLibrarylist to include severalandroidx.xrandcom.google.arlibraries (lines 40-43).
- app/src/main/java/com/google/android/samples/socialite/MainActivity.kt
- Added imports for
EnableXrComponentOverridesandExperimentalMaterial3XrApi(lines 33-34). - Added
@OptIn(ExperimentalMaterial3XrApi::class)annotation to theMainActivityclass (line 40). - Wrapped the
setContentblock's call toMainwithin anif (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE)check (lines 55-65). - Inside the SDK check, wrapped the
Maincomposable withEnableXrComponentOverrides(lines 56-60).
- Added imports for
- gradle/libs.versions.toml
- Added
xrMaterial3Version = "1.0.0-alpha07"to define the version for the XR Material3 library (line 58). - Added
androidx-xr-material3library alias pointing to the new version (line 131).
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
New code arrives,
For worlds unseen, quite grand,
SDK check first.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces XR (Extended Reality) capabilities by adding the androidx.xr.material3 dependency and setting up the necessary configurations in AndroidManifest.xml and MainActivity.kt. The changes are well-targeted, and the conditional enabling of XR features based on SDK version is correctly implemented.
A couple of minor points are noted for consideration, with one suggestion for a small refactor to improve code clarity in MainActivity.kt.
It would also be beneficial for future pull requests to include a brief description of the changes and their purpose, as the current PR description is empty. This helps reviewers and future maintainers understand the context more quickly.
Summary of Findings
- Code Duplication in MainActivity: The
Main(...)composable invocation is duplicated within thesetContentblock inMainActivity.kt. A refactor to define it once and call it in both branches of the SDK version check would improve maintainability. A review comment with a suggestion has been provided (medium severity). - Scope of
tools:overrideLibraryin AndroidManifest (Not commented due to severity settings): Thetools:overrideLibrarylist inAndroidManifest.xmlis broad, including several XR-related libraries beyond the directly addedandroidx.xr.compose.material3. While this is likely intentional for future XR feature integration and is safe due to runtime SDK checks, ensuring all listed libraries are indeed planned for use or specifically require this override could enhance manifest clarity. This was considered low severity and not commented on directly due to review settings. - Alpha Dependency Version (Not commented due to severity settings): The
androidx.xr.material3dependency is introduced at version1.0.0-alpha07. Using alpha versions is acceptable, particularly in sample applications or for experimental features, but it carries an inherent risk of API changes and potential instability. This was considered low severity and not commented on directly due to review settings.
Merge Readiness
The pull request is generally in good shape and introduces the XR feature foundation correctly. There is one medium severity suggestion regarding code duplication in MainActivity.kt that would be good to address for improved maintainability. Once that is considered, the PR should be in a better state for merging. As a reviewer, I am not authorized to approve the pull request; please ensure it undergoes further review and approval as per your team's process.
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) { | ||
| EnableXrComponentOverrides { | ||
| Main( | ||
| appArgs = extractAppArgs(intent), | ||
| ) | ||
| } | ||
| } else { | ||
| Main( | ||
| appArgs = extractAppArgs(intent), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Main(appArgs = extractAppArgs(intent)) call is duplicated in both branches of the if/else statement. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, could we refactor this block to define the Main composable invocation once and then use it within the conditional EnableXrComponentOverrides wrapper and the else block?
This would make the code slightly cleaner and easier to modify if the arguments or structure of the Main call changes in the future.
val mainUi = @Composable {
Main(
appArgs = extractAppArgs(intent),
)
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
EnableXrComponentOverrides {
mainUi()
}
} else {
mainUi()
}|
Please rebase this branch and fix the conflicts! |
No description provided.