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

Fix #4509: Text-based interaction EditText hints sometimes disappear when the keyboard is closed #4556

Conversation

JishnuGoyal
Copy link
Contributor

@JishnuGoyal JishnuGoyal commented Sep 5, 2022

Explanation

This PR fixes #4509. This PR aims to introduce code that ensures that the hint doesn't disappear for custom input layouts upon focus change events such as keyboard open/close. This is a part of the GSoC project: Interactive Onboarding Flow.

Upon attaching a debugger to the process I was able to investigate that the variable which was responsible to hold the hint information was not being initialized at the right time which was leading to this problem.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Before:

WhatsApp.Video.2022-09-09.at.5.58.22.AM.mp4

After:

WhatsApp.Video.2022-09-09.at.5.58.47.AM.mp4

@JishnuGoyal
Copy link
Contributor Author

@BenHenning PTAL, thanks!

@JishnuGoyal
Copy link
Contributor Author

@rt4914 PTAL for codeowners, thanks!

@oppiabot oppiabot bot assigned rt4914 Sep 5, 2022
@BenHenning
Copy link
Sponsor Member

Hmm this is interesting. @JishnuGoyal do you have an explanation for why the issue existed for some of the text input interactions and not others? I don't actually think #4446 actually introduced this so far as I can tell as it seems like an existing issue.

I think you also should be adding a before/after video (or videos based on whatever works best) showing the broken and now-fixed behavior for each interaction (including ones that were already fixed--it'd be good to see that it still works for them).

Beyond that, I think we also want to add tests for these changes since the difference in code is very subtle, but some thought is going to be needed to figure out how to test this change. Do you have thoughts on how to approach this?

@BenHenning BenHenning assigned JishnuGoyal and unassigned BenHenning Sep 6, 2022
@JishnuGoyal
Copy link
Contributor Author

Hmm this is interesting. @JishnuGoyal do you have an explanation for why the issue existed for some of the text input interactions and not others? I don't actually think #4446 actually introduced this so far as I can tell as it seems like an existing issue.

I think you also should be adding a before/after video (or videos based on whatever works best) showing the broken and now-fixed behavior for each interaction (including ones that were already fixed--it'd be good to see that it still works for them).

Beyond that, I think we also want to add tests for these changes since the difference in code is very subtle, but some thought is going to be needed to figure out how to test this change. Do you have thoughts on how to approach this?

Ah, I have updated the description for the PR, thanks. I haven't myself checked while investigating if all of the custom inputs failed to show hints-- I checked how the fractionInput had problem. The same coding pattern that caused the problem in fractions was used in the other inputs as well, so I fixed those.

I'll be adding the videos for before and after. Where should the tests be added though? @BenHenning

@BenHenning
Copy link
Sponsor Member

@JishnuGoyal InputInteractionViewTestActivityTest and MathExpressionInteractionsViewTest would probably be the best places for those tests to be added.

@JishnuGoyal
Copy link
Contributor Author

@BenHenning PTAL

@oppiabot oppiabot bot assigned BenHenning and unassigned JishnuGoyal Sep 9, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 9, 2022

Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks!

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @JishnuGoyal! I'm happy with this, per our discussion that these changes are actually probably not possible to add automated tests for, and that #4574 is tracking adding tests once feasible. I'm also glad to see how elegant and simple the solution ended up being.

@BenHenning
Copy link
Sponsor Member

This is actually a change that would be nice to pull into beta, so I'm going to go ahead and force-merge it (#4556 is tracking ensuring this gets reviewed by @rt4914 at a later date).

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.

Text-based interaction EditText hints sometimes disappear when the keyboard is closed
3 participants