Conversation
This PR migrates the Home Screen to Jetpack Compose, including HomeDrawer and MapContainer integration. It also fixes related tests and includes necessary resource cleanups.
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 significantly refactors the application's main home screen and its navigation drawer by migrating them to Jetpack Compose. This change streamlines the UI development process by replacing imperative XML-based views with a declarative Compose approach. It also includes necessary updates to the testing framework to accommodate Compose UI tests and refines development guidelines for code quality and PR management. 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 is a great step towards migrating the HomeScreen and its components to Jetpack Compose. The use of ComposeView and AndroidView to incrementally adopt Compose is well-executed. The new composables for HomeScreen and HomeDrawer are well-structured. I've identified a few areas for improvement, mainly around code structure, maintainability, and testing. Specifically, I've suggested refactoring some large composables and programmatic view creation to be more idiomatic with Compose. I also noticed some tests for existing functionality were removed and should be restored. Finally, there's a configuration change in gradle.properties that seems specific to a local environment and should probably be reverted.
.../main/java/org/groundplatform/android/ui/home/mapcontainer/HomeScreenMapContainerFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/groundplatform/android/ui/home/HomeDrawer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/groundplatform/android/ui/home/HomeDrawer.kt
Outdated
Show resolved
Hide resolved
| override fun onCreateView( | ||
| inflater: LayoutInflater, | ||
| container: ViewGroup?, | ||
| savedInstanceState: Bundle?, | ||
| ): View { | ||
| super.onCreateView(inflater, container, savedInstanceState) | ||
| binding = HomeScreenFragBinding.inflate(inflater, container, false) | ||
| binding.lifecycleOwner = this | ||
| lifecycleScope.launch { homeScreenViewModel.openDrawerRequestsFlow.collect { openDrawer() } } | ||
| return binding.root | ||
| } | ||
|
|
||
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
| super.onViewCreated(view, savedInstanceState) | ||
| // Ensure nav drawer cannot be swiped out, which would conflict with map pan gestures. | ||
| binding.drawerLayout.setDrawerLockMode(DrawerLayout.LOCK_MODE_LOCKED_CLOSED) | ||
| homeScreenViewModel.showOfflineAreaMenuItem.observe(viewLifecycleOwner) { | ||
| binding.navView.menu.findItem(R.id.nav_offline_areas).isEnabled = it | ||
| } | ||
|
|
||
| binding.navView.setNavigationItemSelectedListener(this) | ||
| val navHeader = binding.navView.getHeaderView(0) | ||
| navHeader.findViewById<TextView>(R.id.switch_survey_button).setOnClickListener { | ||
| findNavController() | ||
| .navigate( | ||
| HomeScreenFragmentDirections.actionHomeScreenFragmentToSurveySelectorFragment(false) | ||
| ) | ||
| } | ||
| viewLifecycleOwner.lifecycleScope.launch { user = userRepository.getAuthenticatedUser() } | ||
| navHeader.findViewById<ShapeableImageView>(R.id.user_image).setOnClickListener { | ||
| showSignOutConfirmationDialogs() | ||
| } | ||
| updateNavHeader() | ||
| // Re-open data collection screen if draft submission is present. | ||
| viewLifecycleOwner.lifecycleScope.launch { | ||
| homeScreenViewModel.getDraftSubmission()?.let { draft -> | ||
| findNavController() | ||
| .navigate( | ||
| HomeScreenFragmentDirections.actionHomeScreenFragmentToDataCollectionFragment( | ||
| draft.loiId, | ||
| draft.loiName ?: "", | ||
| draft.jobId, | ||
| true, | ||
| SubmissionDeltasConverter.toString(draft.deltas), | ||
| draft.currentTaskId ?: "", | ||
| ) | ||
| val composeView = androidx.compose.ui.platform.ComposeView(requireContext()) | ||
| composeView.setViewCompositionStrategy( | ||
| androidx.compose.ui.platform.ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed | ||
| ) | ||
| composeView.setContent { | ||
| org.groundplatform.android.ui.theme.AppTheme { | ||
| val drawerState = | ||
| androidx.compose.material3.rememberDrawerState( | ||
| androidx.compose.material3.DrawerValue.Closed | ||
| ) | ||
|
|
||
| if (!homeScreenViewModel.awaitingPhotoCapture) { | ||
| ephemeralPopups | ||
| .InfoPopup(binding.root, R.string.draft_restored, EphemeralPopups.PopupDuration.SHORT) | ||
| .show() | ||
| } else { | ||
| // We're restoring after an instantaneous photo capture for a photo task; don't show a | ||
| // draft restored toast. | ||
| homeScreenViewModel.awaitingPhotoCapture = false | ||
| val user by | ||
| androidx.compose.runtime.produceState<User?>(initialValue = null) { | ||
| value = userRepository.getAuthenticatedUser() | ||
| } | ||
| val survey by | ||
| homeScreenViewModel.surveyRepository.activeSurveyFlow.collectAsStateWithLifecycle(null) | ||
| val scope = androidx.compose.runtime.rememberCoroutineScope() | ||
|
|
||
| // Handle open drawer requests from ViewModel | ||
| androidx.compose.runtime.LaunchedEffect(Unit) { | ||
| homeScreenViewModel.openDrawerRequestsFlow.collect { drawerState.open() } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| val navigationView = view.findViewById<NavigationView>(R.id.nav_view) | ||
| val menuItem = navigationView.menu.findItem(R.id.nav_log_version) | ||
| menuItem.title = String.format(getString(R.string.build), BuildConfig.VERSION_NAME) | ||
| } | ||
| val showSignOutDialog = remember { mutableStateOf(false) } | ||
|
|
||
| private fun updateNavHeader() = | ||
| lifecycleScope.launch { | ||
| val navHeader = binding.navView.getHeaderView(0) | ||
| val headerBinding = NavDrawerHeaderBinding.bind(navHeader) | ||
| headerBinding.user = userRepository.getAuthenticatedUser() | ||
| homeScreenViewModel.surveyRepository.activeSurveyFlow.collect { | ||
| if (it == null) { | ||
| headerBinding.surveyInfo.visibility = View.GONE | ||
| headerBinding.noSurveysInfo.visibility = View.VISIBLE | ||
| } else { | ||
| headerBinding.noSurveysInfo.visibility = View.GONE | ||
| headerBinding.surveyInfo.visibility = View.VISIBLE | ||
| headerBinding.survey = it | ||
| androidx.activity.compose.BackHandler(enabled = drawerState.isOpen) { | ||
| scope.launch { drawerState.close() } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun openDrawer() { | ||
| binding.drawerLayout.openDrawer(GravityCompat.START) | ||
| } | ||
|
|
||
| private fun closeDrawer() { | ||
| binding.drawerLayout.closeDrawer(GravityCompat.START) | ||
| } | ||
|
|
||
| private fun onApplyWindowInsets(insets: WindowInsetsCompat) { | ||
| val headerView = binding.navView.getHeaderView(0) | ||
| headerView.setPadding(0, insets.systemInsets().top, 0, 0) | ||
| } | ||
|
|
||
| override fun onBack(): Boolean = false | ||
| HomeScreen( | ||
| drawerState = drawerState, | ||
| drawerContent = { | ||
| val offlineAreasEnabled by | ||
| homeScreenViewModel.showOfflineAreaMenuItem.observeAsState(true) | ||
| HomeDrawer( | ||
| user = user, | ||
| survey = survey, | ||
| onSwitchSurvey = { | ||
| findNavController() | ||
| .navigate( | ||
| HomeScreenFragmentDirections.actionHomeScreenFragmentToSurveySelectorFragment( | ||
| false | ||
| ) | ||
| ) | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onNavigateToOfflineAreas = { | ||
| if (offlineAreasEnabled) { | ||
| scope.launch { | ||
| if (homeScreenViewModel.getOfflineAreas().isEmpty()) | ||
| findNavController() | ||
| .navigate(HomeScreenFragmentDirections.showOfflineAreaSelector()) | ||
| else | ||
| findNavController().navigate(HomeScreenFragmentDirections.showOfflineAreas()) | ||
| drawerState.close() | ||
| } | ||
| } | ||
| }, | ||
| onNavigateToSyncStatus = { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showSyncStatus()) | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onNavigateToSettings = { | ||
| findNavController() | ||
| .navigate( | ||
| HomeScreenFragmentDirections.actionHomeScreenFragmentToSettingsActivity() | ||
| ) | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onNavigateToAbout = { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showAbout()) | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onNavigateToTerms = { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showTermsOfService(true)) | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onSignOut = { showSignOutDialog.value = true }, | ||
| versionText = | ||
| String.format( | ||
| getString(R.string.build), | ||
| org.groundplatform.android.BuildConfig.VERSION_NAME, | ||
| ), | ||
| ) | ||
| }, | ||
| content = { | ||
| // Map Container | ||
| androidx.compose.ui.viewinterop.AndroidView( | ||
| factory = { context -> | ||
| android.widget.FrameLayout(context).apply { id = R.id.map_container_fragment } | ||
| }, | ||
| update = { view -> | ||
| val fragment = childFragmentManager.findFragmentById(R.id.map_container_fragment) | ||
| if (fragment == null) { | ||
| val mapFragment = | ||
| org.groundplatform.android.ui.home.mapcontainer.HomeScreenMapContainerFragment() | ||
| childFragmentManager | ||
| .beginTransaction() | ||
| .replace(R.id.map_container_fragment, mapFragment) | ||
| .commit() | ||
| } | ||
| }, | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) | ||
|
|
||
| override fun onNavigationItemSelected(item: MenuItem): Boolean { | ||
| when (item.itemId) { | ||
| R.id.sync_status -> { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showSyncStatus()) | ||
| } | ||
| R.id.nav_offline_areas -> { | ||
| lifecycleScope.launch { | ||
| if (homeScreenViewModel.getOfflineAreas().isEmpty()) | ||
| findNavController().navigate(HomeScreenFragmentDirections.showOfflineAreaSelector()) | ||
| else findNavController().navigate(HomeScreenFragmentDirections.showOfflineAreas()) | ||
| } | ||
| } | ||
| R.id.nav_settings -> { | ||
| findNavController() | ||
| .navigate(HomeScreenFragmentDirections.actionHomeScreenFragmentToSettingsActivity()) | ||
| } | ||
| R.id.about -> { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showAbout()) | ||
| } | ||
| R.id.terms_of_service -> { | ||
| findNavController().navigate(HomeScreenFragmentDirections.showTermsOfService(true)) | ||
| // Overlays | ||
| val view = androidx.compose.ui.platform.LocalView.current | ||
|
|
||
| // Sign Out Confirmation | ||
| if (showSignOutDialog.value) { | ||
| ConfirmationDialog( | ||
| title = R.string.sign_out_dialog_title, | ||
| description = R.string.sign_out_dialog_body, | ||
| confirmButtonText = R.string.sign_out, | ||
| onConfirmClicked = { | ||
| homeScreenViewModel.signOut() | ||
| showSignOutDialog.value = false | ||
| scope.launch { drawerState.close() } | ||
| }, | ||
| onDismiss = { showSignOutDialog.value = false }, | ||
| ) | ||
| } | ||
|
|
||
| // Handle sign out dialog state | ||
|
|
||
| androidx.compose.runtime.LaunchedEffect(Unit) { | ||
| // ... draft restored logic ... | ||
| homeScreenViewModel.getDraftSubmission()?.let { draft -> | ||
| findNavController() | ||
| .navigate( | ||
| HomeScreenFragmentDirections.actionHomeScreenFragmentToDataCollectionFragment( | ||
| draft.loiId, | ||
| draft.loiName ?: "", | ||
| draft.jobId, | ||
| true, | ||
| SubmissionDeltasConverter.toString(draft.deltas), | ||
| draft.currentTaskId ?: "", | ||
| ) | ||
| ) | ||
| if (!homeScreenViewModel.awaitingPhotoCapture) { | ||
| ephemeralPopups | ||
| .InfoPopup(view, R.string.draft_restored, EphemeralPopups.PopupDuration.SHORT) | ||
| .show() | ||
| } else { | ||
| homeScreenViewModel.awaitingPhotoCapture = false | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
| closeDrawer() | ||
| return true | ||
| return composeView |
There was a problem hiding this comment.
The onCreateView method, specifically the setContent block, has become quite large and complex due to holding the entire screen's composition logic. To improve readability and maintainability, I suggest extracting parts of the UI into separate, smaller composable functions. For example, the content lambda for HomeScreen could be its own composable that takes the necessary state and callbacks.
| super.onCreateView(inflater, container, savedInstanceState) | ||
| binding = BasemapLayoutBinding.inflate(inflater, container, false) | ||
| return binding.root | ||
| // Programmatic layout equivalent to basemap_layout.xml | ||
| val root = | ||
| androidx.constraintlayout.widget.ConstraintLayout(requireContext()).apply { | ||
| layoutParams = | ||
| ViewGroup.LayoutParams( | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ) | ||
| } | ||
|
|
||
| // Map Container | ||
| val mapContainer = | ||
| android.widget.FrameLayout(requireContext()).apply { | ||
| id = R.id.map | ||
| layoutParams = | ||
| androidx.constraintlayout.widget.ConstraintLayout.LayoutParams( | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ) | ||
| } | ||
| root.addView(mapContainer) | ||
|
|
||
| // Compose Content | ||
| composeView = | ||
| androidx.compose.ui.platform.ComposeView(requireContext()).apply { | ||
| id = R.id.compose_content | ||
| layoutParams = | ||
| androidx.constraintlayout.widget.ConstraintLayout.LayoutParams( | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ) | ||
| } | ||
| root.addView(composeView) | ||
|
|
||
| // Bottom Container (CoordinatorLayout) | ||
| val coordinatorPos = | ||
| androidx.constraintlayout.widget.ConstraintLayout.LayoutParams( | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ViewGroup.LayoutParams.WRAP_CONTENT, | ||
| ) | ||
| .apply { | ||
| bottomToBottom = androidx.constraintlayout.widget.ConstraintLayout.LayoutParams.PARENT_ID | ||
| startToStart = androidx.constraintlayout.widget.ConstraintLayout.LayoutParams.PARENT_ID | ||
| endToEnd = androidx.constraintlayout.widget.ConstraintLayout.LayoutParams.PARENT_ID | ||
| // gravity = Gravity.BOTTOM (ConstraintLayout handles via constraints) | ||
| } | ||
|
|
||
| bottomContainer = | ||
| androidx.coordinatorlayout.widget.CoordinatorLayout(requireContext()).apply { | ||
| id = R.id.bottom_container | ||
| layoutParams = coordinatorPos | ||
| } | ||
| root.addView(bottomContainer) | ||
|
|
||
| return root |
There was a problem hiding this comment.
The onCreateView method now programmatically creates the view hierarchy using Android Views. While this removes the XML dependency, it's quite verbose and less idiomatic in a Compose-centric architecture. A cleaner approach would be to return a ComposeView directly and define the entire layout in Compose, using AndroidView to host the MapFragment. This would make the layout definition more declarative and easier to manage.
| // Verify drawer closed? | ||
| // composeTestRule.onNodeWithText(menuItemLabel).assertIsNotDisplayed() |
There was a problem hiding this comment.
This test verifies navigation but doesn't assert that the navigation drawer is closed after clicking an item. It's good practice to verify the final state of the UI. Please add an assertion to confirm the drawer is no longer visible after the click, for example by checking that one of its elements is not displayed anymore.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3487 +/- ##
============================================
+ Coverage 69.82% 69.85% +0.03%
+ Complexity 1599 1594 -5
============================================
Files 322 324 +2
Lines 8678 8860 +182
Branches 949 958 +9
============================================
+ Hits 6059 6189 +130
- Misses 2049 2094 +45
- Partials 570 577 +7
🚀 New features to boost your workflow:
|
…o migrate-home-screen
…ormatting Also fixed a bug in push_to_github skill where it referenced a non-existent gradle task.
…o gino-m/patch/gemini-skills
…o migrate-home-screen # Conflicts: # .agent/skills/fetch_pr_comments/scripts/fetch_comments.py # .agent/skills/push_to_github/scripts/push.py
* Refactor `HomeDrawer` to use `DrawerItem` and `IconSource` sealed interface instead of passing Composable lambdas. * Handle null `loiId` in `HomeScreenFragment` when restoring draft submissions.
…d-android into migrate-home-screen
…d-android into migrate-home-screen
…o migrate-home-screen
* Refactor `DataSharingTermsDialog` to be stateless, hoisting state management to the caller. * Move data sharing consent logic and dialog state from `HomeScreenMapContainerFragment` to `HomeScreenMapContainerViewModel`. * Add `queueDataCollection` to ViewModel to check terms and manage pending navigation. * Expose `dataSharingTerms` state and `navigateToDataCollectionFragment` flow for UI observation. * Update `HomeScreenMapContainerScreen` to render the dialog based on the provided state. * Update tests to verify the new state management and navigation flow.
* Update `HomeScreenMapContainerScreen` to conditionally display the `BasemapSelectorScreen` overlay. * Pass supported map types and visibility state from `HomeScreenMapContainerFragment` to the screen composable. * Handle map type selector dismissal events.
Pass `skipPartiallyExpanded = true` to `rememberModalBottomSheetState` in `BasemapSelectorScreen` to prevent the sheet from opening in a half-expanded state.
- Handle InvalidCustomSharingTermsException in queueDataCollection - Show error popup for invalid terms in HomeScreenMapContainerFragment - Fix HomeScreenMapContainerViewModelTest initialization by mocking dependencies and stubbing flows - Address UncompletedCoroutinesError in tests with proper cleanup
…tion parameters to HomeScreenMapContainerScreen.
1 participant
Migrates HomeScreenFragment and HomeDrawer to Compose. Part 1 of 3.
Towards #1795