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

Jump to bottom

Async getQuestionnairePages and Add progress bar to the 'Next' button #2645

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Async getQuestionnairePages and Add progress bar to the 'Next' button #2645

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5ac8323
Refactor QuestionnaireNavigationViewUIState
FikriMilano Jul 27, 2024
0ae5301
Async getQuestionnairePages and Add progress bar to the 'Next' button
FikriMilano Jul 27, 2024
e05493e
spotless
FikriMilano Jul 27, 2024
e3a55b6
Merge branch 'master' into 2639-async-process-for-get-questionnaire-p…
FikriMilano Jul 29, 2024
cea5a07
Merge branch 'master' into 2639-async-process-for-get-questionnaire-p…
FikriMilano Jul 30, 2024
34121d6
Add missing import
ndegwamartin Jul 31, 2024
db8ffbc
Allow style overriding for progress bar
FikriMilano Aug 2, 2024
64deedb
spotless
FikriMilano Aug 2, 2024
4af8cc9
Merge branch '2639-async-process-for-get-questionnaire-pages' of gith…
FikriMilano Aug 2, 2024
68445e0
Merge branch 'master' of github.com:google/android-fhir into 2639-asy…
FikriMilano Nov 5, 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
1 change: 1 addition & 0 deletions datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class QuestionnaireFragment : Fragment() {

// Listen to updates from the view model.
viewLifecycleOwner.lifecycleScope.launchWhenCreated {
viewModel.pages = viewModel.getQuestionnairePages()
viewModel.questionnaireStateFlow.collect { state ->
when (val displayMode = state.displayMode) {
is DisplayMode.ReviewMode -> {
Expand Down
4 changes: 3 additions & 1 deletion ...e/src/main/java/com/google/android/fhir/datacapture/QuestionnaireNavigationViewUIState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package com.google.android.fhir.datacapture
sealed class QuestionnaireNavigationViewUIState(val isShown: Boolean, val isEnabled: Boolean) {
data object Hidden : QuestionnaireNavigationViewUIState(isShown = false, isEnabled = false)

data class Enabled(val labelText: String? = null, val onClickAction: () -> Unit) :
data class Enabled(val labelText: String? = null, val onClickAction: () -> Unit = {}) :
QuestionnaireNavigationViewUIState(isShown = true, isEnabled = true)
}

Expand All @@ -29,4 +29,6 @@ data class QuestionnaireNavigationUIState(
val navSubmit: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden,
val navCancel: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden,
val navReview: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden,
val navNextProgressBar: QuestionnaireNavigationViewUIState =
QuestionnaireNavigationViewUIState.Hidden,
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jingtang10 what do you think of this current code?
Initially I did add a new UI state for loading, but changed my mind, then implement it this current way.

My reasoning:
In the future, I'm thinking there might be other types of UI other than button that might use this UI state, it makes sense to have Loading state for button, but might not be for other types of UI.

Or am I just overthinking it(?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Jing:
Use loading state to update both next button and next button's circular progress indicator together

83 changes: 70 additions & 13 deletions datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.android.fhir.datacapture

import android.app.Application
import android.content.Context
import android.net.Uri
import androidx.annotation.VisibleForTesting
import androidx.lifecycle.AndroidViewModel
Expand All @@ -35,9 +36,12 @@ import com.google.android.fhir.datacapture.extensions.cqfExpression
import com.google.android.fhir.datacapture.extensions.createQuestionnaireResponseItem
import com.google.android.fhir.datacapture.extensions.entryMode
import com.google.android.fhir.datacapture.extensions.filterByCodeInNameExtension
import com.google.android.fhir.datacapture.extensions.flattened
import com.google.android.fhir.datacapture.extensions.forEachItemPair
import com.google.android.fhir.datacapture.extensions.hasDifferentAnswerSet
import com.google.android.fhir.datacapture.extensions.isDisplayItem
import com.google.android.fhir.datacapture.extensions.isEnableWhenReferencedBy
import com.google.android.fhir.datacapture.extensions.isExpressionReferencedBy
import com.google.android.fhir.datacapture.extensions.isHelpCode
import com.google.android.fhir.datacapture.extensions.isHidden
import com.google.android.fhir.datacapture.extensions.isPaginated
Expand All @@ -64,6 +68,7 @@ import com.google.android.fhir.datacapture.validation.Valid
import com.google.android.fhir.datacapture.validation.ValidationResult
import com.google.android.fhir.datacapture.views.QuestionTextConfiguration
import com.google.android.fhir.datacapture.views.QuestionnaireViewItem
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
Expand Down Expand Up @@ -324,6 +329,8 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
*/
private val draftAnswerMap = mutableMapOf<QuestionnaireResponseItemComponent, Any>()

private val isLoadingNextPage = MutableStateFlow(true)

/**
* Callback function to update the view model after the answer(s) to a question have been changed.
* This is passed to the [QuestionnaireViewItem] in its constructor so that it can invoke this
Expand Down Expand Up @@ -378,10 +385,28 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
)
}
modifiedQuestionnaireResponseItemSet.add(questionnaireResponseItem)
viewModelScope.launch(Dispatchers.IO) {
var isReferenced = false
kotlin.run {
isReferenced = questionnaireItem.isExpressionReferencedBy(questionnaire)
if (isReferenced) return@run

questionnaire.item.flattened().forEach { item ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to avoid this for loop? If the questionnaire is long? Also is there any value in avoiding it? Maybe we can just time this part and see how long it takes given a large questionnaire with many expressions and enable whens. If it takes < x% of the time then we don't really need to worry about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

go through only:

  1. pages
  2. do it once

consider to use dependency graph, in the future.

isReferenced = questionnaireItem.isEnableWhenReferencedBy(item)
if (isReferenced) return@run

isReferenced = questionnaireItem.isExpressionReferencedBy(item)
if (isReferenced) return@run
}
}
if (isReferenced) isLoadingNextPage.value = true
Comment on lines +389 to +402
Copy link
Collaborator

Choose a reason for hiding this comment

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

this whole bit should be calculated once for the questionnaire instead of being calculated each time when the answer changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay

modificationCount.update { it + 1 }

updateAnswerWithAffectedCalculatedExpression(questionnaireItem)

modificationCount.update { it + 1 }
updateAnswerWithAffectedCalculatedExpression(questionnaireItem)
pages = getQuestionnairePages()
isLoadingNextPage.value = false
modificationCount.update { it + 1 }
}
Comment on lines +388 to +409
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should investigate if parallel coroutine here could cause any issues incase of answersChangedCallback in quick successions like typing an answer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will investigate

}

private val expressionEvaluator: ExpressionEvaluator =
Expand Down Expand Up @@ -582,6 +607,8 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
.onEach {
if (it.index == 0) {
initializeCalculatedExpressions()
pages = getQuestionnairePages()
isLoadingNextPage.value = false
modificationCount.update { count -> count + 1 }
}
}
Expand Down Expand Up @@ -708,7 +735,6 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
// display all items.
val questionnaireItemViewItems =
if (!isReadOnly && !isInReviewModeFlow.value && questionnaire.isPaginated) {
pages = getQuestionnairePages()
if (currentPageIndexFlow.value == null) {
currentPageIndexFlow.value = pages!!.first { it.enabled && !it.hidden }.index
}
Expand All @@ -728,15 +754,16 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
navSubmit =
if (showSubmitButton) {
QuestionnaireNavigationViewUIState.Enabled(
submitButtonText,
onSubmitButtonClickListener,
labelText = submitButtonText,
onClickAction = onSubmitButtonClickListener,
)
} else {
QuestionnaireNavigationViewUIState.Hidden
},
navCancel =
if (!isReadOnly && shouldShowCancelButton) {
QuestionnaireNavigationViewUIState.Enabled(
labelText = (getApplication() as Context).getString(R.string.cancel_questionnaire),
onClickAction = onCancelButtonClickListener,
)
} else {
Expand Down Expand Up @@ -796,16 +823,31 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
navPrevious =
when {
questionnairePagination.isPaginated && questionnairePagination.hasPreviousPage -> {
QuestionnaireNavigationViewUIState.Enabled { goToPreviousPage() }
QuestionnaireNavigationViewUIState.Enabled(
labelText =
(getApplication() as Context).getString(R.string.button_pagination_previous),
onClickAction = { goToPreviousPage() },
)
}
else -> {
QuestionnaireNavigationViewUIState.Hidden
}
},
navNext =
when {
questionnairePagination.isPaginated &&
questionnairePagination.hasNextPage &&
isLoadingNextPage.value -> {
QuestionnaireNavigationViewUIState.Enabled(
labelText = null,
)
}
questionnairePagination.isPaginated && questionnairePagination.hasNextPage -> {
QuestionnaireNavigationViewUIState.Enabled { goToNextPage() }
QuestionnaireNavigationViewUIState.Enabled(
labelText =
(getApplication() as Context).getString(R.string.button_pagination_next),
onClickAction = { goToNextPage() },
)
}
else -> {
QuestionnaireNavigationViewUIState.Hidden
Expand All @@ -814,24 +856,39 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
navSubmit =
if (showSubmitButton) {
QuestionnaireNavigationViewUIState.Enabled(
submitButtonText,
onSubmitButtonClickListener,
labelText = submitButtonText,
onClickAction = onSubmitButtonClickListener,
)
} else {
QuestionnaireNavigationViewUIState.Hidden
},
navReview =
if (showReviewButton) {
QuestionnaireNavigationViewUIState.Enabled { setReviewMode(true) }
QuestionnaireNavigationViewUIState.Enabled(
labelText = (getApplication() as Context).getString(R.string.button_review),
onClickAction = { setReviewMode(true) },
)
} else {
QuestionnaireNavigationViewUIState.Hidden
},
navCancel =
if (showCancelButton) {
QuestionnaireNavigationViewUIState.Enabled(onClickAction = onCancelButtonClickListener)
QuestionnaireNavigationViewUIState.Enabled(
labelText = (getApplication() as Context).getString(R.string.cancel_questionnaire),
onClickAction = onCancelButtonClickListener,
)
} else {
QuestionnaireNavigationViewUIState.Hidden
},
navNextProgressBar =
when {
questionnairePagination.isPaginated && isLoadingNextPage.value -> {
QuestionnaireNavigationViewUIState.Enabled()
}
else -> {
QuestionnaireNavigationViewUIState.Hidden
}
},
)
val bottomNavigationItems =
listOf(QuestionnaireAdapterItem.Navigation(bottomNavigationUiViewState))
Expand Down Expand Up @@ -1082,7 +1139,7 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
* Gets a list of [QuestionnairePage]s for a paginated questionnaire, or `null` if the
* questionnaire is not paginated.
*/
private suspend fun getQuestionnairePages(): List<QuestionnairePage>? =
internal suspend fun getQuestionnairePages(): List<QuestionnairePage>? =
if (questionnaire.isPaginated) {
questionnaire.item.zip(questionnaireResponse.item).mapIndexed {
index,
Expand Down
26 changes: 25 additions & 1 deletion ...in/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,10 @@ internal val QuestionnaireItemComponent.calculatedExpression: Expression?
it.castToExpression(it.value)
}

/** Returns list of extensions whose value is of type [Expression] */
internal val Questionnaire.expressionBasedExtensions
get() = this.extension.filter { it.value is Expression }

/** Returns list of extensions whose value is of type [Expression] */
internal val QuestionnaireItemComponent.expressionBasedExtensions
get() = this.extension.filter { it.value is Expression }
Expand All @@ -694,7 +698,7 @@ internal val QuestionnaireItemComponent.expressionBasedExtensions
* (e.g. if [item] has an expression `%resource.item.where(linkId='this-question')` where
* `this-question` is the link ID of the current questionnaire item).
*/
internal fun QuestionnaireItemComponent.isReferencedBy(
internal fun Questionnaire.QuestionnaireItemComponent.isExpressionReferencedBy(
item: QuestionnaireItemComponent,
) =
item.expressionBasedExtensions.any {
Expand All @@ -705,6 +709,26 @@ internal fun QuestionnaireItemComponent.isReferencedBy(
.contains(Regex(".*linkId='${this.linkId}'.*"))
}

internal fun Questionnaire.QuestionnaireItemComponent.isExpressionReferencedBy(
questionnaire: Questionnaire,
) =
questionnaire.expressionBasedExtensions.any {
it
.castToExpression(it.value)
.expression
.replace(" ", "")
.contains(Regex(".*linkId='${this.linkId}'.*"))
}

/**
* Whether [item] has any expression directly referencing the current questionnaire item by link ID
* (e.g. if [item] has an expression `%resource.item.where(linkId='this-question')` where
* `this-question` is the link ID of the current questionnaire item).
*/
internal fun Questionnaire.QuestionnaireItemComponent.isEnableWhenReferencedBy(
item: Questionnaire.QuestionnaireItemComponent,
) = item.enableWhen.any { it.question == this.linkId }

internal val QuestionnaireItemComponent.answerExpression: Expression?
get() =
ToolingExtensions.getExtension(this, EXTENSION_ANSWER_EXPRESSION_URL)?.value?.let {
Expand Down
9 changes: 6 additions & 3 deletions ...capture/src/main/java/com/google/android/fhir/datacapture/fhirpath/ExpressionEvaluator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import com.google.android.fhir.datacapture.XFhirQueryResolver
import com.google.android.fhir.datacapture.extensions.calculatedExpression
import com.google.android.fhir.datacapture.extensions.findVariableExpression
import com.google.android.fhir.datacapture.extensions.flattened
import com.google.android.fhir.datacapture.extensions.isExpressionReferencedBy
import com.google.android.fhir.datacapture.extensions.isFhirPath
import com.google.android.fhir.datacapture.extensions.isReferencedBy
import com.google.android.fhir.datacapture.extensions.isXFhirQuery
import com.google.android.fhir.datacapture.extensions.variableExpressions
import org.hl7.fhir.exceptions.FHIRException
Expand Down Expand Up @@ -133,7 +133,10 @@ internal class ExpressionEvaluator(
// no calculable item depending on current item should be used as dependency into current
// item
this.forEach { dependent ->
check(!(current.isReferencedBy(dependent) && dependent.isReferencedBy(current))) {
check(
!(current.isExpressionReferencedBy(dependent) &&
dependent.isExpressionReferencedBy(current)),
) {
"${current.linkId} and ${dependent.linkId} have cyclic dependency in expression based extension"
}
}
Expand Down Expand Up @@ -197,7 +200,7 @@ internal class ExpressionEvaluator(
// Condition 1. item is calculable
// Condition 2. item answer depends on the updated item answer OR has a variable dependency
item.calculatedExpression != null &&
(questionnaireItem.isReferencedBy(item) ||
(questionnaireItem.isExpressionReferencedBy(item) ||
findDependentVariables(item.calculatedExpression!!).isNotEmpty())
}
.map { item ->
Expand Down
22 changes: 19 additions & 3 deletions datacapture/src/main/java/com/google/android/fhir/datacapture/views/NavigationViewHolder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import androidx.recyclerview.widget.RecyclerView
import com.google.android.fhir.datacapture.QuestionnaireNavigationUIState
import com.google.android.fhir.datacapture.QuestionnaireNavigationViewUIState
import com.google.android.fhir.datacapture.R
import com.google.android.material.progressindicator.CircularProgressIndicator

class NavigationViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {

Expand All @@ -41,21 +42,36 @@ class NavigationViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
itemView
.findViewById<Button>(R.id.submit_questionnaire)
.updateState(questionnaireNavigationUIState.navSubmit)
itemView
.findViewById<CircularProgressIndicator>(R.id.pagination_next_button_progress_bar)
.updateState(questionnaireNavigationUIState.navNextProgressBar)
}

private fun Button.updateState(navigationViewState: QuestionnaireNavigationViewUIState) {
when (navigationViewState) {
is QuestionnaireNavigationViewUIState.Enabled -> {
visibility = View.VISIBLE
isEnabled = true
if (navigationViewState.labelText.isNullOrBlank().not() && this is Button) {
text = navigationViewState.labelText
}
text = navigationViewState.labelText
setOnClickListener { navigationViewState.onClickAction() }
}
QuestionnaireNavigationViewUIState.Hidden -> {
visibility = View.GONE
}
}
}

private fun CircularProgressIndicator.updateState(
navigationViewState: QuestionnaireNavigationViewUIState,
) {
visibility =
when (navigationViewState) {
is QuestionnaireNavigationViewUIState.Enabled -> {
View.VISIBLE
}
QuestionnaireNavigationViewUIState.Hidden -> {
View.GONE
}
}
}
}
11 changes: 11 additions & 0 deletions datacapture/src/main/res/layout/pagination_navigation_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@
app:layout_constraintBottom_toBottomOf="parent"
/>

<com.google.android.material.progressindicator.CircularProgressIndicator
android:id="@+id/pagination_next_button_progress_bar"
style="?attr/questionnaireNextButtonCircularProgressIndicatorStyle"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:layout_constraintTop_toTopOf="@id/pagination_next_button"
app:layout_constraintBottom_toBottomOf="@id/pagination_next_button"
app:layout_constraintStart_toStartOf="@id/pagination_next_button"
app:layout_constraintEnd_toEndOf="@id/pagination_next_button"
/>

<Button
android:id="@+id/review_mode_button"
style="?attr/questionnaireButtonStyle"
Expand Down
6 changes: 6 additions & 0 deletions datacapture/src/main/res/values/attrs.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@
<!-- Style for previous and next buttons in paginated layout. -->
<attr name="questionnaireButtonStyle" format="reference" />

<!-- Style for for circular progress indicator for next button in paginated layout. -->
<attr
name="questionnaireNextButtonCircularProgressIndicatorStyle"
format="reference"
/>

<!-- Style for submit button. -->
<attr name="questionnaireSubmitButtonStyle" format="reference" />

Expand Down
2 changes: 1 addition & 1 deletion datacapture/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<string name="review_answers_text">Review answers</string>
<string name="button_review">Review</string>
<string name="submit_questionnaire">Submit</string>
<string name="cancel_questionnaire">@android:string/cancel</string>
<string name="cancel_questionnaire">Cancel</string>

<!-- 1.2 Submit Pop-up -->
<string name="questionnaire_validation_error_headline">Errors found</string>
Expand Down
Loading
Loading