[colemak] Adds English Colemak layout and improves code organization for multiple layouts - #4
Conversation
| Lp3RepeatableKeyboardCallback { | ||
|
|
||
| private val viewModel: EnQwertyLp3KeyboardViewModel<*> by lazy { | ||
| private val viewModel: EnColemakLp3KeyboardViewModel<*> by lazy { |
There was a problem hiding this comment.
I'll of course revert this file back to QWERTY for the PR, but I don't see another way for the code to be told "which layout to use," @dupontgu. I'd expect that and maybe to have a way to "register layouts" with the system as available to pick from.
Let me know if I'm missing something that's already in place?
Otherwise, I assume that's all coming in the future, and just submit the Colemak layout, assuming it will become usable later once layout switching is implemented.
There was a problem hiding this comment.
Leave this for now, I'm gonna think out how we're gonna need to handle label selection.
There was a problem hiding this comment.
Sounds good. Left as is.
There was a problem hiding this comment.
Ok I'm going to open another PR for selecting layout soon. Sorry to flip-flop, but for the sake of keeping a sensible default on main would you mind reverting this to EnQwerty before merging?
There was a problem hiding this comment.
"Sensible default"?! I am offended ;-) kidding.
Reverted :-)
| if (characters.length == 9) { | ||
| // currently this row only has 9 or 10 chars, so add space to left-align if only 9 | ||
| Spacer(Modifier.width(STANDARD_KEY_WIDTH_DP.dp)) | ||
| } |
There was a problem hiding this comment.
Seems fine for now and follows existing pattern below, but brittle. We could also do something like this @dupontgu :
for (char in characters) {
if (char == ' ') {
Spacer(Modifier.width(STANDARD_KEY_WIDTH_DP.dp))
} else {
Key(char, callback, swipeConfig, enableKeyAnimation)
}
}
There was a problem hiding this comment.
Oh good callout. I actually think maybe we change the characters param from String to List<Char?> and use null to signify a non-key rather than blank space. Minor change everywhere else -> instead of passing "qwerty", pass "qwerty".toList() and in this case "firstrow".toList() + null
There was a problem hiding this comment.
Since I added '/" key, I removed this bit as it isn't necessary.
I initially thought "it'd be nice to specify the exact spacing" or "could we take a TextAlign param" ... but then noticed the backspace button shifts a little when going into symbol layout. So it's probably good to spruce up the horizontal layout and padding logic, but probably a larger refactor for another PR.
dupontgu
left a comment
There was a problem hiding this comment.
Great Q's, I think you've got it!
One question from me - it seems like other mobile colemak keyboards throw the semi-colon in the first row above 'o'. Any reason not to do that here too?
|
@bunnyfly no rush on my end! And thanks for that. I trust your gut on what should go there (even if it's nothing) |
There was a problem hiding this comment.
Alright! Ready for review, @dupontgu 🙏 I did almost all the comments/suggestions except for making the layout characters nullable (see comment for why).
For whether and what to put in the empty spot...I wanted to make an informed decision...
The tl;dr:
' and " shifted are the best! Data supports ' as one of the most common symbols, and after trialing the options, it feels more natural vs ? or !. I'd probably recommend also using ' for other layouts that have "extra space" (AZERTY on the bottom row)...and MAAAYBE even QWERTY on the middle...? But that can be for another day.
Read on for the tedious details if you care:
My thinking:
- The unshifted/shifted pair should make sense together (e.g.
'/"or!/?...as opposed to'/?which don't feel related) - While it's nice to optimize for both symbols being high-frequency, the shifted symbol is much less important, because hitting shift to get to it is no easier than hitting the "symbol" button.
- I wanted some data! While not truly rigorous, I pulled and cleaned up ~40,000 SMS messages and the most common symbols in order are
.!'?,:)-". - Since
.is more commonly "double-spaced," I ignored it and it's "pair" symbol,which feels important but less linguistically useful, and lower in frequency than other symbols. - That leaves
!?'"as top choices. - Characters like
?and!are most often at the end of a line; while'is often in the middle of a word (e.g. it's), which makes'more akin to "a word character" that should be prioritized. Hitting "symbol-key" for'feels relatively distracting and disorienting mid-word. It feels more satisfying and "final" to hit "symbol-key" for!or?at the end of a text, when you're already out of the typing flow and mentally preparing to do non-typing tasks (hitting send, correcting spelling, etc). !or?feel more like "human expression!" While'feels more focused on "proper writing." Which makes the former seem more fun, but is also more visually and emotionally distracting!- I said the shifted char is not important...BUT, if it happens to be
", that's a nice coincidence because when you first start typing, the keyboard will automatically be in shift-mode. And"happens to be the one symbol that's often typed at the beginning of a line to start a quote. Making starting a text with a quote very intuitive and nice! Feels good to have side-effects like this! - In the end, I actually tried them all out on my physical LP3. Indeed,
'was the most natural, least frustrating, and visually disappeared the most. It felt really delightful and a relief to not have to hop into the symbol layer for apostrophes mid-word.
Just as a visual comparison, here are the options and below the character frequency chart and how I did my analysis:
(Symbol frequency came from ~40,000 example text messages. I took an online corpus of English SMS texts and ~10 years of my own SMS history from a Samsung and LP3. I removed my own sent messages from the data (to avoid my own typing patterns overwhelming). And I did basic cleanup (removed escaped chars, 2FA codes, automated texts, http links, as many non-human messages as I could, etc) to try to get only human-typed text.)
| Lp3RepeatableKeyboardCallback { | ||
|
|
||
| private val viewModel: EnQwertyLp3KeyboardViewModel<*> by lazy { | ||
| private val viewModel: EnColemakLp3KeyboardViewModel<*> by lazy { |
There was a problem hiding this comment.
Sounds good. Left as is.
| if (characters.length == 9) { | ||
| // currently this row only has 9 or 10 chars, so add space to left-align if only 9 | ||
| Spacer(Modifier.width(STANDARD_KEY_WIDTH_DP.dp)) | ||
| } |
There was a problem hiding this comment.
Since I added '/" key, I removed this bit as it isn't necessary.
I initially thought "it'd be nice to specify the exact spacing" or "could we take a TextAlign param" ... but then noticed the backspace button shifts a little when going into symbol layout. So it's probably good to spruce up the horizontal layout and padding logic, but probably a larger refactor for another PR.
| object EnQwerty { | ||
| object LowerCaseLayout : Layout { | ||
| override val isRootLayout: Boolean | ||
| get() = true |
There was a problem hiding this comment.
Github's diff detection here is really bad. I moved the other layouts below to EnShared, but everything in the EnQwerty object here is just the existing three main rows indented.
| import com.thelightphone.lp3Keyboard.ui.ThirdRow | ||
|
|
||
| /** Layouts and data generally shared across English keyboards. */ | ||
| object EnShared { |
There was a problem hiding this comment.
Everything in this object is just copy-paste-and-indented from EnQwerty
| val initialLayout: Layout, | ||
| val lowerCaseLayout: Layout, | ||
| val upperCaseLayout: Layout, | ||
| val capsLockedLayout: Layout, |
There was a problem hiding this comment.
This class is:
- copy-pasted from EnQwertyViewModel
- renamed to EnBaseViewModel
- abstract
- added these Layout constructor params
- uses these params below instead of hard-coded
EnQwertylayouts
| override val keyboardOptionsFlow: StateFlow<KeyboardOptions> = MutableStateFlow( | ||
| KeyboardOptions( | ||
| defaultEmojis, | ||
| displayReturn = true, | ||
| displayVoice = true, | ||
| enableKeyAnimation = true, | ||
| swipeEnabled = false | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Just calling out that keyboardOptionsFlow here is removed. It's in the EnBaseViewModel but the compiler was complaining about it being an override now that this is extending EnBaseViewModel.
Not sure what or how it's used...so LMK if there's something to be done about this or if it's kind of a placeholder for now.
dupontgu
left a comment
There was a problem hiding this comment.
Some small changes and then this is good to go!! Thanks
| Lp3RepeatableKeyboardCallback { | ||
|
|
||
| private val viewModel: EnQwertyLp3KeyboardViewModel<*> by lazy { | ||
| private val viewModel: EnColemakLp3KeyboardViewModel<*> by lazy { |
There was a problem hiding this comment.
Ok I'm going to open another PR for selecting layout soon. Sorry to flip-flop, but for the sake of keeping a sensible default on main would you mind reverting this to EnQwerty before merging?
| import com.thelightphone.lp3Keyboard.ui.layout.Layout | ||
|
|
||
| class EnColemakLp3KeyboardViewModel<SwipeResult>( | ||
| private val passedCallback: Lp3RepeatableKeyboardCallback, |
There was a problem hiding this comment.
we can get rid of each of the private vals here. Since they are not being used for anything other than passing to the base constructor, there's no need to keep a handle on them.
There was a problem hiding this comment.
Sorry, not sure about this one, can you clarify @dupontgu ?
I believe the base constructor (currently) requires these params. So to remove them from the Qwerty and Colemak concrete classes, we'd need to either pass defaults (e.g. in line 17 here) or declare defaults in the base constructor, right?
There was a problem hiding this comment.
oh I mean just remove the keywords private and val. Those would create an extra field for those values available only for use in EnColemakViewModel, but we have no actual code unique to that class.
So it should look like:
class EnColemakLp3KeyboardViewModel<SwipeResult>(
passedCallback: Lp3RepeatableKeyboardCallback,
swipeCallback: Lp3KeyboardSwipeCallback<SwipeResult>,
haptic: () -> Unit = {},
optionsForLayout: (Layout) -> LayoutOptions = {
LayoutOptions(
displayCloseButton = true
)
},
) ...There was a problem hiding this comment.
Oh ty, that makes sense. Done!
| lowerCaseLayout = EnColemak.LowerCaseLayout, | ||
| upperCaseLayout = EnColemak.UpperCaseLayout, | ||
| capsLockedLayout = EnColemak.CapsLockedLayout, | ||
| ) { } |
There was a problem hiding this comment.
No need to keep { } for a blank class
| lowerCaseLayout = EnQwerty.LowerCaseLayout, | ||
| upperCaseLayout = EnQwerty.UpperCaseLayout, | ||
| capsLockedLayout = EnQwerty.CapsLockedLayout, | ||
| ) { } |
There was a problem hiding this comment.
same - no brackets necessary!
| import kotlinx.coroutines.launch | ||
|
|
||
| class EnQwertyLp3KeyboardViewModel<SwipeResult>( | ||
| private val passedCallback: Lp3RepeatableKeyboardCallback, |
There was a problem hiding this comment.
same - no need for private vals
| lowerCaseLayout = EnColemak.LowerCaseLayout, | ||
| upperCaseLayout = EnColemak.UpperCaseLayout, | ||
| capsLockedLayout = EnColemak.CapsLockedLayout, | ||
| ) { } |
| lowerCaseLayout = EnQwerty.LowerCaseLayout, | ||
| upperCaseLayout = EnQwerty.UpperCaseLayout, | ||
| capsLockedLayout = EnQwerty.CapsLockedLayout, | ||
| ) { } |
| Lp3RepeatableKeyboardCallback { | ||
|
|
||
| private val viewModel: EnQwertyLp3KeyboardViewModel<*> by lazy { | ||
| private val viewModel: EnColemakLp3KeyboardViewModel<*> by lazy { |
There was a problem hiding this comment.
"Sensible default"?! I am offended ;-) kidding.
Reverted :-)
| import com.thelightphone.lp3Keyboard.ui.layout.Layout | ||
|
|
||
| class EnColemakLp3KeyboardViewModel<SwipeResult>( | ||
| private val passedCallback: Lp3RepeatableKeyboardCallback, |
There was a problem hiding this comment.
Oh ty, that makes sense. Done!
| import kotlinx.coroutines.launch | ||
|
|
||
| class EnQwertyLp3KeyboardViewModel<SwipeResult>( | ||
| private val passedCallback: Lp3RepeatableKeyboardCallback, |
dupontgu
left a comment
There was a problem hiding this comment.
Thank you!!! The team is thrilled to have an alt layout (and hopefully more soon). Cooking a way to switch between them now.
|
Yay! And thank you! <3 |

Adds English Colemak keyboard layout; and as the first non-QWERTY layout, refactors some of the code to make multiple layouts cleaner.
See #3 for background and proposal.
The layout is not yet selectable, but this makes it available for when the layout selection feature is implemented.
Since Colemak has 1 less first-row-key and 1 more second-row-key vs QWERTY, the first row is shifted to be left-aligned to keep more visually consistent with physical keyboard and avoid an awkward visual skew. This leaves an "empty" space at the top right. On Android's Gboard, it's used for
;, which is what's physically there on a keyboard, but obviously that's a little-used key on mobile. This uses that for'(and"shifted) as one of the most common characters typed.?/!would be another equally useful choice.Lower, upper, caps:
