Feature/kathleen audio flow#169
Conversation
kathleenalexandra24
requested review from
JolandaVerhoef,
calren,
ksemenova and
lethargicpanda
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for an AI glasses experience in the Gemini Live To-Do sample. Key changes include the addition of a new GlassesActivity, specialized UI components for visual and audio modes on glasses, and updates to the TodoRepository and TodoScreenViewModel to manage live sessions and microphone status. Feedback focuses on correcting architectural violations where Activity references are passed to or held by the ViewModel, improving encapsulation of the LiveSession, and removing redundant dependencies and logic within the repository.
| private val TAG = "TodoScreenViewModel" | ||
| private var session: LiveSession? = null | ||
| internal var session: LiveSession? = null | ||
| private var hostActivityRef: WeakReference<Activity>? = null |
There was a problem hiding this comment.
ViewModels should not hold references to Activity objects, even via WeakReference. This violates the principle of ViewModels being agnostic of the Android lifecycle and can lead to memory leaks or crashes if the Activity is recreated. Consider using a more robust way to handle Activity-dependent actions, such as a shared Flow or a callback mechanism initiated from the UI layer.
References
- ViewModels should be agnostic of the Android lifecycle. Avoid references to lifecycle-related types, Activities, Fragments, Context, or Resources. (link)
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
|
|
||
| viewModel.initializeGeminiLive(this) |
There was a problem hiding this comment.
Passing an Activity instance to a ViewModel function is strongly discouraged as it can lead to memory leaks and violates the separation of concerns between the UI and data layers. The ViewModel should be agnostic of the Activity lifecycle and should not depend on UI-specific types like Activity.
References
- ViewModel functions should never take an activity as a parameter to avoid memory leaks. (link)
| class TodoScreenViewModel @Inject constructor(private val todoRepository: TodoRepository) : ViewModel() { | ||
| private val TAG = "TodoScreenViewModel" | ||
| private var session: LiveSession? = null | ||
| internal var session: LiveSession? = null |
There was a problem hiding this comment.
The 'session' property should be private to encapsulate the implementation details of the live session. Exposing it as 'internal' allows the UI layer to bypass the ViewModel and interact with the session directly, which violates the Unidirectional Data Flow (UDF) principle and separation of concerns.
| internal var session: LiveSession? = null | |
| private var session: LiveSession? = null |
| scope.launch { | ||
| // Give Gemini Live a moment to open the channel | ||
| kotlinx.coroutines.delay(500) | ||
| viewModel.session?.send(content { text("Turning Microphone On") }) |
There was a problem hiding this comment.
Directly accessing and calling 'send' on the ViewModel's session object from the UI layer breaks encapsulation and the observer pattern. The ViewModel should provide a dedicated method for sending voice feedback or commands to maintain a clean separation of concerns and adhere to Unidirectional Data Flow (UDF).
|
|
||
| implementation(libs.androidx.xr.glimmer) | ||
| implementation(libs.androidx.xr.projected) | ||
| implementation("com.google.firebase:firebase-ai:17.5.0") |
There was a problem hiding this comment.
This dependency is redundant as 'libs.firebase.ai' is already included on line 62. It is recommended to use the version catalog ('libs') for all dependencies to ensure consistency and easier version management across the project.
| statusText = "Mic Status", | ||
| isMicOn = false | ||
| ), | ||
| ).filterNot { it.id == MIC_STATUS_TODO_ID }, |
There was a problem hiding this comment.
The filter operation here appears to be redundant because 'MIC_STATUS_TODO_ID' (-999) is not present in the preceding list. This logic can be simplified by removing the filter call.
| ).filterNot { it.id == MIC_STATUS_TODO_ID }, | |
| ) |
|
Refer to #168 instead |
5 participants
No description provided.