Conversation
Summary of ChangesHello @rteyssandier, 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 delivers a new 'ADB Commands' feature, enhancing the application's utility by providing a centralized interface for executing and managing Android Debug Bridge commands. The changes span across the data, domain, and presentation layers, ensuring a robust and integrated experience for users to interact with their connected Android devices through custom commands. Highlights
Changelog
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
|
![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 a new feature for executing ADB commands, which is a great addition. The implementation follows a clean architecture, separating concerns into presentation, domain, and data layers, and includes database persistence for saved commands. My review focuses on improving UI theme adherence for colors, which will enhance robustness against future theme changes, and some minor code refinements for better clarity and use of idiomatic Kotlin. Overall, this is a solid contribution.
| Text( | ||
| "Saved Commands", | ||
| color = FloconTheme.colorPalette.onSurface, | ||
| style = | ||
| FloconTheme.typography.bodyMedium.copy( | ||
| fontWeight = FontWeight.Bold | ||
| ), | ||
| maxLines = 1, | ||
| overflow = TextOverflow.Ellipsis, | ||
| modifier = Modifier.weight(1f) | ||
| ) |
There was a problem hiding this comment.
The background of this component is FloconTheme.colorPalette.primary, but the text color is set to FloconTheme.colorPalette.onSurface. According to the project's general rules, content on a primary background should use the onPrimary color to ensure proper contrast and theme consistency.
| Text( | |
| "Saved Commands", | |
| color = FloconTheme.colorPalette.onSurface, | |
| style = | |
| FloconTheme.typography.bodyMedium.copy( | |
| fontWeight = FontWeight.Bold | |
| ), | |
| maxLines = 1, | |
| overflow = TextOverflow.Ellipsis, | |
| modifier = Modifier.weight(1f) | |
| ) | |
| Text( | |
| "Saved Commands", | |
| color = FloconTheme.colorPalette.onPrimary, | |
| style = | |
| FloconTheme.typography.bodyMedium.copy( | |
| fontWeight = FontWeight.Bold | |
| ), | |
| maxLines = 1, | |
| overflow = TextOverflow.Ellipsis, | |
| modifier = Modifier.weight(1f) | |
| ) |
References
- When a component's background color is
primary, its content color should beonPrimaryfor semantic correctness and to ensure robustness against future theme changes.
| Text( | ||
| "History", | ||
| color = FloconTheme.colorPalette.onSurface, | ||
| style = FloconTheme.typography.bodyMedium.copy(fontWeight = FontWeight.Bold), | ||
| modifier = Modifier.padding(16.dp) |
There was a problem hiding this comment.
The background of this component is FloconTheme.colorPalette.primary, but the text color is set to FloconTheme.colorPalette.onSurface. To adhere to the project's theming rules, the text color should be onPrimary when on a primary background.
| Text( | |
| "History", | |
| color = FloconTheme.colorPalette.onSurface, | |
| style = FloconTheme.typography.bodyMedium.copy(fontWeight = FontWeight.Bold), | |
| modifier = Modifier.padding(16.dp) | |
| Text( | |
| "History", | |
| color = FloconTheme.colorPalette.onPrimary, | |
| style = FloconTheme.typography.bodyMedium.copy(fontWeight = FontWeight.Bold), | |
| modifier = Modifier.padding(16.dp) | |
| ) |
References
- When a component's background color is
primary, its content color should beonPrimaryfor semantic correctness and to ensure robustness against future theme changes.
| Text( | ||
| text = "adb ${item.command}", | ||
| style = FloconTheme.typography.labelLarge, | ||
| color = FloconTheme.colorPalette.onPrimary, | ||
| modifier = Modifier.weight(1f) | ||
| ) |
There was a problem hiding this comment.
The text color is onPrimary, but the background is a variation of the secondary color. For proper theme adherence and to ensure good contrast, the text color should be onSecondary to match the background.
| Text( | |
| text = "adb ${item.command}", | |
| style = FloconTheme.typography.labelLarge, | |
| color = FloconTheme.colorPalette.onPrimary, | |
| modifier = Modifier.weight(1f) | |
| ) | |
| Text( | |
| text = "adb ${item.command}", | |
| style = FloconTheme.typography.labelLarge, | |
| color = FloconTheme.colorPalette.onSecondary, | |
| modifier = Modifier.weight(1f) | |
| ) |
| Text( | ||
| text = item.output, | ||
| style = FloconTheme.typography.bodySmall, | ||
| color = FloconTheme.colorPalette.onSecondary, | ||
| modifier = | ||
| Modifier.fillMaxWidth() | ||
| .background( | ||
| FloconTheme.colorPalette.primary.copy(alpha = 0.3f) | ||
| ) | ||
| .padding(4.dp) | ||
| ) |
There was a problem hiding this comment.
The background for this text is a variation of the primary color, but the text color is onSecondary. Based on the project's general rules, the text color should be onPrimary to ensure it's readable and consistent with the theme.
| Text( | |
| text = item.output, | |
| style = FloconTheme.typography.bodySmall, | |
| color = FloconTheme.colorPalette.onSecondary, | |
| modifier = | |
| Modifier.fillMaxWidth() | |
| .background( | |
| FloconTheme.colorPalette.primary.copy(alpha = 0.3f) | |
| ) | |
| .padding(4.dp) | |
| ) | |
| Text( | |
| text = item.output, | |
| style = FloconTheme.typography.bodySmall, | |
| color = FloconTheme.colorPalette.onPrimary, | |
| modifier = | |
| Modifier.fillMaxWidth() | |
| .background( | |
| FloconTheme.colorPalette.primary.copy(alpha = 0.3f) | |
| ) | |
| .padding(4.dp) | |
| ) |
References
- When a component's background color is
primary, its content color should beonPrimaryfor semantic correctness and to ensure robustness against future theme changes.
| viewModelScope.launch(Dispatchers.IO) { | ||
| val currentDevice = currentDeviceUseCase() | ||
| val isConnected = observeIsCurrentActiveDevicesUseCase().firstOrNull() | ||
| if ((isConnected == false) or (currentDevice == null)) { |
There was a problem hiding this comment.
The or infix function is being used for a logical condition. While it works here, the idiomatic way to perform a logical OR in Kotlin is with the || operator. The || operator also provides short-circuiting, which is generally safer and more performant for conditional logic.
| if ((isConnected == false) or (currentDevice == null)) { | |
| if (isConnected == false || currentDevice == null) { |
| @Dao | ||
| interface AdbCommandDao { | ||
|
|
||
| @Upsert suspend fun upsertAll(commands: List<AdbCommandEntity>) |
There was a problem hiding this comment.
Since the data source method insertOrReplace operates on a single item, it's more direct and efficient for the DAO to have an upsert method for a single entity rather than upsertAll. This avoids the need to wrap a single item in a list in the data source. This change should be applied along with the corresponding change in AdbCommandLocalDataSourceImpl.
| @Upsert suspend fun upsertAll(commands: List<AdbCommandEntity>) | |
| @Upsert suspend fun upsert(command: AdbCommandEntity) |
| AdbCommandLocalDataSource { | ||
|
|
||
| override suspend fun insertOrReplace(adbCommand: AdbCommand) { | ||
| dao.upsertAll(listOf(adbCommand.toLocal())) |
There was a problem hiding this comment.
Following the suggested change in AdbCommandDao to use upsert for a single entity, this call can be simplified to avoid creating an unnecessary list.
| dao.upsertAll(listOf(adbCommand.toLocal())) | |
| dao.upsert(adbCommand.toLocal()) |
1 participant
No description provided.