-
Notifications
You must be signed in to change notification settings - Fork 138
Update static assets URLs #147
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
Conversation
Summary of ChangesHello @yrezgui, 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! This pull request focuses on standardizing the source of static video assets used within the Android AI samples. By updating the URLs to a unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
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 correctly updates the URLs for static video assets across the sample applications. However, this change introduces significant duplication of hardcoded URL strings in multiple files. My review includes suggestions to refactor these URLs into constants to improve the code's maintainability and make future updates easier and less error-prone, following the DRY (Don't Repeat Yourself) principle.
| "https://storage.googleapis.com/androiddevelopers/samples_assets/exoplayer-test-media-0/shorts_android_developers/shorts_10.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_big_buck_bunny, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/BigBuckBunny.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_tears_of_steel, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/TearsOfSteel.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/TearsOfSteel.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_for_bigger_blazes, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerBlazes.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/ForBiggerBlazes.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_for_bigger_escape, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerEscapes.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/ForBiggerEscapes.mp4".toUri(), |
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.
To improve maintainability and avoid repeating the same URLs, consider extracting these hardcoded video URLs into constants. You could define a common base URL and then construct the full URLs from it. This would make future updates much easier.
For example:
private const val BASE_URL = "https://storage.googleapis.com/androiddevelopers/samples_assets"
private const val GTV_BUCKET_URL = "$BASE_URL/gtv-videos-bucket/sample"
private const val BIG_BUCK_BUNNY_URL = "$GTV_BUCKET_URL/BigBuckBunny.mp4"
// ... and so on for other videosThen you can use BIG_BUCK_BUNNY_URL.toUri() in your VideoItem definitions.
References
- To adhere to the Don't Repeat Yourself (DRY) principle, avoid duplicating hardcoded values. Extracting them into constants improves maintainability and reduces the risk of errors during updates.
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/BigBuckBunny.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_android_spotlight_shorts, | ||
| "https://storage.googleapis.com/exoplayer-test-media-0/shorts_android_developers/shorts_10.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/exoplayer-test-media-0/shorts_android_developers/shorts_10.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_youtube_google_tv, | ||
| "https://www.youtube.com/watch?v=QFMIP5GOo70".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_tears_of_steel, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/TearsOfSteel.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/TearsOfSteel.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_for_bigger_blazes, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerBlazes.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/ForBiggerBlazes.mp4".toUri(), | ||
| ), | ||
| VideoItem( | ||
| R.string.video_title_for_bigger_escape, | ||
| "https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerEscapes.mp4".toUri(), | ||
| "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/ForBiggerEscapes.mp4".toUri(), |
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.
Similar to the other video list, these hardcoded URLs are duplicated. To improve maintainability, it's recommended to extract them into constants. This will centralize the URLs and make them easier to manage and update.
For example:
private const val BASE_URL = "https://storage.googleapis.com/androiddevelopers/samples_assets"
private const val GTV_BUCKET_URL = "$BASE_URL/gtv-videos-bucket/sample"
private const val BIG_BUCK_BUNNY_URL = "$GTV_BUCKET_URL/BigBuckBunny.mp4"
// ... and so on for other videosThis approach reduces redundancy and the risk of inconsistencies.
References
- To adhere to the Don't Repeat Yourself (DRY) principle, avoid duplicating hardcoded values. Extracting them into constants improves maintainability and reduces the risk of errors during updates.
| VideoPickerData("Big Buck Bunny", "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/BigBuckBunny.mp4".toUri()), | ||
| VideoPickerData("Tears of Steel", "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/TearsOfSteel.mp4".toUri()), | ||
| VideoPickerData("For Bigger Blazes", "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample/ForBiggerBlazes.mp4".toUri()), |
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.
These sample video URLs are hardcoded and duplicated in other parts of the project. To improve code maintainability and reduce redundancy, consider extracting these URLs into constants. Ideally, these constants would live in a shared location so they can be reused across all sample lists.
Example:
private const val BASE_URL = "https://storage.googleapis.com/androiddevelopers/samples_assets/gtv-videos-bucket/sample"
private const val BIG_BUCK_BUNNY_URL = "$BASE_URL/BigBuckBunny.mp4"References
- To adhere to the Don't Repeat Yourself (DRY) principle, avoid duplicating hardcoded values. Extracting them into constants improves maintainability and reduces the risk of errors during updates.
lethargicpanda
left a comment
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.
can you remove the changes in the geminilivetodo sample.
I think they are just linting/detekt changes but to make this PR cleaner, let's remove it and we'll fix it separately.
|
|
||
| import kotlin.random.Random | ||
|
|
||
|
|
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.
Let's remove this, I will run detekt in a separate PR.
...mini-live-todo/src/main/java/com/android/ai/samples/geminilivetodo/ui/TodoScreenViewModel.kt
Show resolved
Hide resolved
| fun getTodoList(): List<Todo> = _todos.value | ||
|
|
||
| fun addTodo(taskDescription: String) : Int? { | ||
| fun addTodo(taskDescription: String): Int? { |
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.
Let's remove this.
No description provided.