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

Soft-Keyboard numpad #188

Closed
wants to merge 10 commits into from
Closed

Soft-Keyboard numpad #188

wants to merge 10 commits into from

Conversation

nebkrid
Copy link
Contributor

@nebkrid nebkrid commented Feb 17, 2023

It is not exactly as in your suggestion from #159, but similar to it. Avoiding the left control column give more room for the buttons. In order to include more options with avoiding a second control row long presses on the existing control buttons might be an option.

@sspanak sspanak self-requested a review February 20, 2023 13:08
Copy link
Owner

@sspanak sspanak left a comment

Choose a reason for hiding this comment

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

I tried it out and I must say it looks great! Appart from the fact it needs minor visual adjustment, it is almost like I imagined it.

However, from functional perspective, it needs more more work before I can approve it:

  1. It is not possible to select a suggestion by touching it. And this means the suggestion list is basically unusable on a touchscreen-only device. The first word is not always the one users would want to type.
  2. The full-sized keypad you created replaces the current simple one. This will be unacceptable for some users, including me. I would like to keep the small toolbar-like and use the touch Backspace and Settings key, but type using the physical keys, and only have the full keypad on my tablet, which doesn't have hardware keys. So, a three way option is a must.
  3. The "inscribed" letters on the keys do not change when the language changes. This is not OK for Cyrillic-based languages, where the layout is different.

@nebkrid
Copy link
Contributor Author

nebkrid commented Mar 11, 2023

Sorry for my late reply and thanks for your feedback.

It is not possible to select a suggestion by touching it. And this means the suggestion list is basically unusable on a touchscreen-only device. The first word is not always the one users would want to type.

I am missing this feature, too. But this has to be written somewhere completely else in the code (the spot where the suggestions are populated). I will look into this, but still have to find and understood how acutally a complete word is then sent to the edit text field. Found and implemented. :)

The full-sized keypad you created replaces the current simple one. This will be unacceptable for some users, including me. I would like to keep the small toolbar-like and use the touch Backspace and Settings key, but type using the physical keys, and only have the full keypad on my tablet, which doesn't have hardware keys. So, a three way option is a must.

Yes, an additional menu option would be useful. Or do you think an automatic suggestion based on the screen size via the android system of xml-file choosing is better? - implemented as preference setting. Was actually easier than I thought and in this way everyone can choose by its own.

The "inscribed" letters on the keys do not change when the language changes. This is not OK for Cyrillic-based languages, where the layout is different.

Thanks to pointing for this bug. This should be easily fixable. Fixed. :)

@nebkrid
Copy link
Contributor Author

nebkrid commented Mar 12, 2023

Please let me know if there is still something crucial missing or in case of any bugs.

@sspanak
Copy link
Owner

sspanak commented Mar 14, 2023

Please let me know if there is still something crucial missing or in case of any bugs.

I am going to need some time for a detailed code review.

Anyway, here are several things that I noticed when I skimmed the code and tested a bit:

  • Holding the number keys does not work in Predictive mode, but it should allow typing the respective number.
  • There is some commented out and debugging code here and there. Please clean up all that.
  • The light theme is not properly applied. I saw some commented out code, it may be because of that.
  • Do not throw Exceptions. This is bad, it would cause the keyboard to die, meaning the user will no longer have control. Instead, find a way to gracefully handle the case or perhaps, just ignore it.
  • I didn't get what the new DPAD function does and it does not seem to work on my phone. The direction keys work as usual even if it is enabled.
  • I don't like what is going on in SoftKeyHandler.java, but I need some more time to suggest something better. I am thinking of maybe splitting the code somehow and have one class for the small keyboard view and one for the full-sized one.

For now, you could clean up the code, while I think of how to improve the view handler.

@nebkrid
Copy link
Contributor Author

nebkrid commented Mar 19, 2023

Thanks for your feedback!

Holding the number keys does not work in Predictive mode, but it should allow typing the respective number.

Strange. Since I just send KEY-Action UP- and Down-Events to the TT9 class, all behaviour should be the same. How/Where is this solved with the physical buttons?

There is some commented out and debugging code here and there. Please clean up all that.

Done, except for one line which is explicitly marked as reference (also see below dpad navigation).

The light theme is not properly applied. I saw some commented out code, it may be because of that.

Good point, I forgot to check this... I will have a look on this.

Do not throw Exceptions. This is bad, it would cause the keyboard to die, meaning the user will no longer have control. Instead, find a way to gracefully handle the case or perhaps, just ignore it.

Actually this is intended since this happens only if a developer (me or someone in future using this method) send a not supported Event, so he will directly note it while testing. During normal usage there is no chance that this will throw an error (if tested at least one time). However, if it is easy to send toast messages from this code, this might be an option, too. But currently I don't know how to do it from this point of the code.

I didn't get what the new DPAD function does and it does not seem to work on my phone. The direction keys work as usual even if it is enabled.

Explicit usecase is Android TV: There is no Touch-Screen but huge space on screen to show buttons for accessing e.g. settings menu, if one does not remember which long-press shortcut button was defined once weeks ago. So one can navigate with the dpad to the button. However, if this is enabled, this function breaks the default behaviour of DPAD_DOWN. To avoid this, I added the preference option. As written in the description of the setting, this is not working on Touch-Screen-Devices (since nobody should need it there and technically enabling this would have side-effects: In order to enable it, the buttons need to be setFocusableInTouchMode(true) which at least on my mobile caused this behaviour: first touch -> focused ("highlighted") but nothing happens, second touch -> click). If you want to try this option uncomment settingsButton.SetFocusable... in SoftKeyHandler.java handleDpadNavigation(...)-method, but be warned that the "Highlighting" is not very obvious (at least in dark mode), so look detailed on the visual change of the settings button (slightly darker). In order to navigate over more buttons, you would need to set them focusable, too. Since I don't have a Touch-Screen device with dpad, I tested it using adb sending Key Events (I think it was adb shell input keyevent YOUR_KEY_EVENT) and on the TV it is working out-of-the-box as intended for non-touchscreen-devices.

I don't like what is going on in SoftKeyHandler.java, ...

Ok, let me know when you figured it out. Generally, I would be interested what it is, since I thought I understood the program structure. But please keep in mind that I am happy to contribute instead of just requesting features, but I am still doing this in my free time. So, if you already have some plan in your mind and depending on the extent of restruturing, it might be even easier for you to restructure it even directly by your own afterwards, instead of explaining me first. :)

@sspanak
Copy link
Owner

sspanak commented Mar 20, 2023

Actually this is intended since this happens only if a developer (me or someone in future using this method) send a not supported Event, so he will directly note it while testing. During normal usage there is no chance that this will throw an error (if tested at least one time)

There is always a chance for something to go wrong. Never trust Android 😉

As for the unsupported keys, there is no reason to throw an error. Just ignore them. This is a normal scenario, not an error. For example, your remote control has many keys that TT9 does not support, but it only looks for the ones that it knows about and ignores the rest. It's as simple as that.

Explicit usecase is Android TV: There is no Touch-Screen but huge space on screen to show buttons for accessing e.g. settings menu, if one does not remember which long-press shortcut button was defined once weeks ago

May I suggest a different approach? I understand you are trying to make TT9 usable on a TV. But the TV is not a touchscreen device, so I think creating a touchscreen UI, then adapting it for a non-touchscreen device isn't an optimal solution.

You want to see the full-sized keyboard for touchscreens and use it only because of the hotkey hints and that's fine. But why don't we add support for more keys on the remote (volume up/down, next/previous channel and other common ones)? There are plenty of choices there. And then, just like you have labelled each number key with the letters it supports, also label each TT9 function key (the gear, the planet, etc) with the corresponding hardware key? For example, we can display "planet" and below it "volume up" or whatever you have configured.

I think this way it will quite be clear how to access a function and there will be no need to change anything "outside" the softkey scope. Two birds with one stone!

Of course, displaying the extra labels must be a configuration option, not to ruin the experience on small screens.

So, if you already have some plan in your mind and depending on the extent of restruturing, it might be even easier for you to restructure it even directly by your own afterwards, instead of explaining me first. :)

The SoftKeyHandler... I was indeed thinking restructing it, indeed. Sort of like in preferences/screens. Would you mind if I did it in your branch, before merging this?

I am really eager to start using TT9 on my large smartphone (I hate QWERTY 😬), so I say, let's do finish this off! I can join as soon as I deal with the newly reported bugs.

@sspanak
Copy link
Owner

sspanak commented Mar 21, 2023

I have invited you to the repository. I believe, we can work easier if you create a branch here. But if you prefer "the Github" way, it is perfectly fine to ignore the invitation.

@nebkrid nebkrid mentioned this pull request Mar 25, 2023
@nebkrid
Copy link
Contributor Author

nebkrid commented Mar 25, 2023

Thank you. I am happy to join - but since I am still quite new to github you may have to give me a hint how to best work with the different tools. Since I didn't found a way to redirect this pull request directly, I created a new one and close this on.

@nebkrid nebkrid closed this Mar 25, 2023
@nebkrid
Copy link
Contributor Author

nebkrid commented Mar 25, 2023

Oh, maybe closing was too early...
Is there a discussion possible in the new branch? Didn't find it...
So I am answering here to you last comments:

There is always a chance for something to go wrong. Never trust Android 😉

Good point ;) Ok, a Log.e(...) message should be good enough and would not cause any breakdowns. I will change it this way.

For example, we can display "planet" and below it "volume up" or whatever you have configured.

True, I guess this was just too easy for my mind 😂

But why don't we add support for more keys on the remote

Yes, this would be anyway good! I just still didn't get yet how to add new keys, and already thought one step further. Best would be an option to just push the button we want to configure (I saw such an approach in a different app), so no need for pre-defined buttons or scrolling. But I didn't had time yet to dig deeper how to enable this. Maybe with a special preference which listens for key inputs... But since you already know how to add a key and therefore it will be (hopefully) pretty easy for you, it would be great if you could add more. I will try to find out which key events would be useful.

Would you mind if I did it in your branch, before merging this?

Feel free to restructure it :)

@sspanak
Copy link
Owner

sspanak commented Mar 27, 2023

Since I didn't found a way to redirect this pull request directly, I created a new one and close this on.

I don't think it is possible when you had opened a PR from one repo to another. It doesn't matter, just open some pull request, so we can chat there. If I haven't answered or have forgotten about some of your questions, just copy them and I'll do there.

Now, regarding the long press to type a number, this may not be strictly necessary on touchscreen keyboard. From what I saw, all other keyboards, require manually switching to numeric mode to type numbers, which means we are all good in this regard.

@nebkrid nebkrid mentioned this pull request Mar 28, 2023
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.

2 participants