improve popup key handling and update tests

remove outdated part from layouts.md
fixes #883

app/src/main/java/helium314/keyboard/keyboard/Key.java

@@ -1130,7 +1130,7 @@ public class Key implements Comparable<Key> {
                         : hintLabel;
             }
 
-            String outputText = KeySpecParser.getOutputText(keySpec);
+            String outputText = KeySpecParser.getOutputText(keySpec, code);
             if (needsToUpcase) {
                 outputText = StringUtils.toTitleCaseOfKeyLabel(outputText, localeForUpcasing);
             }

app/src/main/java/helium314/keyboard/keyboard/internal/KeySpecParser.java

@@ -150,7 +150,7 @@ public final class KeySpecParser {
     }
 
     @Nullable
-    public static String getOutputText(@Nullable final String keySpec) {
+    public static String getOutputText(@Nullable final String keySpec, final int code) {
         if (keySpec == null) {
             // TODO: Throw {@link KeySpecParserError} once Key.keyLabel attribute becomes mandatory.
             return null;
@@ -170,7 +170,9 @@ public final class KeySpecParser {
             return outputText;
         }
         final String label = getLabel(keySpec);
-        if (label == null && DebugFlags.DEBUG_ENABLED) {
+        if (label == null) {
+            if (keySpec.startsWith(KeyboardIconsSet.PREFIX_ICON) && code != KeyCode.UNSPECIFIED && code != KeyCode.MULTIPLE_CODE_POINTS)
+                return null; // allow empty label in case of icon & actual code
             throw new KeySpecParserError("Empty label: " + keySpec);
         }
         // Code is automatically generated for one letter label. See {@link getCode()}.

app/src/main/java/helium314/keyboard/keyboard/internal/PopupKeySpec.java

@@ -60,7 +60,7 @@ public final class PopupKeySpec {
             mOutputText = mLabel;
         } else {
             mCode = code;
-            final String outputText = KeySpecParser.getOutputText(popupKeySpec);
+            final String outputText = KeySpecParser.getOutputText(popupKeySpec, code);
             mOutputText = needsToUpperCase
                     ? StringUtils.toTitleCaseOfKeyLabel(outputText, locale) : outputText;
         }

app/src/main/java/helium314/keyboard/keyboard/internal/keyboard_parser/floris/TextKeyData.kt

@@ -12,6 +12,7 @@ import kotlinx.serialization.Transient
 import helium314.keyboard.keyboard.Key
 import helium314.keyboard.keyboard.KeyboardId
 import helium314.keyboard.keyboard.KeyboardTheme
+import helium314.keyboard.keyboard.internal.KeySpecParser
 import helium314.keyboard.keyboard.internal.KeyboardIconsSet
 import helium314.keyboard.keyboard.internal.KeyboardParams
 import helium314.keyboard.keyboard.internal.keyboard_parser.floris.KeyCode.checkAndConvertCode
@@ -303,13 +304,21 @@ sealed interface KeyData : AbstractKeyData {
             if (newLabel.endsWith("|")) return "${newLabel}!code/$newCode" // for toolbar keys
             return if (newCode == code) newLabel else "${newLabel}|!code/$newCode"
         }
-        if (code >= 32)
+        if (code >= 32) {
-            return "${newLabel}|${StringUtils.newSingleCodePointString(code)}"
+            if (newLabel.startsWith(KeyboardIconsSet.PREFIX_ICON)) {
+                // we ignore everything after the first |
+                // todo (later): for now this is fine, but it should rather be done when creating the popup key,
+                //  and it should be consistent with other popups and also with normal keys
+                return "${newLabel.substringBefore("|")}|${StringUtils.newSingleCodePointString(code)}"
+            }
+            return "$newLabel|${StringUtils.newSingleCodePointString(code)}"
+
+        }
         if (code in KeyCode.Spec.CURRENCY) {
             return getCurrencyLabel(params)
         }
-        return if (newLabel.endsWith("|")) "${newLabel}!code/${processCode()}" // for toolbar keys
+        return if (newLabel.endsWith("|")) "$newLabel!code/${processCode()}" // for toolbar keys
-        else "${newLabel}|!code/${processCode()}"
+        else "$newLabel|!code/${processCode()}"
     }
 
     fun getCurrencyLabel(params: KeyboardParams): String {

app/src/main/java/helium314/keyboard/latin/PunctuationSuggestions.java

@@ -64,7 +64,7 @@ public final class PunctuationSuggestions extends SuggestedWords {
         final String keySpec = super.getWord(index);
         final int code = KeySpecParser.getCode(keySpec);
         return (code == KeyCode.MULTIPLE_CODE_POINTS)
-                ? KeySpecParser.getOutputText(keySpec)
+                ? KeySpecParser.getOutputText(keySpec, code)
                 : StringUtils.newSingleCodePointString(code);
     }
 

app/src/main/java/helium314/keyboard/latin/utils/CustomLayoutUtils.kt

@@ -92,7 +92,7 @@ private fun checkLayout(layoutContent: String, context: Context): Boolean? {
             return null
         return false
     } catch (e: Exception) { Log.w(TAG, "error parsing custom simple layout", e) }
-    if (layoutContent.startsWith("[")) {
+    if (layoutContent.trimStart().startsWith("[")) {
         // layout can't be loaded, assume it's json -> load json layout again because the error message shown to the user is from the most recent error
         try {
             RawKeyboardParser.parseJsonString(layoutContent).map { row -> row.mapNotNull { it.compute(params)?.toKeyParams(params) } }
@@ -101,7 +101,7 @@ private fun checkLayout(layoutContent: String, context: Context): Boolean? {
     return null
 }
 
-private fun checkKeys(keys: List<List<Key.KeyParams>>): Boolean {
+fun checkKeys(keys: List<List<Key.KeyParams>>): Boolean {
     if (keys.isEmpty() || keys.any { it.isEmpty() }) {
         Log.w(TAG, "empty rows")
         return false

app/src/test/java/helium314/keyboard/KeyboardParserTest.kt

@@ -8,6 +8,7 @@ import helium314.keyboard.keyboard.Key.KeyParams
 import helium314.keyboard.keyboard.Keyboard
 import helium314.keyboard.keyboard.KeyboardId
 import helium314.keyboard.keyboard.KeyboardLayoutSet
+import helium314.keyboard.keyboard.internal.KeySpecParser
 import helium314.keyboard.keyboard.internal.KeyboardBuilder
 import helium314.keyboard.keyboard.internal.KeyboardParams
 import helium314.keyboard.keyboard.internal.TouchPositionCorrection
@@ -20,7 +21,10 @@ import helium314.keyboard.latin.LatinIME
 import helium314.keyboard.latin.RichInputMethodSubtype
 import helium314.keyboard.latin.utils.AdditionalSubtypeUtils.createEmojiCapableAdditionalSubtype
 import helium314.keyboard.latin.utils.POPUP_KEYS_LAYOUT
+import helium314.keyboard.latin.utils.checkKeys
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertThrows
+import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -39,24 +43,22 @@ import java.util.Locale
 ])
 class ParserTest {
     private lateinit var latinIME: LatinIME
+    private lateinit var params: KeyboardParams
 
     @Before
     fun setUp() {
         latinIME = Robolectric.setupService(LatinIME::class.java)
         ShadowLog.setupLogging()
         ShadowLog.stream = System.out
+        params = KeyboardParams()
+        params.mId = KeyboardLayoutSet.getFakeKeyboardId(KeyboardId.ELEMENT_ALPHABET)
+        params.mPopupKeyTypes.add(POPUP_KEYS_LAYOUT)
+        addLocaleKeyTextsToParams(latinIME, params, POPUP_KEYS_NORMAL)
     }
 
-    // todo: add more tests
+    // todo: add tests for background type, also consider e.g. emoji key has functional bg by default
-    //  (popup) keys with label and code
-    //  (popup) keys with icon
-    //  (popup) keys with that are essentially toolbar keys (yes, this should work at some point!)
-    //  correct background type, depending on key type and maybe sth else
 
     @Test fun simpleParser() {
-        val params = KeyboardParams()
-        params.mId = KeyboardLayoutSet.getFakeKeyboardId(KeyboardId.ELEMENT_ALPHABET)
-        addLocaleKeyTextsToParams(latinIME, params, POPUP_KEYS_NORMAL)
         val layoutStrings = listOf(
 """
 a
@@ -122,51 +124,109 @@ f""", // no newline at the end
         }
     }
 
-    @Test fun jsonParser() {
+    @Test fun simpleKey() {
-        val params = KeyboardParams()
+        assertIsExpected("""[[{ "$": "auto_text_key" "label": "a" }]]""", Expected('a'.code, "a"))
-        params.mId = KeyboardLayoutSet.getFakeKeyboardId(KeyboardId.ELEMENT_ALPHABET)
+        assertIsExpected("""[[{ "$": "text_key" "label": "a" }]]""", Expected('a'.code, "a"))
-        params.mPopupKeyTypes.add(POPUP_KEYS_LAYOUT)
+        assertIsExpected("""[[{ "label": "a" }]]""", Expected('a'.code, "a"))
-        addLocaleKeyTextsToParams(latinIME, params, POPUP_KEYS_NORMAL)
+    }
-        data class Expected(val label: String?, val icon: String?, val text: String?, val code: Int, val popups: List<Pair<String, Int>>? = null)
+
-        val expected = listOf(
+    @Test fun labelAndExplicitCode() {
-            Expected("a", null, null, 'a'.code, null),
+        assertIsExpected("""[[{ "$": "text_key" "label": "a", "code": 98 }]]""", Expected('b'.code, "a"))
-            Expected("a", null, null, 'a'.code, null),
+    }
-            Expected("a", null, null, 'b'.code, listOf("b" to 'a'.code)),
+
-            Expected("$", null, null, '$'.code, listOf("£", "€", "¢", "¥", "₱").map { it to it.first().code }),
+    @Test fun labelAndImplicitCode() {
-            Expected("$", null, null, '¥'.code, listOf("£", "€", "¢", "¥", "₱").map { it to it.first().code }),
+        assertIsExpected("""[[{ "$": "text_key" "label": "a|b" }]]""", Expected('b'.code, "a"))
-            Expected("i", null, null, 105, null),
+    }
-            Expected("্র", null, "্র", KeyCode.MULTIPLE_CODE_POINTS, null),
+
-            Expected("x", null, "্র", KeyCode.MULTIPLE_CODE_POINTS, null),
+    @Test fun labelAndImplicitText() {
-            Expected(";", null, null, ';'.code, listOf(":").map { it to it.first().code }),
+        assertIsExpected("""[[{ "$": "text_key" "label": "a|bb" }]]""", Expected(KeyCode.MULTIPLE_CODE_POINTS, "a", text = "bb"))
-            Expected(".", null, null, '.'.code, listOf(">").map { it to it.first().code }),
+        // todo: should this actually work?
-            Expected("'", null, null, '\''.code, listOf("!", "\"").map { it to it.first().code }),
+        assertIsExpected("""[[{ "$": "text_key" "label": "a|" }]]""", Expected(KeyCode.MULTIPLE_CODE_POINTS, "a", text = ""))
-            Expected("9", null, null, '9'.code, null), // todo (later): also should have different background or whatever is related to type
+    }
-            Expected(null, "delete_key", null, -7, null),
+
-            Expected("?123", null, "?123", -202, null),
+    @Test fun labelAndImplicitAndExplicitCode() { // explicit code overrides implicit code
-            Expected(null, "space_key", null, ' '.code, null),
+        assertIsExpected("""[[{ "code": 32, "label": "a|b" }]]""", Expected(' '.code, "a"))
-            Expected("(", null, null, '('.code, listOf("<", "[", "{").map { it to it.first().code }),
+        assertIsExpected("""[[{ "code": 32, "label": "a|!code/key_delete" }]]""", Expected(' '.code, "a"))
-            Expected("$", null, null, '$'.code, listOf("£" to '£'.code, "₱" to '₱'.code, "€" to '€'.code, "¢" to '¢'.code, "¥" to '¥'.code, "¥" to '€'.code)),
+        // todo: should text be null? it's not used at all (it could be, but it really should not)
-            Expected("a", null, null, ' '.code, null),
+        assertIsExpected("""[[{ "code": 32, "label": "a|bb" }]]""", Expected(' '.code, "a", text = "bb"))
-            Expected("a", null, null, ' '.code, null),
+    }
-            Expected(null, "clipboard_action_key", null, KeyCode.CLIPBOARD, null),
+
-            Expected(null, "clipboard_action_key", null, KeyCode.MULTIPLE_CODE_POINTS, null), // todo: this works here, but crashes on phone
+    @Test fun keyWithIconAndExplicitCode() {
-            Expected("p", null, null, 'p'.code, listOf("$" to '$'.code)),
+        assertIsExpected("""[[{ "label": "!icon/clipboard", "code": 55 }]]""", Expected(55, icon = "clipboard"))
+    }
+
+    @Test fun keyWithIconAndImplicitCode() {
+        assertIsExpected("""[[{ "label": "!icon/clipboard_action_key|!code/key_clipboard" }]]""", Expected(KeyCode.CLIPBOARD, icon = "clipboard_action_key"))
+    }
+
+    @Test fun popupKeyWithIconAndExplicitCode() {
+        assertIsExpected("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key", "code": 32 }
+      ]
+    } }]]""", Expected('a'.code, "a", popups = listOf(null to ' '.code)))
+    }
+
+    @Test fun popupKeyWithIconAndExplicitAndImplicitCode() {
+        assertIsExpected("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|", "code": 32 }
+      ]
+    } }]]""", Expected('a'.code, "a", popups = listOf(null to ' '.code)))
+        assertIsExpected("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|abc", "code": 32 }
+      ]
+    } }]]""", Expected('a'.code, "a", popups = listOf(null to ' '.code)))
+    }
+
+    @Test fun labelAndImplicitCodeForPopup() {
+        assertIsExpected("""[[{ "$": "text_key" "label": "a|b", "popup": { "main": { "label": "b|a" } } }]]""", Expected('b'.code, "a", popups = listOf("b" to 'a'.code)))
+        assertIsExpected("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|" }
+      ]
+    } }]]""", Expected('a'.code, "a",
+            popups = listOf(null to KeyCode.MULTIPLE_CODE_POINTS))
         )
-        val layoutString = """
+    }
-[
+
-  [
+    @Test fun `| works`() {
-    { "$": "auto_text_key" "label": "a" },
+        assertIsExpected("""[[{ "label": "|", "popup": { "main": { "label": "|" } } }]]""", Expected('|'.code, "|", popups = listOf("|" to '|'.code)))
-    { "$": "text_key" "label": "a" },
+    }
-    { "$": "text_key" "label": "a|b", "popup": { "main": { "label": "b|a" } } },
+
-    { "label": "$$$" },
+    @Test fun currencyKey() {
-    { "label": "$$$", code: -805 },
+        assertIsExpected("""[[{ "label": "$$$" }]]""", Expected('$'.code, "$", popups = listOf("£", "€", "¢", "¥", "₱").map { it to it.first().code }))
-    { "$": "case_selector",
+    }
+
+    @Test fun currencyKeyWithOtherCurrencyCode() {
+        assertIsExpected("""[[{ "label": "$$$", code: -805 }]]""", Expected('¥'.code, "$", popups = listOf("£", "€", "¢", "¥", "₱").map { it to it.first().code }))
+    }
+
+    @Test fun currencyPopup() {
+        assertIsExpected("""[[{ "label": "p", "popup": { "main": { "label": "$$$" } } }]]""", Expected('p'.code, "p", null, null, listOf("$" to '$'.code)))
+        assertIsExpected("""[[{ "label": "p", "popup": { "main": { "label": "a", "code": -804 } } }]]""", Expected('p'.code, "p", null, null, listOf("a" to '€'.code)))
+        assertIsExpected("""[[{ "label": "p", "popup": { "main": { "label": "!icon/clipboard_action_key", "code": -804 } } }]]""", Expected('p'.code, "p", null, null, listOf(null to '€'.code)))
+    }
+
+    @Test fun weirdCurrencyKey() {
+        assertIsExpected("""[[{ "code": -801, "label": "currency_slot_1", "popup": {
+      "main": { "code": -802, "label": "currency_slot_2" },
+      "relevant": [
+        { "code": -806, "label": "currency_slot_6" },
+        { "code": -803, "label": "currency_slot_3" },
+        { "code": -804, "label": "currency_slot_4" },
+        { "code": -805, "label": "currency_slot_5" },
+        { "code": -804, "label": "$$$4" }
+      ]
+    } }]]""", Expected('$'.code, "$", popups = listOf("£" to '£'.code, "₱" to '₱'.code, "€" to '€'.code, "¢" to '¢'.code, "¥" to '¥'.code, "¥" to '€'.code)))
+    }
+
+    @Test fun caseSelector() {
+        assertIsExpected("""[[{ "$": "case_selector",
       "lower": { "code":  105, "label": "i" },
       "upper": { "code":  304, "label": "İ" }
-    },
+    }]]""", Expected(105, "i"))
-    { "$": "multi_text_key", "codePoints": [2509, 2480], "label": "্র" },
+    }
-    { "$": "multi_text_key", "codePoints": [2509, 2480], "label": "x" },
+
-    { "$": "case_selector",
+    @Test fun caseSelectorWithPopup() {
+        assertIsExpected("""[[{ "$": "case_selector",
       "lower": { "code":   59, "label": ";", "popup": {
         "relevant": [
           { "code":   58, "label": ":" }
@@ -177,8 +237,11 @@ f""", // no newline at the end
           { "code":   59, "label": ";" }
         ]
       } }
-    },
+    }]]""", Expected(';'.code, ";", popups = listOf(":").map { it to it.first().code }))
-    { "$": "shift_state_selector",
+    }
+
+    @Test fun shiftSelector() {
+        assertIsExpected("""[[{ "$": "shift_state_selector",
       "shiftedManual": { "code":   62, "label": ">", "popup": {
         "relevant": [
           { "code":   46, "label": "." }
@@ -189,8 +252,11 @@ f""", // no newline at the end
           { "code":   62, "label": ">" }
         ]
       } }
-    },
+    }]]""", Expected('.'.code, ".", popups = listOf(">").map { it to it.first().code }))
-    { "$": "shift_state_selector",
+    }
+
+    @Test fun nestedSelectors() {
+        assertIsExpected("""[[{ "$": "shift_state_selector",
       "shiftedManual": { "code":   34, "label": "\"", "popup": {
         "relevant": [
           { "code":   33, "label": "!" },
@@ -207,12 +273,11 @@ f""", // no newline at the end
           ]
         } }
       }
-    },
+    }]]""", Expected('\''.code, "'", popups = listOf("!", "\"").map { it to it.first().code }))
-    { "code":   57, "label": "9", "type": "numeric" },
+    }
-    { "code":   -7, "label": "delete", "type": "enter_editing" },
+
-    { "code": -207, "label": "view_phone2", "type": "system_gui" },
+    @Test fun layoutDirectionSelector() {
-    { "code":   32, "label": "space" },
+        assertIsExpected("""[[{ "$": "layout_direction_selector",
-    { "$": "layout_direction_selector",
       "ltr": { "code":   40, "label": "(", "popup": {
         "main": { "code":   60, "label": "<" },
         "relevant": [
@@ -227,70 +292,129 @@ f""", // no newline at the end
           { "code":  125, "label": "{" }
         ]
       } }
-    },
+    }]]""", Expected('('.code, "(", popups = listOf("<", "[", "{").map { it to it.first().code }))
-    { "code": -801, "label": "currency_slot_1", "popup": {
+    }
-      "main": { "code": -802, "label": "currency_slot_2" },
+    
-      "relevant": [
+    @Test fun autoMultiTextKey() {
-        { "code": -806, "label": "currency_slot_6" },
+        assertIsExpected("""[[{ "label": "্র" }]]""", Expected(KeyCode.MULTIPLE_CODE_POINTS, "্র", text = "্র"))
-        { "code": -803, "label": "currency_slot_3" },
+    }
-        { "code": -804, "label": "currency_slot_4" },
+
-        { "code": -805, "label": "currency_slot_5" },
+    @Test fun multiTextKey() { // pointless without codepoints!
-        { "code": -804, "label": "$$$4" }
+        assertIsExpected("""[[{ "$": "multi_text_key", "codePoints": [2509, 2480], "label": "্র" }]]""", Expected(KeyCode.MULTIPLE_CODE_POINTS, "্র", text = "্র"))
-      ]
+        assertIsExpected("""[[{ "$": "multi_text_key", "codePoints": [2509, 2480], "label": "x" }]]""", Expected(KeyCode.MULTIPLE_CODE_POINTS, "x", text = "্র"))
-    } },
+    }
-    { "code": 32, "label": "a|!code/key_delete" },
+
-    { "code": 32, "label": "a|b" },
+    @Test fun negativeCode() {
-    { "label": "!icon/clipboard_action_key|!code/key_clipboard" },
+        assertIsExpected("""[[{ "code":   -7, "label": "delete" }]]""", Expected(-7, icon = "delete_key"))
-    { "label": "!icon/clipboard_action_key" },
+    }
-    { "label": "p", "popup": { "main": { "label": "$$$" } } }
+
-  ],
+    @Test fun keyWithType() {
-  [
+        assertIsExpected("""[[{ "code":   57, "label": "9", "type": "numeric" }]]""", Expected(57, "9"))
-    { "label": "q" },
+        assertIsExpected("""[[{ "code":   -7, "label": "delete", "type": "enter_editing" }]]""", Expected(-7, icon = "delete_key"))
-    { "label": "s" },
+        // -207 gets translated to -202 in Int.toKeyEventCode
-    { "label": "d" },
+        assertIsExpected("""[[{ "code": -207, "label": "view_phone2", "type": "system_gui" }]]""", Expected(-202, "?123", text = "?123"))
-    { "label": "f" },
+    }
-    { "label": "g" },
+
-    { "label": "h" },
+    @Test fun spaceKey() {
-    { "label": "j" },
+        assertIsExpected("""[[{ "code":   32, "label": "space" }]]""", Expected(32, icon = "space_key"))
-    { "label": "k" },
+    }
-    { "label": "l" },
+
-    { "label": "m", "popup": { "main": { "label": "/" } } }
+    @Test fun invalidKeys() {
-  ],
+        assertThrows(KeySpecParser.KeySpecParserError::class.java) {
-  [
+            RawKeyboardParser.parseJsonString("""[[{ "label": "!icon/clipboard_action_key" }]]""").map { it.mapNotNull { it.compute(params)?.toKeyParams(params) } }
-    { "label": "w", "popup": {
-          "main": { "code":   55, "label": "!" }
-    } },
-    { "label": "x", "popup": {
-          "main": { "label": "undo" }
-    } },
-    { "label": "c", "popup": {
-          "main": { "code":   -10001, "label": "x" }
-    } },
-    { "label": "v" },
-    { "label": "b" },
-    { "label": "n" }
-  ]
-]
-        """.trimIndent()
-        val keys = RawKeyboardParser.parseJsonString(layoutString).map { it.mapNotNull { it.compute(params) } }
-        keys.first().forEachIndexed { index, keyData ->
-            println("data: key ${keyData.label}: code ${keyData.code}, popups: ${keyData.popup.getPopupKeyLabels(params)}")
-            val keyParams = keyData.toKeyParams(params)
-            println("params: key ${keyParams.mLabel}: code ${keyParams.mCode}, popups: ${keyParams.mPopupKeys?.toList()}")
-            assertEquals(expected[index].label, keyParams.mLabel)
-            assertEquals(expected[index].icon, keyParams.mIconName)
-            assertEquals(expected[index].code, keyParams.mCode)
-            // todo (later): what's wrong with popup order?
-            assertEquals(expected[index].popups?.sortedBy { it.first }, keyParams.mPopupKeys?.mapNotNull { it.mLabel to it.mCode }?.sortedBy { it.first })
-            assertEquals(expected[index].text, keyParams.outputText)
         }
-        assertEquals("!", keys.last()[0].toKeyParams(params).mPopupKeys?.first()?.mLabel)
+    }
-        assertEquals('7'.code, keys.last()[0].toKeyParams(params).mPopupKeys?.first()?.mCode)
+
-        assertEquals(null, keys.last()[1].toKeyParams(params).mPopupKeys?.first()?.mLabel)
+    @Test fun popupWithCodeAndLabel() {
-        assertEquals("undo", keys.last()[1].toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "w", "popup": {
-        assertEquals(KeyCode.UNDO, keys.last()[1].toKeyParams(params).mPopupKeys?.first()?.mCode)
+          "main": { "code":   55, "label": "!" }
-        assertEquals("x", keys.last()[2].toKeyParams(params).mPopupKeys?.first()?.mLabel)
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
-        assertEquals(-10001, keys.last()[2].toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals("!", key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals('7'.code, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
+    }
+
+    @Test fun popupWithCodeAndIcon() {
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "w", "popup": {
+          "main": { "code":   55, "label": "!icon/clipboard_action_key" }
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("clipboard_action_key", key.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals('7'.code, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
+    }
+
+    @Test fun popupToolbarKey() {
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "x", "popup": {
+          "main": { "label": "undo" }
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("undo", key.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(KeyCode.UNDO, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
+    }
+
+    @Test fun popupKeyWithIconAndImplicitText() {
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|aa" }
+      ]
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("go_key", key.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(KeyCode.MULTIPLE_CODE_POINTS, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals("aa", key.toKeyParams(params).mPopupKeys?.first()?.mOutputText)
+
+        val key2 = RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|" }
+      ]
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key2.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("go_key", key2.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(KeyCode.MULTIPLE_CODE_POINTS, key2.toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals("", key2.toKeyParams(params).mPopupKeys?.first()?.mOutputText)
+    }
+
+    // output text is null here, maybe should be changed?
+    @Test fun popupKeyWithIconAndCodeAndImplicitText() {
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|", "code": 55 }
+      ]
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("go_key", key.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(55, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals(null, key.toKeyParams(params).mPopupKeys?.first()?.mOutputText)
+
+        val key2 = RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|a", "code": 55 }
+      ]
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key2.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("go_key", key2.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(55, key2.toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals(null, key2.toKeyParams(params).mPopupKeys?.first()?.mOutputText)
+
+        val key3 = RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": { "relevant": [
+       { "label": "!icon/go_key|aa", "code": 55 }
+      ]
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals(null, key3.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals("go_key", key3.toKeyParams(params).mPopupKeys?.first()?.mIconName)
+        assertEquals(55, key3.toKeyParams(params).mPopupKeys?.first()?.mCode)
+        assertEquals(null, key3.toKeyParams(params).mPopupKeys?.first()?.mOutputText)
+    }
+
+    @Test fun invalidPopupKeys() {
+        assertThrows(KeySpecParser.KeySpecParserError::class.java) {
+            RawKeyboardParser.parseJsonString("""[[{ "label": "a", "popup": {
+          "main": { "label": "!icon/clipboard_action_key" }
+    } }]]""").map { it.mapNotNull { it.compute(params)?.toKeyParams(params) } }
+        }
+    }
+
+    @Test fun popupSymbolAlpha() {
+        val key = RawKeyboardParser.parseJsonString("""[[{ "label": "c", "popup": {
+          "main": { "code":   -10001, "label": "x" }
+    } }]]""").map { it.mapNotNull { it.compute(params) } }.flatten().single()
+        assertEquals("x", key.toKeyParams(params).mPopupKeys?.first()?.mLabel)
+        assertEquals(-10001, key.toKeyParams(params).mPopupKeys?.first()?.mCode)
     }
 
     @Test fun canLoadKeyboard() {
@@ -334,6 +458,28 @@ f""", // no newline at the end
         }
     }
 
+    private data class Expected(val code: Int, val label: String? = null, val icon: String? = null, val text: String? = null, val popups: List<Pair<String?, Int>>? = null)
+
+    private fun assertIsExpected(json: String, expected: Expected) {
+        assertAreExpected(json, listOf(expected))
+    }
+
+    private fun assertAreExpected(json: String, expected: List<Expected>) {
+        val keys = RawKeyboardParser.parseJsonString(json).map { it.mapNotNull { it.compute(params) } }.flatten()
+        keys.forEachIndexed { index, keyData ->
+            println("data: key ${keyData.label}: code ${keyData.code}, popups: ${keyData.popup.getPopupKeyLabels(params)}")
+            val keyParams = keyData.toKeyParams(params)
+            println("params: key ${keyParams.mLabel}: code ${keyParams.mCode}, popups: ${keyParams.mPopupKeys?.toList()}")
+            assertEquals(expected[index].label, keyParams.mLabel)
+            assertEquals(expected[index].icon, keyParams.mIconName)
+            assertEquals(expected[index].code, keyParams.mCode)
+            // todo (later): what's wrong with popup order?
+            assertEquals(expected[index].popups?.sortedBy { it.first }, keyParams.mPopupKeys?.mapNotNull { it.mLabel to it.mCode }?.sortedBy { it.first })
+            assertEquals(expected[index].text, keyParams.outputText)
+            assertTrue(checkKeys(listOf(listOf(keyParams))))
+        }
+    }
+
     private fun buildKeyboard(editorInfo: EditorInfo, subtype: InputMethodSubtype, elementId: Int): Pair<Keyboard, List<List<KeyParams>>> {
         val layoutParams = KeyboardLayoutSet.Params()
         val editorInfoField = KeyboardLayoutSet.Params::class.java.getDeclaredField("mEditorInfo").apply { isAccessible = true }

layouts.md

@@ -98,8 +98,6 @@ Usually the label is what is displayed on the key. However, there are some speci
 * If you want different key label and input text, set the label to [label]|[text], e.g. `aa|bb` will show `aa`, but pressing the key will input `bb`.
 You can also specify special key codes like `a|!code/key_action_previous`, but it's cleaner to use a json layout and specify the code explicitly. Note that when specifying a code in the label, and a code in a json layout, the code in the label will be ignored.
 * It's also possible to specify an icon, like `!icon/previous_key|!code/key_action_previous`.
-  * For normal keys, even if you specify a code, you will need to add a `|` to the label, e.g. `!icon/go_key|` or `!icon/go_key|ignored` (to be fixed).
-  * For popups keys, you must _not_ add a `|` (to be fixed).
   * You can find available icon names in [KeyboardIconsSet](/app/src/main/java/helium314/keyboard/keyboard/internal/KeyboardIconsSet.kt). You can also use toolbar key icons using the uppercase name of the [toolbar key](/app/src/main/java/helium314/keyboard/latin/utils/ToolbarUtils.kt#L109), e.g. `!icon/redo`
 
 ## Adding new layouts / languages