Conversation
Summary of ChangesHello @florent37, 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 significantly enhances the Flocon Desktop application by introducing a comprehensive performance monitoring feature. It provides developers with a powerful tool to analyze the runtime behavior of their Android applications, offering real-time insights into critical metrics like FPS, memory, and jank through an intuitive interface that includes both detailed event logs and graphical trends. This addition streamlines the process of identifying and addressing performance bottlenecks, thereby improving the overall quality and responsiveness of Android applications. Highlights
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 performance monitoring screen, which is a great addition. The implementation includes fetching metrics like FPS, RAM, and battery from a connected device via ADB and displaying them in a new window with charts and a detailed list. The code is well-structured with new ViewModels, UseCases, and UI components.
I've left several comments with suggestions for improvement, focusing on:
- Correctness: Fixing a potential logic issue in the FPS calculation for idle screens and an incorrect unit conversion for RAM in charts.
- Efficiency: Improving how performance metrics are stored and accessed to avoid potential bottlenecks.
- Maintainability & Best Practices: Addressing hardcoded paths and colors by suggesting the use of standard application directories and theme values, and ensuring semantic color usage where applicable.
- UI/UX: Minor improvements to UI layout flexibility and clarity in data presentation.
Overall, this is a solid feature, and with a few adjustments, it will be even better. Great work!
| it.rawRamMb?.toDouble()?.let { it / 1024.0 }?.let { | ||
| ram.add(it) |
There was a problem hiding this comment.
The RAM usage for the graph is calculated in Kilobytes (it / 1024.0), but the chart title in PerformanceChartView is "RAM Usage (MB)". This is misleading for the user as rawRamMb holds the value in bytes. To display it in Megabytes, you should divide by 1024.0 * 1024.0.
| it.rawRamMb?.toDouble()?.let { it / 1024.0 }?.let { | |
| ram.add(it) | |
| it.rawRamMb?.toDouble()?.let { it / (1024.0 * 1024.0) }?.let { | |
| ram.add(it) |
| if (frameDelta > 0) { | ||
| (frameDelta / timeDeltaSeconds).coerceIn(0.0, refreshRate) | ||
| } else { | ||
| // If no new frames, assume it's running at refresh rate (Idle) | ||
| refreshRate | ||
| } | ||
| } else refreshRate | ||
| } else { | ||
| refreshRate // Default to refresh rate for the first fetch or if data is missing | ||
| } |
There was a problem hiding this comment.
The current FPS calculation for idle or static screens seems to be incorrect. When no new frames are rendered (frameDelta is not positive), or for the very first measurement, the function defaults to returning the device's refreshRate. This will report high FPS (e.g., 60 FPS) for a completely static screen, which should be closer to 0 FPS. This will significantly skew the average FPS metric upwards and misrepresent the application's performance during idle periods.
Consider returning 0.0 when frameDelta is not positive. For the initial measurement where deltas are not available, you could also return 0.0.
| if (frameDelta > 0) { | |
| (frameDelta / timeDeltaSeconds).coerceIn(0.0, refreshRate) | |
| } else { | |
| // If no new frames, assume it's running at refresh rate (Idle) | |
| refreshRate | |
| } | |
| } else refreshRate | |
| } else { | |
| refreshRate // Default to refresh rate for the first fetch or if data is missing | |
| } | |
| if (frameDelta > 0) { | |
| (frameDelta / timeDeltaSeconds).coerceIn(0.0, refreshRate) | |
| } else { | |
| // If no new frames are rendered, FPS is 0. | |
| 0.0 | |
| } | |
| } else 0.0 // Avoid division by zero, returning 0 FPS. | |
| } else { | |
| 0.0 // Cannot calculate on first run, return 0. | |
| } |
|
|
||
| Box(modifier = Modifier.fillMaxSize().weight(1f)) { | ||
| if (points.size >= 2) { | ||
| KoalaPlotTheme(axis = KoalaPlotTheme.axis.copy(color = Color.Black, minorGridlineStyle = null)) { |
There was a problem hiding this comment.
The axis color for the chart is hardcoded to Color.Black. To ensure the chart adapts correctly to different themes (e.g., a future dark theme), you should use colors from the FloconTheme.
| KoalaPlotTheme(axis = KoalaPlotTheme.axis.copy(color = Color.Black, minorGridlineStyle = null)) { | |
| KoalaPlotTheme(axis = KoalaPlotTheme.axis.copy(color = FloconTheme.colorPalette.onSurface, minorGridlineStyle = null)) { |
| ) | ||
| } | ||
| Text( | ||
| modifier = Modifier.width(140.dp), |
There was a problem hiding this comment.
Using a fixed width(140.dp) for this column makes the layout rigid. If the window is resized or content overflows, it may not look right. Consider using Modifier.weight() on this and the other Text composables in the Row to create a more flexible layout that distributes space proportionally.
| modifier = Modifier.width(140.dp), | |
| modifier = Modifier.weight(1f), |
| color = FloconTheme.colorPalette.primary, | ||
| contentColor = FloconTheme.colorPalette.onPrimary, | ||
| ) { | ||
| Row( | ||
| modifier = Modifier.padding(horizontal = 12.dp, vertical = 4.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(16.dp), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Box( | ||
| modifier = Modifier.width(imageSize).background(Color.Red), | ||
| ) | ||
| Text( | ||
| modifier = Modifier.width(140.dp), | ||
| text = "Time", | ||
| style = bodySmall, | ||
| color = FloconTheme.colorPalette.onSurface.copy(alpha = 0.5f) | ||
| ) |
There was a problem hiding this comment.
The FloconSurface has its color set to FloconTheme.colorPalette.primary, and its contentColor is correctly set to FloconTheme.colorPalette.onPrimary. However, the "Time" Text composable inside explicitly sets its color to a derivative of onSurface. This violates the semantic color system and the general rule: "When a component's background color is primary, its content color should be onPrimary". To maintain consistency and robustness against theme changes, you should derive the color from onPrimary.
| color = FloconTheme.colorPalette.primary, | |
| contentColor = FloconTheme.colorPalette.onPrimary, | |
| ) { | |
| Row( | |
| modifier = Modifier.padding(horizontal = 12.dp, vertical = 4.dp), | |
| horizontalArrangement = Arrangement.spacedBy(16.dp), | |
| verticalAlignment = Alignment.CenterVertically | |
| ) { | |
| Box( | |
| modifier = Modifier.width(imageSize).background(Color.Red), | |
| ) | |
| Text( | |
| modifier = Modifier.width(140.dp), | |
| text = "Time", | |
| style = bodySmall, | |
| color = FloconTheme.colorPalette.onSurface.copy(alpha = 0.5f) | |
| ) | |
| color = FloconTheme.colorPalette.primary, | |
| contentColor = FloconTheme.colorPalette.onPrimary, | |
| ) { | |
| Row( | |
| modifier = Modifier.padding(horizontal = 12.dp, vertical = 4.dp), | |
| horizontalArrangement = Arrangement.spacedBy(16.dp), | |
| verticalAlignment = Alignment.CenterVertically | |
| ) { | |
| Box( | |
| modifier = Modifier.width(imageSize).background(Color.Red), | |
| ) | |
| Text( | |
| modifier = Modifier.width(140.dp), | |
| text = "Time", | |
| style = bodySmall, | |
| color = FloconTheme.colorPalette.onPrimary.copy(alpha = 0.5f) | |
| ) |
References
- When a component's background color is
primary, its content color should beonPrimaryfor semantic correctness and to ensure robustness against future theme changes.
| PerformanceChartView( | ||
| title = "RAM Usage (MB)", | ||
| data = graphs.ram, | ||
| color = Color.Yellow, |
There was a problem hiding this comment.
The chart color is hardcoded to Color.Yellow. To ensure consistency and better support for theming (like dark mode), consider defining this color in your FloconTheme and referencing it from there.
| PerformanceChartView( | ||
| title = "FPS", | ||
| data = graphs.fps, | ||
| color = Color.Red, |
There was a problem hiding this comment.
The chart color is hardcoded to Color.Red. To ensure consistency and better support for theming (like dark mode), consider defining this color in your FloconTheme and referencing it from there.
|
|
||
| fun clearTmpFiles() { | ||
| try { | ||
| val performancesDir = Paths.get(System.getProperty("user.home"), "Desktop", "Flocon", "performances").toFile() |
There was a problem hiding this comment.
The path for temporary performance files is hardcoded to the user's Desktop (~/Desktop/Flocon/performances). This has several issues:
- It pollutes the user's desktop with application files.
- The folder name "Desktop" is language-dependent and may not exist on all systems.
- It's not the standard location for application data or cache.
A better approach is to use a dedicated application data directory. You can construct a path within the user's home directory that is specific to your application, for example in a hidden folder like ~/.flocon/cache/performances.
| val performancesDir = Paths.get(System.getProperty("user.home"), "Desktop", "Flocon", "performances").toFile() | |
| val performancesDir = Paths.get(System.getProperty("user.home"), ".flocon", "cache", "performances").toFile() |
| val fileName = "perf_screenshot_${packageName}_$timestamp.png" | ||
| val onDeviceFilePath = "/sdcard/$fileName" | ||
|
|
||
| val metricsDir = Paths.get(System.getProperty("user.home"), "Desktop", "Flocon", "performances", "${deviceSerial}_$packageName").toFile() |
There was a problem hiding this comment.
The path for saving performance screenshots is hardcoded to the user's Desktop (~/Desktop/Flocon/performances). This has several issues:
- It pollutes the user's desktop with application files.
- The folder name "Desktop" is language-dependent and may not exist on all systems.
- It's not the standard location for application data or cache.
A better approach is to use a dedicated application data directory, for example in a hidden folder like ~/.flocon/cache/performances.
| val metricsDir = Paths.get(System.getProperty("user.home"), "Desktop", "Flocon", "performances", "${deviceSerial}_$packageName").toFile() | |
| val metricsDir = Paths.get(System.getProperty("user.home"), ".flocon", "cache", "performances", "${deviceSerial}_$packageName").toFile() |
| fun onNext() { | ||
| val metrics = repository.getMetrics() | ||
| val currentIndex = metrics.indexOfFirst { it.timestamp == _event.value.timestamp } | ||
| if (currentIndex > 0) { | ||
| _event.value = metrics[currentIndex - 1] | ||
| updateNavigationState() | ||
| } | ||
| } | ||
|
|
||
| fun onPrevious() { | ||
| val metrics = repository.getMetrics() | ||
| val currentIndex = metrics.indexOfFirst { it.timestamp == _event.value.timestamp } | ||
| if (currentIndex != -1 && currentIndex < metrics.size - 1) { | ||
| _event.value = metrics[currentIndex + 1] | ||
| updateNavigationState() | ||
| } | ||
| } |
There was a problem hiding this comment.
The onNext and onPrevious functions are inefficient as they fetch the full list of metrics from the repository and find the current index on every invocation. Since the list of metrics can grow, this should be optimized.
Additionally, the naming is confusing. onNext moves to a newer event (lower index), while onPrevious moves to an older event (higher index). The UI uses forward/back arrows which might be interpreted differently by users.
Consider caching the list of metrics and the current index within the ViewModel. You could also rename the functions to be more explicit, like onNewerEvent() and onOlderEvent() to avoid ambiguity.
2 participants
No description provided.