Fix cursor moving back and missing characters

[MJ1998]

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

Fixes #1914

Cause of the issue: Sometimes processing of input is slower than the typing speed. This results into inconsistency between the answer stored in the QuestionnaireViewItem and the current text in the EditText. The UI is updated everytime the QuestionnaireViewItem is bound. So here we see the inconsistency where we use EditText#setText which does 2 things: Sets the EditText content to the answer in QuestionnaireViewItem (leading to missing character) AND also sets the cursor to beginning of the input.

Solution:-

1. Cursor does not go to the beginning: EditText#append moves the cursor to the last position.
2. Characters are not missed (skipped): We separate the validation update from input-text-box update. And we update the input text box only when the view is not in focus. This happens only when EITHER the RecyclerView loads a new question into the view OR there is a read-only question. This results into not updating the EditText programmatically for each user input.

The EditText temporarily contains an extra character. The TextWatcher gets removed (here) before it can process this extra character, leaving the saved text state (in QuestionnaireViewItem) inconsistent with the actual EditText content. This inconsistency triggers a UI update using EditText#setText (here), which unintentionally resets the cursor position to the beginning.

Read comments in rest of the PR for more details.

Description
Clear and concise code change description.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)

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).

[MJ1998]

This, however, has a side effect (which existed earlier as well but was overlooked because of cursor resetting) of missing some characters when typing too fast.
Reason: EditText's extra character is skipped when using the above solution.

Solving this will require more effort. I can think of a solution which will involve detecting "user has stopped typing". Something like this - stackoverflow.

Let me know if you want to fix this as well. Will raise a separate issue for this.

Alternative: Use RxBindings or do some backpressure solutioning to handle all character inputs from user. But this will need a more complex handling - like "not updating the EditText until upstream processing (handleInput) is complete".

[pld]

@jingtang10 what are your thoughts on this?

[MJ1998]

I made an attempt at solving the "side effect" as well. Seems good to me but Idk if this behaviour - "detect user has stopped typing" can have any other side effects.

Only difficulty is to test this as racing conditions are always a pain to test.
How I tested it - Ran my finger over "QWERTY" tabs on keyboard back and forth really fast. Before the last commit I could see some missed characters. After the last commit, I didn't see any missed character in quite some attempts trying this test.

Can you try testing this PR ?
@pld

[AngelaKabari]

Thanks for all your work on this @MJ1998. @FikriMilano will test the PR tomorrow and we'll get back to you.

[FikriMilano]

Thanks @MJ1998, I like your first idea and it's working, in a nutshell -> only apply new answer to questionnaireViewItem after user stops typing for 0.5 second, if there are more inputs coming in, the answer won't be applied since the timer is canceled.

Not sure if it's worth it to introduce RxBinding right now.

override fun afterTextChanged(editable: Editable?) {
        timer?.cancel()
        // If editable has become empty or this is the first character then invoke afterTextChanged
        // instantly else start a timer for user to finish typing
        if (editable?.toString().isNullOrEmpty() || firstCharacter) {
          Log.d("FIKRI1", "$editable")
          afterTextChanged(editable?.toString())
        } else {
          // countDownInterval is simply kept greater than delay as we don't need onTick
          timer =
            object : CountDownTimer(delay, delay * 2) {
                override fun onTick(millisUntilFinished: Long) {}

                override fun onFinish() {
                  Log.d("FIKRI2", "$editable")
                  afterTextChanged(editable?.toString())
                }
              }
              .start()
        }
      }

2024-05-09 11:24:26.372  2316-2316  FIKRI1                  com.google.android.fhir.catalog      D  
2024-05-09 11:24:27.912  2316-2316  FIKRI1                  com.google.android.fhir.catalog      D  a
2024-05-09 11:24:30.037  2316-2316  FIKRI2                  com.google.android.fhir.catalog      D  aa
2024-05-09 11:24:35.997  2316-2316  FIKRI2                  com.google.android.fhir.catalog      D  aaqwertqwertqwertqwertqwert

qwerty.webm

[MJ1998]

Just FYI an elaboration on how I am testing this:-
I slide my finger and keep it pressed over "QWERTY" keys back and forth. So the input I generate is "qwertytrewqwertytrewq...".

You can observe some characters being missed before the last commit if you test like this. @FikriMilano

In actual case the upstream function can take more time to finish (for example, resolving fhir query) - which makes characters missing clearly visible.

[MJ1998]

Regarding your question on RxBinding, I think its a much bigger effort as we dont use simple TextView or EditText views, rather its a EditTextInputLayout which offers more features like validation, header views along with it.

So moving from EditTextInputLayout to RxBinding views will be difficult.
@FikriMilano

[FikriMilano]

@MJ1998 I agree, we can keep it this way for now

[AngelaKabari]

@MJ1998 this fix you shared is working great. Thank you for prioritising this and getting it resolved so quickly!

[AngelaKabari]

@MJ1998 upon further testing, the issue of the missing characters is significantly affecting the data entry.

Missing.letters.bug.webm

Can we please go back to the drawing board on this?

[MJ1998]

Is this issue happening on this PR?

[AngelaKabari]

@MJ1998 yes it is.

[MJ1998]

Gotcha. I had a feeling this will introduce more side effects.
Can you tell me if there is any expression in your questionnaire's items that are dependent (directly or indirectly) on this item being modified ?

[FikriMilano]

@MJ1998 there's none, it's just a normal text field

[MJ1998]

Can you try increasing the DELAY TIME to 1000 or 2000 milli seconds and see if this is happening ?
Not the best solution but I am trying to figure out what is causing the delay in the upstream function (that validates the input string and sends it to the QuestionnaireViewModel for updating the RecyclerView) if there is no dependent items also.
To increase delay time search for this line: textInputEditText.afterTextChangedDelayed(500)

[FikriMilano]

@MJ1998 after investigation from my end, the cursor issue only happens in one of our project where it has some additional features that the Android FHIR SDK doesn't have yet (I do plan to push them to this repo, with finalization). This PR's change seems to not work well with those additional features, so the fix will come from my end. After reverting some of our additional features, the cursor issue is gone. Plus, when I try the catalog app, the cursor issue doesn't happen.

Your solution is likely already correct.
Thanks a lot for looking into this!

@f-odhiambo @ndegwamartin for the main FHIRCore branch, feel free to pull this change, the issue should not happen since main doesn't have additional feature which I refer before.

[FikriMilano]

@MJ1998 but yeah, we will try it in our main FHIRCore branch then get back to you on the result.

[FikriMilano]

@MJ1998 in the main branch, the solution works on my end, but it's showing a glitch in @f-odhiambo device.

screen-20240528-152054.mp4

[FikriMilano]

@MJ1998 could you try my other solution? (a different one, this won't break calculatedExpression)

It's a rough solution, but the idea is to prevent unwanted re-binding by only bind the text from questionnaireViewItem, if the text is different compared to before the re-binding.

Highlight of the change:

   val answer = questionnaireViewItem.answers.singleOrNull()?.value?.asStringValue()
    if (answer != textInputEditText.text.toString()) {
      updateUI(questionnaireViewItem, textInputEditText, textInputLayout)
    }

More details:

override fun bind(questionnaireViewItem: QuestionnaireViewItem) {
    header.bind(questionnaireViewItem)
    with(textInputLayout) {
      hint = questionnaireViewItem.enabledDisplayItems.localizedFlyoverSpanned
      helperText = getRequiredOrOptionalText(questionnaireViewItem, context)
    }
    displayValidationResult(questionnaireViewItem.validationResult)

    textInputEditText.removeTextChangedListener(textWatcher)
    val answer = questionnaireViewItem.answers.singleOrNull()?.value?.asStringValue()
    if (answer != textInputEditText.text.toString()) {
      updateUI(questionnaireViewItem, textInputEditText, textInputLayout)
    }

    unitTextView?.apply {
      text = questionnaireViewItem.questionnaireItem.unit?.code
      visibility = if (text.isNullOrEmpty()) GONE else VISIBLE
    }

    textWatcher =
      textInputEditText.doAfterTextChanged { editable: Editable? ->
        context.lifecycleScope.launch { handleInput(editable!!, questionnaireViewItem) }
      }
  }

bug-fix.mov

[MJ1998]

Can you try this solution in @f-odhiambo device and confirm if it works ?
I am not sure because that if condition is what we are anyway doing in updateUI.

[FikriMilano]

Also NVM about that, I was testing it again with more random words, and it failed the tests...

[MJ1998]

Context: User input is sent back to QuestionnaireViewModel for validation, updating dependent items and then building the RecyclerView list -> which then comes back to the EditViewHolderFactory.
The problem is happening when user input speed exceeds the speed at which this loop finishes.
So, post discussing with @jingtang10, here's what we are implementing:
We will detect new user input and when loop comes back to EditViewHolderFactor we will cut off the loop to prevent updating UI when THERE IS new user input present!
@FikriMilano

[pld]

yay, awesome, thank you @MJ1998 🎉 !

[FikriMilano]

@MJ1998 awesome work! 🚀

[aditya-07]

The approach taken to fix the issue looks fine and the changes look good as well.
Just a comment to add Unit test to make sure that the code keeps working as expected.

[jingtang10]

thanks jajoo for this!

[aditya-07]

LGTM! Thanks @MJ1998 !