Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,11 @@ import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.livedata.observeAsState
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.semantics.testTag
import androidx.navigation.fragment.findNavController
import dagger.hilt.android.AndroidEntryPoint
import org.groundplatform.android.databinding.SyncStatusFragBinding
import org.groundplatform.android.ui.common.AbstractFragment
import org.groundplatform.android.util.setComposableContent

/**
* This fragment summarizes the synchronization statuses of local changes that are being uploaded to
Expand All @@ -54,21 +45,14 @@ class SyncStatusFragment : AbstractFragment() {
savedInstanceState: Bundle?,
): View {
super.onCreateView(inflater, container, savedInstanceState)
val binding = SyncStatusFragBinding.inflate(inflater, container, false)
binding.viewModel = viewModel
binding.lifecycleOwner = this
binding.composeView.setComposableContent { ShowSyncItems() }
getAbstractActivity().setSupportActionBar(binding.syncStatusToolbar)
return binding.root
}

@Composable
private fun ShowSyncItems() {
val list by viewModel.uploadStatus.observeAsState()
list?.let {
LazyColumn(Modifier.fillMaxSize().testTag("sync list")) {
items(it) {
SyncListItem(modifier = Modifier.semantics { testTag = "item ${it.user}" }, detail = it)
return androidx.compose.ui.platform.ComposeView(requireContext()).apply {
setViewCompositionStrategy(
androidx.compose.ui.platform.ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed
)
setContent {
org.groundplatform.android.ui.theme.AppTheme {
val list by viewModel.uploadStatus.observeAsState(emptyList())
SyncStatusScreen(uploadStatuses = list, onBack = { findNavController().navigateUp() })
}
}
}
Comment on lines 48 to 58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve code readability, it's better to import the classes used here instead of using their fully qualified names. Please add imports for androidx.compose.ui.platform.ComposeView, androidx.compose.ui.platform.ViewCompositionStrategy, and org.groundplatform.android.ui.theme.AppTheme.

Suggested change
return androidx.compose.ui.platform.ComposeView(requireContext()).apply {
setViewCompositionStrategy(
androidx.compose.ui.platform.ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed
)
setContent {
org.groundplatform.android.ui.theme.AppTheme {
val list by viewModel.uploadStatus.observeAsState(emptyList())
SyncStatusScreen(uploadStatuses = list, onBack = { findNavController().navigateUp() })
}
}
}
return ComposeView(requireContext()).apply {
setViewCompositionStrategy(
ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed
)
setContent {
AppTheme {
val list by viewModel.uploadStatus.observeAsState(emptyList())
SyncStatusScreen(uploadStatuses = list, onBack = { findNavController().navigateUp() })
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright 2026 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.groundplatform.android.ui.syncstatus

import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.automirrored.filled.ArrowBack
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.Scaffold
import androidx.compose.material3.Text
import androidx.compose.material3.TopAppBar
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.semantics.testTag
import org.groundplatform.android.R

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun SyncStatusScreen(uploadStatuses: List<SyncStatusDetail>, onBack: () -> Unit) {
Scaffold(
topBar = {
TopAppBar(
title = { Text(text = stringResource(R.string.data_sync_status)) },
navigationIcon = {
IconButton(onClick = onBack) {
Icon(
imageVector = Icons.AutoMirrored.Filled.ArrowBack,
contentDescription = stringResource(R.string.previous),
)
}
},
)
}
) { paddingValues ->
LazyColumn(modifier = Modifier.fillMaxSize().padding(paddingValues).testTag("sync list")) {
items(uploadStatuses) {
SyncListItem(modifier = Modifier.semantics { testTag = "item ${it.user}" }, detail = it)
}
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Here are a couple of suggestions to improve this LazyColumn implementation:

  1. Provide a unique key: For better performance and to prevent potential issues with state when the list is modified, it's a best practice to provide a unique key for each item. Since SyncStatusDetail doesn't have a unique ID field, you can compose a key from its existing properties.

  2. Ensure unique testTag: The testTag is generated using only the user's name, which is not guaranteed to be unique if a user has multiple items. This could lead to flaky UI tests. A more robust testTag can be created from a combination of fields like label and subtitle.

      items(
        items = uploadStatuses,
        key = { "${it.timestamp.time}-${it.label}-${it.subtitle}" }
      ) {
        SyncListItem(
          modifier = Modifier.semantics { testTag = "item ${it.label} ${it.subtitle}" },
          detail = it
        )
      }

}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down
53 changes: 0 additions & 53 deletions app/src/main/res/layout/sync_status_frag.xml

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ package org.groundplatform.android.ui.syncstatus
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.onNodeWithText
import androidx.test.espresso.Espresso.onView
import androidx.test.espresso.assertion.ViewAssertions.matches
import androidx.test.espresso.matcher.ViewMatchers.isDisplayed
import androidx.test.espresso.matcher.ViewMatchers.withId
import dagger.hilt.android.testing.HiltAndroidTest
import javax.inject.Inject
import kotlinx.coroutines.ExperimentalCoroutinesApi
Expand Down Expand Up @@ -61,7 +57,12 @@ class SyncStatusFragmentTest : BaseHiltTest() {
fun `Toolbar should be displayed`() {
setupFragment()

onView(withId(R.id.sync_status_toolbar)).check(matches(isDisplayed()))
composeTestRule
.onNodeWithText(
androidx.test.core.app.ApplicationProvider.getApplicationContext<android.content.Context>()
.getString(R.string.data_sync_status)
)
.assertIsDisplayed()
}

@Test
Expand Down
1 change: 1 addition & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This hardcoded, absolute path for org.gradle.java.home is specific to a single developer's machine and should not be committed to version control. It will cause build failures for other team members on different operating systems or with different Android Studio installation paths. This setting should be configured locally by each developer, for example in the local.properties file (which is typically ignored by Git) or as an environment variable.

#Thu Dec 26 12:25:19 EST 2019
android.enableJetifier=true
android.useAndroidX=true
Expand Down
Loading