search

gazlaws-dev / codeboard

Codeboard App
Other
566 stars 116 forks source link

Set switchedKeyboard before showing keyboard picker. #54

Closed DDoSolitary closed 5 years ago

DDoSolitary commented 6 years ago

Fix #51.

gazlaws-dev commented 5 years ago

This only changes the variable, not the order of onKey and onKeyLongPress. Also, I cannot recreate issue #54

DDoSolitary commented 5 years ago

I'm quite surprised because I've observed this in a number of ROMs, including resurrection remix pie and MIUI(Xiaomi’s official ROM) pie. I can upload a screen record of this issue if you want.

I'm not an Android developer but from what I can see from the code, I guess the following happens when I long press the space button.

  1. I press down the space button, onPress is called and a timer is started.
  2. Timer goes up and onKeyLongPress is called.
  3. The keyboard picker is shown and the space button is released (although I didn't release my finger, but the button does get released. It's easier to notice if you experiment with Gboard, because background color of the button changes after the picker pops up)
  4. onKey is called but switchedKeyboard hasn't been set yet, so a space is written.
  5. showInputMethodPicker returns and switchedKeyboard gets set.

I don't know if showInputMethodPicker returns after the picker is shown or after it is closed. But given that you can't reproduce this issue, I think it's reasonable to assume the former. If that is the case, then I guess there's some sort of racing happening here: on your device showInputMethodPicker returns faster and the flag is set before onKey reads it, while on my device onKey runs faster.

Again, I'm not an Android developer, so I might have some mistakes in my theory and I can't test my patch because I don't know how to build and debug this app. Please let me know if you spotted any mistakes. And could you please build the app with the patch applied and upload the apk so that I can see if the patch actually fixes the issue on my device. Thanks!

PS: You said that "feel free to reopen it" in the issue I opened, but an issue can't be reopened by the original poster if it is closed by a repo owner/collaborator.

DDoSolitary commented 5 years ago

screenrecord.tar.gz

gazlaws-dev commented 5 years ago

Thanks for looking into it! I'll upload an apk here, please do test it and let me know.

app.zip

DDoSolitary commented 5 years ago

Just tested and it works fine, but I just found out it might not be my patch doing the magic:

  1. There's already a commit in the commit history to address this issue. I checked the version I was having problem with (installed from Play Store) and found that its rather old. It seems that the version on Play Store is even older than your commit to fix this.
  2. I re-read the code and noticed that onKeyLongPress is executed in the UI thread rather than a separate thread as I previously thought, so there's no chance for a race to occur (or it's an Android's bug...)

So could you please upload an apk with my commit reverted to verify this? And it will be nice to have an up-to-date version available at Play Store after this issue (and probably other issues that you want to fix now) is properly dealt with.

gazlaws-dev commented 5 years ago

Yeah, I really need to update the Play Store version. Here's the original apk app_og.zip

DDoSolitary commented 5 years ago

The original one works fine either, so my patch is completely unnecessary 🤣