make "hide letters" only hide letters, add new toggle "hide symbols" (fixes #299) #370

nejni-marji · 2023-08-30T20:28:44Z

I think this PR is set up correctly this time, and I already figured out how to use the linter, so it should be pretty much good.

nejni-marji · 2023-08-30T20:31:39Z

Okay, I have no idea what I've done wrong here, ugh. I don't know why the CI is upset about this.

edit: I guess it's fine now?

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt

dessalines · 2023-08-31T15:31:50Z

app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardKey.kt

@@ -363,7 +364,12 @@ fun KeyText(key: KeyC, keySize: Dp, hideLetters: Boolean, capsLock: Boolean) {
        }
        is KeyDisplay.TextDisplay -> {
            // Only  hide the letters for text, not symbols
-            if (!hideLetters) {
+            val showKey = if (!display.text.any { !it.isLetter() }) {


What's going on here?

This isn't going to work for things like multiple letter keys, etc. You're going to need to add a KeyType enum {Letter, Symbol} to each key, probably with a default of KeyType.Letter, and alter every keyboard to set that.

I'm using isLetter() on every character in the string. For example: "a" is a letter, "abc" is a letter, and "?" is a symbol, but "a?" is also a symbol. My assumption is that anything that is purely letter-characters is probably basically a letter.

Okay, but I'd move the isLetter check above, and have this do a simple if (!hideLetters && !isMadeOfLetters)

Because I made hiding symbols a separate toggle, I found that I have to use the isLetter check to determine which of hideLetters and hideSymbols to apply. I tried a bunch of logic like what you're describing in a python script and none of my attempts correctly resolved every possibility.

So you haven't tested this code in thumbkey?

Currently, .com is hidden with hideSymbols. I'm not sure if I necessarily intended that when I initially wrote it, but in a way it feels like the behaviour you would expect? Obviously, this is a particular edge case that could go either way, and it's a reason to potentially explicitly label each key as a symbol or not a symbol, but I think it's probably fine.

Bad news: it does cause issues with the Greek keyboard, because combining accents are not letters:

thumb-key/app/src/main/java/com/dessalines/thumbkey/keyboards/ThumbKeyGRv1.kt

Lines 294 to 295 in 7134891

display = KeyDisplay.TextDisplay("Ϋ́"),

action = KeyAction.CommitText("Ϋ́"),

thumb-key/app/src/main/java/com/dessalines/thumbkey/keyboards/ThumbKeyGRv1.kt

Lines 320 to 321 in 7134891

display = KeyDisplay.TextDisplay("Ϊ́"),

action = KeyAction.CommitText("Ϊ́"),

Hrm... Could you check if there exists any other kotlin / java function would work? Might be something better than isLetter

If not, even if imperfect, this would probably still be better than just hiding everything as it currently does.

There's no better single function to my knowledge. It's possible to compare the character's Unicode category, and check if it's one of the 3 "mark" categories. I'm not 100% what all is in the mark categories overall, but punctuation and other special symbols generally shouldn't be.

dessalines

Actually DB migrations are still broken.

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt

dessalines · 2023-09-01T13:26:17Z

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt

+val MIGRATION_7_8 = object : Migration(7, 8) {
+    override fun migrate(database: SupportSQLiteDatabase) {
+        database.execSQL(
+            "alter table AppSettings add column hide_symbols BOOLEAN NOT NULL default $DEFAULT_HIDE_SYMBOLS",


Whew, good thing I tested this, because it breaks existing installations. This needed to be an INTEGER, not a boolean. I'm fixing it now.

dessalines · 2023-09-01T13:41:54Z

Okay I fixed this up. In the future please actually test this. If you did so, you'd see that it would cause an immediate crash on startup, because sqlite doesn't have a boolean type.

dessalines · 2023-09-01T13:42:33Z

app/src/main/java/com/dessalines/thumbkey/ui/components/keyboard/KeyboardKey.kt

@@ -363,7 +364,15 @@ fun KeyText(key: KeyC, keySize: Dp, hideLetters: Boolean, capsLock: Boolean) {
        }
        is KeyDisplay.TextDisplay -> {
            // Only  hide the letters for text, not symbols
-            if (!hideLetters) {
+            val containsLetters = display.text.any { it.isLetter() }


I fixed this to be clearer, and not use a triple negative

This actually changes the logic, but after thinking it over, I do agree that this is probably the better of the two choices here anyway.

nejni-marji added 2 commits August 30, 2023 13:07

make "hide letters" hide only letters (dessalines#299)

0570d82

add "hide symbols" setting (dessalines#229)

1c1bdc6

nejni-marji requested a review from dessalines as a code owner August 30, 2023 20:28

refactor hideLetters/hideSymbols check

7d3ba2c

dessalines requested changes Aug 31, 2023

View reviewed changes

correctly implement DB migration for hide_symbols

64b0bcc

dessalines reviewed Aug 31, 2023

View reviewed changes

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt Show resolved Hide resolved

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt Show resolved Hide resolved

actually use DB migration for hide_symbols

d5788df

dessalines reviewed Aug 31, 2023

View reviewed changes

app/src/main/java/com/dessalines/thumbkey/db/AppDb.kt Show resolved Hide resolved

Merge branch 'main' into hide_letters

ac5a84a

dessalines reviewed Sep 1, 2023

View reviewed changes

Removed double negatives, fixed app DB.

84f34f1

dessalines reviewed Sep 1, 2023

View reviewed changes

dessalines approved these changes Sep 1, 2023

View reviewed changes

dessalines enabled auto-merge (squash) September 1, 2023 13:42

dessalines merged commit b2be8ee into dessalines:main Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make "hide letters" only hide letters, add new toggle "hide symbols" (fixes #299) #370

make "hide letters" only hide letters, add new toggle "hide symbols" (fixes #299) #370

nejni-marji commented Aug 30, 2023

nejni-marji commented Aug 30, 2023 •

edited

Loading

dessalines Aug 31, 2023

nejni-marji Aug 31, 2023

dessalines Aug 31, 2023 •

edited

Loading

nejni-marji Aug 31, 2023

dessalines Aug 31, 2023

nejni-marji Aug 31, 2023

nejni-marji Aug 31, 2023 •

edited

Loading

dessalines Aug 31, 2023

dessalines Aug 31, 2023

nejni-marji Aug 31, 2023

dessalines left a comment

dessalines Sep 1, 2023

dessalines commented Sep 1, 2023

dessalines Sep 1, 2023

nejni-marji Sep 1, 2023

	display = KeyDisplay.TextDisplay("Ϋ́"),
	action = KeyAction.CommitText("Ϋ́"),

	display = KeyDisplay.TextDisplay("Ϊ́"),
	action = KeyAction.CommitText("Ϊ́"),

make "hide letters" only hide letters, add new toggle "hide symbols" (fixes #299) #370

make "hide letters" only hide letters, add new toggle "hide symbols" (fixes #299) #370

Conversation

nejni-marji commented Aug 30, 2023

nejni-marji commented Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nejni-marji Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nejni-marji commented Aug 30, 2023 •

edited

Loading

dessalines Aug 31, 2023 •

edited

Loading

nejni-marji Aug 31, 2023 •

edited

Loading