Migrate Offline Areas to Jetpack Compose #3490
Conversation
This PR migrates OfflineAreaSelectorFragment and OfflineAreaViewerFragment to Jetpack Compose, removing legacy XML layouts and fixing related tests.
Summary of ChangesHello @gino-m, 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 finalizes the migration of the Offline Areas feature's user interface to Jetpack Compose. It involves a complete refactoring of the UI layer for both the offline area selection and viewing screens, transitioning from a view-based system to a modern declarative Compose approach. This change streamlines the UI codebase, enhances maintainability, and aligns the application with current Android development best practices. 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
|
![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 successfully migrates OfflineAreaSelectorFragment and OfflineAreaViewerFragment to Jetpack Compose, which is a great step forward in modernizing the codebase. The removal of XML layouts and related view-binding code simplifies the architecture. However, I've identified a critical issue in the gradle.properties file that will affect other developers, and a critical bug in the download cancellation logic that needs to be addressed. I've also provided some suggestions to improve the structure and maintainability of the new Composable screens.
| fun OfflineAreaSelectorScreen( | ||
| downloadEnabled: Boolean, | ||
| onDownloadClick: () -> Unit, | ||
| onCancelClick: () -> Unit, | ||
| onLocationLockClick: () -> Unit, | ||
| locationLockIcon: MapFloatingActionButtonType, | ||
| bottomText: String, | ||
| showProgressDialog: Boolean, | ||
| downloadProgress: Float, | ||
| mapView: @Composable () -> Unit, | ||
| ) { |
There was a problem hiding this comment.
The onCancelClick parameter is used for both the main cancel button and the download progress dialog's cancel button. This is incorrect, as the dialog's button should stop the download, while the main button navigates up. This is likely why the related test stopDownloading cancels active download and updates UI state is currently ignored.
A separate onStopDownloadClick parameter should be introduced for the dialog's cancel action. This new parameter should then be invoked in the onClick of the AlertDialog's button.
fun OfflineAreaSelectorScreen(
downloadEnabled: Boolean,
onDownloadClick: () -> Unit,
onCancelClick: () -> Unit,
onStopDownloadClick: () -> Unit,
onLocationLockClick: () -> Unit,
locationLockIcon: MapFloatingActionButtonType,
bottomText: String,
showProgressDialog: Boolean,
downloadProgress: Float,
mapView: @Composable () -> Unit,
)
gradle.properties
Outdated
| # This option should only be used with decoupled projects. More details, visit | ||
| # http://www.gradle.org/docs/current/userguide/multi_project_builds.html#sec:decoupled_projects | ||
| # org.gradle.parallel=true | ||
| org.gradle.java.home=/Applications/Android Studio.app/Contents/jbr/Contents/Home |
There was a problem hiding this comment.
The org.gradle.java.home property should not be committed to the repository's gradle.properties file. This is a local, machine-specific setting, and including it here will likely cause build failures for other developers on different machines or operating systems. This setting should be placed in the user-level gradle.properties file (~/.gradle/gradle.properties) instead.
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| @Suppress("LongMethod") |
There was a problem hiding this comment.
The function OfflineAreaSelectorScreen is quite long, as indicated by @Suppress("LongMethod"). To improve readability and maintainability, consider breaking it down into smaller, more focused composable functions. For example, the viewport overlay, the bottom buttons, and the progress dialog could each be their own composable.
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| @Suppress("LongMethod") |
There was a problem hiding this comment.
Similar to OfflineAreaSelectorScreen, the function OfflineAreaViewerScreen is long, as indicated by the @Suppress("LongMethod"). It would be beneficial to refactor it into smaller composables for better readability and maintainability. The details section, remove button, and progress overlay are good candidates for extraction into their own composable functions.
| Modifier.fillMaxSize().background(Color.Black.copy(alpha = 0.5f)).run { | ||
| // Make it clickable to block interactions? | ||
| // .clickable(enabled = false) {} logic in Compose? | ||
| // Box consumes touches if clickable without indication? | ||
| // Actually, just cover everything. | ||
| this | ||
| }, |
There was a problem hiding this comment.
The .run { ... } block and the comments within it are unnecessary. A Box with a background modifier is sufficient to intercept touch events and block interaction with the UI behind it. Removing this will make the code cleaner and more concise.
Modifier.fillMaxSize().background(Color.Black.copy(alpha = 0.5f))
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3490 +/- ##
============================================
+ Coverage 69.82% 70.14% +0.32%
- Complexity 1599 1607 +8
============================================
Files 322 324 +2
Lines 8678 8860 +182
Branches 949 954 +5
============================================
+ Hits 6059 6215 +156
- Misses 2049 2073 +24
- Partials 570 572 +2
🚀 New features to boost your workflow:
|
gino-m
force-pushed
the
migrate-offline-areas
branch
from
32fcbbf to
624a2b1
Compare
1 participant
Migrates OfflineAreaSelectorFragment and OfflineAreaViewerFragment to Compose. Part 3 of 3.
Towards #1795