Skip to content

Commit

Permalink
Improve performance of Filter Types by limiting recomposition & bug f…
Browse files Browse the repository at this point in the history 
…ix (#41)

* This commit improves the performance of the FILTERS SportTypes section by limiting recomposition.
* Various bugs are fixed relating to issues with threading by making all filter-mutating & accessing operations on the activity thread.
  • Loading branch information
@Tyler-Lopez
Tyler-Lopez authored Mar 3, 2023
1 parent 37d0d51 commit 8118bbb
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 50 deletions.
2 changes: 1 addition & 1 deletion app/src/main/java/com/activityartapp/presentation/editArtScreen/EditArtModels.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ sealed interface EditArtViewState : ViewState {
val filterDistanceTotalEnd: State<Double?>,
val filterDistancePendingChangeStart: State<String?>,
val filterDistancePendingChangeEnd: State<String?>,
val filterTypes: SnapshotStateMap<SportType, Boolean>,
val filterTypes: SnapshotStateList<Pair<SportType, Boolean>>,
override val pagerStateWrapper: PagerStateWrapper,
val listStateFilter: LazyListState = LazyListState(),
val listStateStyle: LazyListState = LazyListState(),
Expand Down
104 changes: 80 additions & 24 deletions app/src/main/java/com/activityartapp/presentation/editArtScreen/EditArtViewModel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package com.activityartapp.presentation.editArtScreen

import android.graphics.Bitmap
import android.util.Size
import androidx.compose.runtime.*
import androidx.compose.runtime.snapshots.SnapshotStateMap
import androidx.compose.runtime.mutableStateListOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.snapshotFlow
import androidx.compose.runtime.toMutableStateList
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.viewModelScope
import com.activityartapp.architecture.BaseRoutingViewModel
Expand All @@ -27,6 +29,8 @@ import com.google.accompanist.pager.ExperimentalPagerApi
import com.google.accompanist.pager.PagerState
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.*
import okhttp3.internal.toImmutableList
import java.util.stream.Collectors.toList
import javax.inject.Inject
import kotlin.math.roundToInt

Expand Down Expand Up @@ -170,11 +174,22 @@ class EditArtViewModel @Inject constructor(
TYPE -> {
val newFilterTypes = activityFilterUtils.getPossibleActivityTypes(
activities = filteredActivities,
filterTypesPrevious = _filterTypes
)
_filterTypes.clear()
filterTypesPrevious = _filterTypes.toMap() // todo
)?.toMutableMap()

if (newFilterTypes != null) {
_filterTypes.putAll(newFilterTypes)
val iterator = _filterTypes.iterator()
while (iterator.hasNext()) {
val filterType: Pair<SportType, Boolean> = iterator.next()
if (newFilterTypes.contains(filterType.first)) {
newFilterTypes.remove(filterType.first)
} else {
iterator.remove()
}
}
_filterTypes.addAll(newFilterTypes.toList())
} else {
_filterTypes.clear()
}
}
DISTANCE -> {
Expand Down Expand Up @@ -237,7 +252,7 @@ class EditArtViewModel @Inject constructor(
private val _filterDistanceTotalEnd = mutableStateOf<Double?>(null)
private val _filterDistancePendingChangeStart = mutableStateOf<String?>(null) // not save
private val _filterDistancePendingChangeEnd = mutableStateOf<String?>(null) // not save
private val _filterTypes: SnapshotStateMap<SportType, Boolean> = mutableStateMapOf()
private val _filterTypes = mutableStateListOf<Pair<SportType, Boolean>>()

// SIZE
private val _sizeResolutionList =
Expand Down Expand Up @@ -406,19 +421,19 @@ class EditArtViewModel @Inject constructor(
}

private fun onFilterChangeEvent(event: FilterChanged) {
/** Updates UI in response to adjustment without adjusted filtered activities **/
when (event) {
is FilterChanged.FilterDateSelectionChanged -> onFilterDateSelectionChanged(event)
is FilterChanged.FilterDateCustomChanged -> onFilterDateCustomChanged(event)
is FilterChanged.FilterDistanceChanged -> onFilterDistanceChanged(event)
is FilterChanged.FilterDistancePendingChangeConfirmed -> onFilterDistancePendingChangeConfirmed(
event
)
is FilterChanged.FilterTypeToggled -> onFilterTypeToggled(event)
}

/** Run all operations which require many linear scans of activities off of the main thread **/
viewModelScope.launch(activitiesProcessingDispatcher) {
/** Updates UI in response to adjustment without adjusted filtered activities **/
when (event) {
is FilterChanged.FilterDateSelectionChanged -> onFilterDateSelectionChanged(event)
is FilterChanged.FilterDateCustomChanged -> onFilterDateCustomChanged(event)
is FilterChanged.FilterDistanceChanged -> onFilterDistanceChanged(event)
is FilterChanged.FilterDistancePendingChangeConfirmed -> onFilterDistancePendingChangeConfirmed(
event
)
is FilterChanged.FilterTypeToggled -> onFilterTypeToggled(event)
}

event.filterType.apply {
/** In response to [EditArtViewState.Standby] changes relevant to this
* [EditArtFilterType] which just occurred, update its filtered activities **/
Expand Down Expand Up @@ -540,7 +555,17 @@ class EditArtViewModel @Inject constructor(
}

private fun onFilterTypeToggled(event: FilterChanged.FilterTypeToggled) {
_filterTypes[event.type] = _filterTypes[event.type]!!.not()
/** If an athlete clicks on a previous filter to TYPE which will result in [_filterTypes]
* becoming smaller, then rapidly clicks on the last index before the list resizes,
* there would be an [IndexOutOfBoundsException] if this logic was not used. **/
_filterTypes
.indexOfFirst { it.first == event.type }
.takeIf { it != -1 }
?.let {
_filterTypes[it] = _filterTypes[it].run {
copy(second = second.not())
}
}
}

private fun onNavigateUpClicked() {
Expand All @@ -556,9 +581,19 @@ class EditArtViewModel @Inject constructor(
}

private fun onSaveClicked() {
viewModelScope.launch {
viewModelScope.launch(activitiesProcessingDispatcher) {
/** Prevent rapid-click after changing a filter from routing to SaveArt when
* no activities are selected.
*
* Clicking SaveArt rapidly and then changing a filter will not cause issue on
* SaveArt, but on back-press that filter will have taken effect. */
if (activitiesFiltered.isEmpty()) {
return@launch
}

val targetSize = _sizeResolutionList[_sizeResolutionListSelectedIndex.value]
val filterRange = activitiesDateRangeUnixMs

routeTo(
MainDestination.NavigateSaveArt(
activityTypes = filteredTypes,
Expand Down Expand Up @@ -918,7 +953,28 @@ class EditArtViewModel @Inject constructor(
private fun Double.milesToMeters(): Double = this / 0.000621371192

private val filteredTypes: List<SportType>
get() = _filterTypes.entries.filter { it.value }.map { it.key }
get() = _filterTypes.filter { it.second }.map { it.first }
/*
.takemutableListOf<SportType>().apply {
/** [_filterTypes] must NOT be iterated over using an [Iterator] or a
* [ConcurrentModificationException] will occur when rapidly toggling a filter type.
*
* Though it might seem this may cause an issue where the result of [filteredTypes]
* does not reflect the UI, it does not seem to by manual test.
*
* If it does cause issue, an alternative solution is to move access of this
* variable into the [activitiesProcessingDispatcher] thread - or just remove that thread. **/
for (i in 0.._filterTypes.lastIndex) {
_filterTypes
.getOrNull(i)
?.takeIf { it.second }
?.let { add(it.first) }
}
*/


private val filteredDistanceRangeMeters: IntRange
get() = (_filterDistanceSelectedStart.value
?.roundToInt()
Expand Down Expand Up @@ -958,9 +1014,9 @@ class EditArtViewModel @Inject constructor(
ssh.get<Double?>(SSH_FILTER_DISTANCE_TOTAL_END)?.let {
_filterDistanceTotalEnd.value = it
}
ssh.get<Map<SportType, Boolean>>(SSH_FILTER_TYPES)?.let {
ssh.get<List<Pair<SportType, Boolean>>>(SSH_FILTER_TYPES)?.let {
_filterTypes.clear()
_filterTypes.putAll(it)
_filterTypes.addAll(it)
}
ssh.get<List<Resolution>>(SSH_SIZE_RESOLUTION_LIST)?.let {
_sizeResolutionList.clear()
Expand Down Expand Up @@ -1047,7 +1103,7 @@ class EditArtViewModel @Inject constructor(
}
}
viewModelScope.launch {
snapshotFlow { _filterTypes.toMap() }.collect {
snapshotFlow { _filterTypes.toList() }.collect {
ssh[SSH_FILTER_TYPES] = it
}
}
Expand Down
6 changes: 3 additions & 3 deletions .../com/activityartapp/presentation/editArtScreen/subscreens/filters/EditArtFiltersScreen.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ fun EditArtFiltersScreen(
distancePendingChangeStart: State<String?>,
distancePendingChangeEnd: State<String?>,
listState: LazyListState,
typesWithSelectedFlag: SnapshotStateMap<SportType, Boolean>,
typesWithSelectedFlag: SnapshotStateList<Pair<SportType, Boolean>>,
eventReceiver: EventReceiver<EditArtViewEvent>
) {
val sections = mutableListOf<EditArtFiltersSectionType>().apply {
Expand All @@ -44,7 +44,7 @@ fun EditArtFiltersScreen(
if (typesWithSelectedFlag.isNotEmpty()) {
add(EditArtFiltersSectionType.ACTIVITY_TYPE)
}
if (distanceTotalStart.value != null) { // todo add back start != end case reminder
if (distanceTotalStart.value != null && distanceTotalStart.value != distanceTotalEnd.value) { // todo add back start != end case reminder
add(EditArtFiltersSectionType.DISTANCE)
}
}
Expand All @@ -66,7 +66,7 @@ fun EditArtFiltersScreen(
)
EditArtFiltersSectionType.ACTIVITY_TYPE -> SectionActivityType(
count = activitiesCountType.value,
typesWithSelectedFlag = typesWithSelectedFlag.toList(),
typesWithSelectedFlag = typesWithSelectedFlag,
eventReceiver = eventReceiver
)
EditArtFiltersSectionType.DISTANCE -> distanceTotalEnd.value?.let {
Expand Down
66 changes: 45 additions & 21 deletions ...ivityartapp/presentation/editArtScreen/subscreens/filters/sections/SectionActivityType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,63 @@ import androidx.compose.material.Checkbox
import androidx.compose.material.MaterialTheme
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.snapshots.SnapshotStateList
import androidx.compose.ui.Alignment
import androidx.compose.ui.res.stringResource
import com.activityartapp.architecture.EventReceiver
import com.activityartapp.presentation.editArtScreen.EditArtViewEvent
import com.activityartapp.presentation.editArtScreen.EditArtViewEvent.ArtMutatingEvent.FilterChanged
import com.activityartapp.presentation.editArtScreen.subscreens.filters.composables.FilterCount
import com.activityartapp.presentation.ui.theme.spacing
import com.activityartapp.util.enums.SportType

@Composable
fun SectionActivityType(
count: Int,
typesWithSelectedFlag: List<Pair<SportType, Boolean>>,
typesWithSelectedFlag: SnapshotStateList<Pair<SportType, Boolean>>,
eventReceiver: EventReceiver<EditArtViewEvent>
) {
typesWithSelectedFlag.forEach { typeMap ->
Row(
horizontalArrangement = Arrangement.spacedBy(spacing.medium),
verticalAlignment = Alignment.CenterVertically
) {
Checkbox(
checked = typeMap.second,
onCheckedChange = {
eventReceiver.onEvent(
EditArtViewEvent.ArtMutatingEvent.FilterChanged.FilterTypeToggled(
typeMap.first
)
TypeRows(
typesWithSelectedFlag = typesWithSelectedFlag,
onFilterTypeToggled = eventReceiver::onEvent
)
FilterCount(count)
}

@Composable
private fun TypeRows(
typesWithSelectedFlag: List<Pair<SportType, Boolean>>,
onFilterTypeToggled: (FilterChanged.FilterTypeToggled) -> Unit
) {
typesWithSelectedFlag.forEach { selection ->
TypeRowItem(
typeWithSelectedFlag = selection,
onFilterTypeToggled = onFilterTypeToggled
)
}
}

@Composable
private fun TypeRowItem(
typeWithSelectedFlag: Pair<SportType, Boolean>,
onFilterTypeToggled: (FilterChanged.FilterTypeToggled) -> Unit
) {
Row(
horizontalArrangement = Arrangement.spacedBy(spacing.medium),
verticalAlignment = Alignment.CenterVertically
) {
Checkbox(
checked = typeWithSelectedFlag.second,
onCheckedChange = {
onFilterTypeToggled(
FilterChanged.FilterTypeToggled(
type = typeWithSelectedFlag.first
)
})
Text(
text = stringResource(typeMap.first.stringRes),
style = MaterialTheme.typography.body1
)
}
)
})
Text(
text = stringResource(typeWithSelectedFlag.first.stringRes),
style = MaterialTheme.typography.body1
)
}
FilterCount(count)
}
}
2 changes: 1 addition & 1 deletion ...pp/presentation/editArtScreen/subscreens/style/sections/SectionColorBackgroundGradient.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ fun SectionColorBackgroundGradient(
}
)
.background(MaterialTheme.colors.background)
.padding(vertical = spacing.xSmall),
.padding(vertical = spacing.small),
horizontalArrangement = Arrangement.spacedBy(spacing.xSmall)
) {
item { Spacer(modifier = Modifier.width(spacing.xSmall)) }
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/com/activityartapp/util/enums/SportType.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ package com.activityartapp.util.enums

import androidx.annotation.StringRes
import com.activityartapp.R
import javax.annotation.concurrent.Immutable

/**
* Last updated 2/15/2023
* https://developers.strava.com/docs/reference/#api-models-SportType
*/
@Immutable
enum class SportType(@StringRes val stringRes: Int) {
ALPINE_SKI(R.string.sport_type_alpine_ski),
BACKCOUNTRY_SKI(R.string.sport_type_backcountry_ski),
Expand Down

0 comments on commit 8118bbb

Please sign in to comment.