-
Notifications
You must be signed in to change notification settings - Fork 231
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
Emoji Picker #415
Emoji Picker #415
Conversation
Merging emoji branch to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great! Thanks a ton for this. Just a few fixes / suggestions.
app/src/main/java/com/dessalines/thumbkey/keyboards/CommonKeys.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/keyboards/CommonKeys.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/keyboards/CommonKeys.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/keyboards/CommonKeys.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Outdated
Show resolved
Hide resolved
backgroundColor = ColorVariant.SURFACE_VARIANT, | ||
) | ||
|
||
val controllerKeys = listOf(emojiBackKey, NUMERIC_KEY_ITEM, RETURN_KEY_ITEM, BACKSPACE_KEY_ITEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of the return and backspace need to be switched.
app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardScreen.kt
Show resolved
Hide resolved
I don't think there is... it looks verbose, but there's no way around passing those required params.
That's up to those maintainers to upkeep, not you or me. Just replacing the settings key with the new emoji one should be enough for this, and if they'd like to fix / remove some things, they can do so at a later time. |
…r changes for light/dark theme.
There we go, I've done all the changes you mentioned, although I kept the logging line, because there's one for the numeric mode and shift mode. We should either keep all of them or remove all of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested and it works great, in dark mode also. Thanks a ton for this, ppl are going to really love it.
There's a single git commit for incrementing the indentation by 4 spaces, because I have put an if/else clause in KeyboardScreen.kt. The indentation messed up the git diff and it was hard to see what code I had changed, so I fixed the indentation as a separate commit.
It's worth mentioning, because new PRs might have issues if they're adding commits to the old KeyboardScreen.kt file.
For undo/redo, I tried to make an object KeyActions.Undo, and put the code there, but it just doesn't work, so I have done it like this in the CommonKeys.kt file
action = KeyAction.SendEvent( KeyEvent(/* downTime= */ 0, /* eventTime= */ 0, KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_Z, /* repeat= */ 0, /* metaState= */ KeyEvent.META_CTRL_ON)
In KeyboardScreen.kt I have duplicate code for the massive function call KeyboardKey(). I couldn't figure out a better way to do it.
Broken Keyboards:
Keyboards that don't work with the emoji key, and so may be missing the ability to change layouts, because I removed that option from the number pad key and put it on the emoji key.
I don't use these layouts, so I'm unsure what the best way to add emoji to them. We could add a swipe option, or we could add the emoji key. Any recommendations? I would love some input from someone :).