Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to navigate to new trace from resultscreen #597

Merged
merged 12 commits into from
Oct 7, 2024
Merged

Add option to navigate to new trace from resultscreen #597

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c54d61a
WIP
puneet-pdx Sep 30, 2024
620fec3
reset current trace
puneet-pdx Sep 30, 2024
effe5f9
WIP
puneet-pdx Sep 30, 2024
02d9e3a
update padding values
puneet-pdx Sep 30, 2024
14b8f76
remove newline
puneet-pdx Oct 1, 2024
85a5e87
Merge branch 'feature-branches/utilitynetworktrace' into puneet/4492_…
puneet-pdx Oct 2, 2024
8ef418b
address code review comments
puneet-pdx Oct 3, 2024
048642b
refactor tabrow
puneet-pdx Oct 3, 2024
303fbed
add newline
puneet-pdx Oct 3, 2024
a0d4df8
address code review comments
puneet-pdx Oct 4, 2024
069f389
remove explicit name
puneet-pdx Oct 4, 2024
094570c
remove unused import
puneet-pdx Oct 7, 2024
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
18 changes: 14 additions & 4 deletions toolkit/utilitynetworks/src/main/java/com/arcgismaps/toolkit/utilitynetworks/TraceState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import androidx.compose.runtime.Immutable
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.Stable
import androidx.compose.runtime.State
import androidx.compose.runtime.mutableIntStateOf
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused import

import androidx.compose.runtime.mutableStateListOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.snapshots.SnapshotStateList
Expand Down Expand Up @@ -119,7 +120,8 @@ public class TraceState(
private val _currentScreen: MutableState<TraceNavRoute> = mutableStateOf(TraceNavRoute.TraceOptions)
internal var currentScreen: State<TraceNavRoute> = _currentScreen

private val completedTraces: MutableList<TraceRun> = mutableListOf()
private val _completedTraces: SnapshotStateList<TraceRun> = mutableStateListOf()
internal val completedTraces: List<TraceRun> = _completedTraces

private var _currentTraceName: MutableState<String> = mutableStateOf("")
/**
Expand Down Expand Up @@ -194,7 +196,7 @@ public class TraceState(

internal fun setSelectedTraceConfiguration(config: UtilityNamedTraceConfiguration) {
_selectedTraceConfiguration.value = config
_currentTraceName.value = "${config.name} ${(completedTraces.count { it.configuration.name == config.name } + 1)}"
_currentTraceName.value = "${config.name} ${(_completedTraces.count { it.configuration.name == config.name } + 1)}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I set the name in advanced options before running the trace, this is overwriting it.

I would set the currentTraceName to the default value at initialization and in resetCurrentTrace, then just not do this here at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as per our discussion this is working as expected.

}

internal fun showScreen(screen: TraceNavRoute) {
Expand Down Expand Up @@ -277,7 +279,7 @@ public class TraceState(
featureResults = currentTraceElementResults,
functionResults = currentTraceFunctionResults,
geometryTraceResult = currentTraceGeometryResults
).also { completedTraces.add(it) }
).also { _completedTraces.add(it) }

if (_currentTraceZoomToResults.value) {
currentTraceResultGeometriesExtent?.let {
Expand All @@ -288,10 +290,18 @@ public class TraceState(
)
}
}

resetCurrentTrace()
return true
}

private fun resetCurrentTrace() {
_selectedTraceConfiguration.value = null
_currentTraceStartingPoints.clear()
Copy link
Collaborator

Choose a reason for hiding this comment

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

the design spec has a user story that says As a dispatcher comparing incident reports, I want to be able to perform analysis on a new trace type without losing the specified starting points.

I'd interpret this as meaning the starting points should remain between traces.

_currentTraceName.value = ""
currentTraceGraphicsColor = Color.green
_currentTraceZoomToResults.value = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

the design spec also says By default, the control will zoom to the trace result area. This feature can be disabled by setting the AutoZoomToTraceResults property of the control to false. which I interpret as meaning this value should be true by default.

}

private fun createGraphicForSimpleLineSymbol(geometry: Geometry, style: SimpleLineSymbolStyle, color: Color) =
Graphic(
geometry = geometry,
Expand Down
3 changes: 3 additions & 0 deletions ...tilitynetworks/src/main/java/com/arcgismaps/toolkit/utilitynetworks/ui/TraceNavigation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ internal fun TraceNavHost(traceState: TraceState) {
defaultTraceName = traceState.currentTraceName.value,
selectedColor = traceState.currentTraceGraphicsColorAsComposeColor,
zoomToResult = traceState.currentTraceZoomToResults.value,
showResultsTab = traceState.completedTraces.isNotEmpty(),
onPerformTraceButtonClicked = {
coroutineScope.launch {
try {
Expand All @@ -66,6 +67,7 @@ internal fun TraceNavHost(traceState: TraceState) {
},
selectedConfig = traceState.selectedTraceConfiguration.value,
onStartingPointRemoved = { traceState.removeStartingPoint(it) },
onBackToResults = { traceState.showScreen(TraceNavRoute.TraceResults) },
onConfigSelected = { newConfig ->
traceState.setSelectedTraceConfiguration(newConfig)
},
Expand Down Expand Up @@ -93,6 +95,7 @@ internal fun TraceNavHost(traceState: TraceState) {
require (traceRun != null)
TraceResultScreen(
traceRun = traceRun,
onBackToNewTrace = { traceState.showScreen(TraceNavRoute.TraceOptions) },
onDeleteResult = {

}, onZoomToResults = {
Expand Down
42 changes: 40 additions & 2 deletions ...itynetworks/src/main/java/com/arcgismaps/toolkit/utilitynetworks/ui/TraceOptionsScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,17 @@ import androidx.compose.material3.ElevatedButton
import androidx.compose.material3.FilterChipDefaults
import androidx.compose.material3.Icon
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.TabRow
import androidx.compose.material3.OutlinedTextField
import androidx.compose.material3.Surface
import androidx.compose.material3.Switch
import androidx.compose.material3.Tab
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableIntStateOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.saveable.Saver
Expand Down Expand Up @@ -95,7 +98,9 @@ internal fun TraceOptionsScreen(
defaultTraceName: String,
selectedColor: Color,
zoomToResult: Boolean,
showResultsTab: Boolean,
onStartingPointRemoved: (StartingPoint) -> Unit,
onBackToResults: () -> Unit,
onConfigSelected: (UtilityNamedTraceConfiguration) -> Unit,
onPerformTraceButtonClicked: () -> Unit,
onAddStartingPointButtonClicked: () -> Unit,
Expand All @@ -108,10 +113,20 @@ internal fun TraceOptionsScreen(
modifier = Modifier
.fillMaxSize()
) {
Column (horizontalAlignment = Alignment.CenterHorizontally) {
Column (modifier = Modifier.padding(horizontal = 10.dp),
horizontalAlignment = Alignment.CenterHorizontally) {

if (showResultsTab) {
TabRow(onBackToResults)
Spacer(
modifier = Modifier
.fillMaxWidth()
.height(15.dp)
)
}
LazyColumn(
modifier = Modifier
.padding(10.dp, 3.dp)
.padding(vertical = 3.dp)
.weight(1f),
horizontalAlignment = Alignment.CenterHorizontally
) {
Expand Down Expand Up @@ -161,6 +176,29 @@ internal fun TraceOptionsScreen(
}
}

@Composable
private fun TabRow(onBackToResults: () -> Unit) {
val tabItems = listOf("New Trace", "Results")
Copy link
Collaborator

Choose a reason for hiding this comment

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

these should be localized

var selectedTabIndex by remember { mutableIntStateOf(0) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var selectedTabIndex by remember { mutableIntStateOf(0) }
var selectedTabIndex by rememberSaveable { mutableIntStateOf(0) }


TabRow(selectedTabIndex = selectedTabIndex) {
tabItems.forEachIndexed { index, title ->
var selected by remember { mutableStateOf(index == 0) }
Tab(
selected = selected,
onClick = {
selectedTabIndex = index
selected = true
if (index == 1) {
onBackToResults()
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var selected by remember { mutableStateOf(index == 0) }
Tab(
selected = selected,
onClick = {
selectedTabIndex = index
selected = true
if (index == 1) {
onBackToResults()
}
},
Tab(
selected = index == selectedTabIndex,
onClick = {
selectedTabIndex = index
if (index == 1) {
onBackToResults()
}
},

text = { Text(title) }
)
}
}
}

@Composable
private fun TraceConfigurations(
configs: List<UtilityNamedTraceConfiguration>,
Expand Down
69 changes: 57 additions & 12 deletions ...litynetworks/src/main/java/com/arcgismaps/toolkit/utilitynetworks/ui/TraceResultScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.PaddingValues
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.shape.RoundedCornerShape
Expand All @@ -39,9 +41,12 @@ import androidx.compose.material3.HorizontalDivider
import androidx.compose.material3.Icon
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.Surface
import androidx.compose.material3.Tab
import androidx.compose.material3.TabRow
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableIntStateOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
Expand All @@ -64,28 +69,68 @@ import com.arcgismaps.utilitynetworks.UtilityTraceFunctionOutput
@Composable
internal fun TraceResultScreen(
traceRun: TraceRun,
onBackToNewTrace: () -> Unit,
onDeleteResult: () -> Unit,
onZoomToResults: () -> Unit,
onClearAllResults: () -> Unit
) {
Surface(modifier = Modifier.fillMaxSize()) {
LazyColumn {
item {
TraceTitle(traceRun.name, onZoomToResults = onZoomToResults, onDeleteResult = onDeleteResult)
}
item {
FeatureResult(traceRun.featureResults)
}
item {
FunctionResult(traceRun.functionResults)
}
item {
ClearAllResultsButton(onClearAllResults)
Column(modifier = Modifier
.fillMaxSize()
.padding(horizontal = 10.dp)) {

TabRow(onBackToNewTrace)
Spacer(
modifier = Modifier
.fillMaxWidth()
.height(15.dp)
)
Comment on lines +81 to +85
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest moving this to TabRow as you do the same in both Result screen and Options screen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reading this in the code here makes more sense, as we are adding the tabrow and the tabrow should not be adding additional space after it implicitly.

LazyColumn {
item {
TraceTitle(
traceRun.name,
onZoomToResults = onZoomToResults,
onDeleteResult = onDeleteResult
)
}
item {
FeatureResult(traceRun.featureResults)
}
item {
FunctionResult(traceRun.functionResults)
}
item {
ClearAllResultsButton(onClearAllResults)
}
}
}
}
}

@Composable
Copy link
Collaborator

Choose a reason for hiding this comment

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

same advice as above. Also, it seems like the internalTabRow could be reused in both results and trace options by passing in the lambda, tabItems, and selected index.

private fun TabRow(onBackToNewTrace: () -> Unit) {
val tabItems = listOf("New Trace", "Results")
var selectedTabIndex by remember { mutableIntStateOf(1) }

TabRow(selectedTabIndex = selectedTabIndex) {
tabItems.forEachIndexed { index, title ->
var selected by remember { mutableStateOf(index == 1) }
Tab(
selected = selected,
onClick = {
selectedTabIndex = index
selected = true
if (index == 0) {
onBackToNewTrace()
}
},
text = { Text(title) }
)
}
}
}


@Composable
private fun TraceTitle(traceName: String, onZoomToResults: () -> Unit, onDeleteResult: () -> Unit) {
var expanded by remember { mutableStateOf(false) }
Expand Down