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.

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 custom callback for autocomplete #2764

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

Conversation

dilys-l
Copy link

@dilys-l dilys-l commented Jan 3, 2025

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2738

Description
Adds the ability for the client to pass a custom callback to AutoComplete for dynamic AutoComplete answer resolution

Alternative(s) considered
NA

Type
Feature

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@dilys-l dilys-l requested a review from a team as a code owner January 3, 2025 03:26
@dilys-l dilys-l requested a review from aditya-07 January 3, 2025 03:26
Copy link

google-cla bot commented Jan 3, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Comment on lines 50 to 58
callbacks = mapOf(Pair(CustomCallbackType.AUTO_COMPLETE, AutoCompleteCallback(
callback = { query ->
run {
listOf(AutoCompleteViewAnswerOption("a", "Type 2 Diabetes Mellitus"),
AutoCompleteViewAnswerOption("b", "Test")
)
}
}
)))
Copy link
Collaborator

@FikriMilano FikriMilano Jan 7, 2025

Choose a reason for hiding this comment

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

on line 51, other than the query, I think we also need the value set resolver url to be passed in the API.

the url will be provided by the questionnaire item, what it does that it can point to a terminology server to retrieve the option lists, then the query will filter that lists so we can have less options to process

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think what fikri meant is tha tthe url should be passed as another parameter besides the query string itself. otherwise how do we distinguish different valueset searches?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah good point. I will make this change.

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Thanks @dilys-l! A couple of questions below.

Comment on lines 50 to 58
callbacks = mapOf(Pair(CustomCallbackType.AUTO_COMPLETE, AutoCompleteCallback(
callback = { query ->
run {
listOf(AutoCompleteViewAnswerOption("a", "Type 2 Diabetes Mellitus"),
AutoCompleteViewAnswerOption("b", "Test")
)
}
}
)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think what fikri meant is tha tthe url should be passed as another parameter besides the query string itself. otherwise how do we distinguish different valueset searches?

}

enum class CustomCallbackType {
AUTO_COMPLETE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are the other callbacks types?

i think the definition of the callback should probably be the same for different widgets (e.g. auto complete, or dropdown), but the ui should behave differently.

Copy link
Author

Choose a reason for hiding this comment

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

My initial thinking was to make it a bit more open ended, but i agree that standardising the definition would be better. I'll make this change

Comment on lines 50 to 51
callbacks = mapOf(Pair(CustomCallbackType.AUTO_COMPLETE, AutoCompleteCallback(
callback = { query ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

this code is throwing an error when I tried to build the project, could you have a look?
and since this callback is being used to resolve searched options from value sets, should we rename it to something else?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, i forgot to include the update for this file. Is there a preferred naming convention for something like this?
Perhaps a ValueSetLookupResolver or similar?
I'll update the code once we agree on a new name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's a good name actually

@@ -391,3 +391,5 @@ internal object DiffCallbacks {
}
}
}

typealias CustomCallback<T> = (String, String) -> List<T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we expecting any other types than coding?

Copy link
Author

Choose a reason for hiding this comment

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

probably not, i can change this

* A [CustomCallback] may be set by the client to override the behaviour of an existing component
* in the sdc.
*/
var callback: CustomCallback<*>? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you just add an option url to ExternalAnswerValueSetResolver below would it achieve the same thing?

Copy link
Author

Choose a reason for hiding this comment

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

the way that answerOptionValues are handled in AutoCompleteViewHolderFactory would need to change. i could probably change it so that it's handled in the adapter either way, but then the resolver would be triggered every time the query changes which might not be ideal

* A [CustomCallback] may be set by the client to override the behaviour of an existing component
* in the sdc.
*/
var callback: CustomCallback<*>? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

since we are going to use this callback only for valueSetLookupResolver, should we change the type to (String, String) -> List<Coding>?

Comment on lines 318 to 323
if (callback != null && answerValueSet != null && objects.isEmpty()) {
(callback as? CustomCallback<AutoCompleteViewAnswerOption>)?.invoke(
query,
answerValueSet,
)
?: emptyList()
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you move this implementation to the QuestionnaireViewModel? resolving the options in the UI code seems a bit wrong

val (enabledQuestionnaireAnswerOptions, disabledQuestionnaireResponseAnswers) =
answerOptionsEvaluator.evaluate(
questionnaireItem,
questionnaireResponseItem,
)

dilys-l and others added 2 commits January 23, 2025 11:57
- Combined functionality with the ExternalAnswerValueSetResolver
- Renamed various functions and arguments for improved clarity
- Moved filter handling out of the adapter and into the textChangedListener
- Moved resolver to the viewmodel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance the ExternalAnswerValueSetResolver to take a search string
3 participants