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

Fixed showing keyboard #5843

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Nov 21, 2023

Closes #5828

Why is this the best possible solution? Were any other approaches considered?

It looks like on some devices there is a problem with showing a keyboard just after hiding it. We used to hide the keyboard every time we created a new question view and then show it if needed. I guess the short time between those two operations might be the cause.
To fix the problem I've removed hiding the keyboard before creating a new question view. We do that either way for questions that do not need the keyboard in setFocus method. The only problem is that it will happen a little bit later and that difference might be visible on some devices.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It's important to test the changes on all our devices. The risk here is that on some of them showing/hiding the keyboard might not work well.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@lognaturel
Copy link
Member

lognaturel commented Nov 21, 2023

Works great on the Galaxy A13 🎉

I did notice that when I jump to a text question the keyboard is not shown. I believe that's a change but I'll need to verify. We should also check things like whether the keyboard shows if the first question in a form requires it.

@lognaturel lognaturel added the high priority Should be looked at before other PRs/issues label Nov 22, 2023
@grzesiek2010
Copy link
Member Author

I did notice that when I jump to a text question the keyboard is not shown. I believe that's a change but I'll need to verify.

I can reproduce it too but it's not caused by this pr. I will try to address that issue as well if it's possible.

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Nov 23, 2023

@lognaturel I think I was able to fix that second issue, please test it.
I don't like the solution: https://github.com/getodk/collect/pull/5843/files#diff-c2ee7ae36f7c566b42448bec3d149e4f128d0e941e5450d5ad3f64aa37692613R12 but this is what we also do in other places:

I'm going to file a separate issue for reviewing those problems and implementing a better and more reliable solution.

[EDIT] the issue is here #5853

@grzesiek2010 grzesiek2010 marked this pull request as ready for review November 23, 2023 13:36
@seadowg seadowg linked an issue Nov 23, 2023 that may be closed by this pull request
@grzesiek2010
Copy link
Member Author

@lognaturel is off so I'm adding @seadowg as a reviewer too.

@lognaturel
Copy link
Member

I’m not at a computer today 🦃 but wanted to say that it’s not a very significant issue so if you both dislike the implementation we could address it another time or not at all.

@grzesiek2010
Copy link
Member Author

I’m not at a computer today 🦃 but wanted to say that it’s not a very significant issue so if you both dislike the implementation we could address it another time or not at all.

I mean I don't like it because it's another case where we use a workaround like that what I have described in #5853 but if it works that's ok for v2023.3.1. I think it's not risky and 100ms is not a significant delay.

}
// Subtle delay in displaying the keyboard is necessary for the keyboard to be displayed at all,
// for example, when returning from the hierarchy view.
Handler(Looper.getMainLooper()).postDelayed({
Copy link
Member

Choose a reason for hiding this comment

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

Were you able to experiment shorter delays than 100ms here or even just adding no time based delay (with just a post)?

Also, we should probably use Scheduler here rather than referencing Handler directly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Were you able to experiment shorter delays than 100ms here or even just adding no time based delay (with just a post)?

I think I did and maybe it even worked but wanted to use a delay similar to what we have in other places (see above) to not risk that on some devices it wouldn't be enough.

Also, we should probably use Scheduler here rather than referencing Handler directly here.

As I said it's rather a temporary solution so I didn't want to implement too many changes but passing scheduler to SoftKeyboardController would require more work.

Copy link
Member

Choose a reason for hiding this comment

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

Ah you mean that we'd get rid of this after #5853? I'll make sure that's in the next milestone.

@dbemke
Copy link

dbemke commented Nov 29, 2023

When the issue was filed I wasn't able to reproduced it on my devices (one of them is Samsung Galaxy M12 Android 11). I've seen the keyboard missing twice in 2 months (happening randomly) but wasn't able to reproduce it afterwards.
I tested the PR on Android 8.1 (Nexus), Android 10 (Redmi 9t), Android 11 ( Galaxy M12) after many tries I managed to get to this state once.
identifyUserDontKeepAct

It's the identify User form + don't keep activities enabled in device settings and some minimizing the app and some swipe. I'm not able to determine the factors/ steps as I'm not able to reproduce it again.

@srujner
Copy link

srujner commented Nov 29, 2023

In the same form (Identity user) + don't keep activities enabled I was able to reproduce the missing keyboard:

XRecorder_29112023_165728.mp4

Other than that I checked on Android 5.1, 12 and 13 and all other issues has been fixed

@lognaturel @grzesiek2010 if you decide that the one thing that we found with the Identity User form is not something that we should be worried about, then we're Ok with Closing this PR and merging it.

@lognaturel
Copy link
Member

Wow! That’s an interesting case. I think we shouldn’t worry about it for this PR because it seems much rarer. If that sounds right to the rest of you, let’s merge and get this point release out!

@lognaturel lognaturel merged commit b81862e into getodk:v2023.3.x Nov 29, 2023
6 checks passed
@grzesiek2010
Copy link
Member Author

I agree @srujner you can file a separate issue for that case.

seadowg pushed a commit to seadowg/collect that referenced this pull request Nov 30, 2023
@seadowg seadowg mentioned this pull request Nov 30, 2023
seadowg pushed a commit to seadowg/collect that referenced this pull request Dec 7, 2023
seadowg pushed a commit to seadowg/collect that referenced this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard collapsed on some text field (every other?)
5 participants