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

Make it easier to create variants #700

Closed
wants to merge 11 commits into from

Conversation

BlueDrink9
Copy link
Contributor

@BlueDrink9 BlueDrink9 commented Feb 9, 2024

Should help a little to tame the ridiculous proliferation of variants, and reduce conflicts when people update them or fix bugs.

@BlueDrink9
Copy link
Contributor Author

BlueDrink9 commented Feb 9, 2024

@dessalines I'd like some help with this one. It's working flawlessly once you've shifted and gone back, but the initial keyboard isn't showing the change. Any idea why? Is that just some cached state issue?
By 'initial' I mean when first switching to the relevant variant.

Current code has a test set up on english thumb-key. Should have thorn symbols in the middle

@BlueDrink9
Copy link
Contributor Author

Also added a function to auto-create shifted layers

@dessalines
Copy link
Owner

I'm sure it has to do with mutability, and how terribly its handled in kotlin / java. Make sure that you are never changing or modifying the original keyboard, but make .copy( 's . Or when you call toMutable(), that its a new variable. All functions should be immutable and never actually change their inputs.


// Merge swipes, with variant's swipes overriding source's swipes for the same SwipeDirection
val newSwipes =
source.swipes.orEmpty().toMutableMap().apply {
Copy link
Owner

Choose a reason for hiding this comment

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

apply might be the issue here, I think it alters source data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I thought I was creating a mutable deep copy. I'll investigate, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no dice so far with this route of investigation. converting to a map creates a copy, so the original isn't being modified at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not looking like this was the issue. Probably need to figure out how to apply the debugger. Gonna be a wee while, I might not get time to work on this for a week or two

@BlueDrink9
Copy link
Contributor Author

Thinking more about where this is heading.

  • i'm working on fixing merging layouts with different key numbers, mainly merging numeric and symbols.
  • This could lead to real composability for layouts: we'd have the base numeric-only layer, and a symbols-only layer. Keyboards could all share a symbols definition but have different layouts
  • we could eventually have a mix-and-match system in the settings that allows a custom layout that picks and chooses your bast layout, symbols, numeric, and maybe a separate numeric or shifted set of symbols.
  • potential pitfall if not handled correctly in the unhandled combos might crash the app. Would have to only support grids

@BlueDrink9
Copy link
Contributor Author

i'm not going to touch settings, at least for a while, so once this is merged that might be someone else's job

// Returns a variant of a layer created by replacing source keys and swipes
// with any non-dummy equivalents in the variant.

// Assuming both source and variant have the same structure (same number of rows and columns)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be prudent to have some check for source and variant actually having the same structure, although I'm not sure how to enforce that at compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the works

@asdkant
Copy link
Contributor

asdkant commented Feb 11, 2024

@BlueDrink9 I've been thinking that it may be a good idea to do some slight changes on the dataclass KeyboardC, adding a flag to indicate if we expect the MAIN layer to have lower-case only letter. If so, capitaliseKeyboardLetters() would be used to automatically calculate the SHIFTED variant. And also some kind of enforcing of lowercase letter in MAIN if the flag is set.

I'm not sure if another mechanism should be set for languages that are much different, if there's another slightly similar mechanic then the value could be an enum instead of a boolean flag (maybe make it an enum anyway since it's safer and more maintainable)

@BlueDrink9
Copy link
Contributor Author

i think i'm missing some pieces, could you describe the motivation in more detail? that function already calculates shifted layers

@asdkant
Copy link
Contributor

asdkant commented Feb 12, 2024

I'm assuming that there are languages we'd want to have but don't have lower/upper case as we would think of it, or someone may want to make a keyboard that doesn't just change alphanumeric characters to upper case on the shifted keyboard, but we want to make life easier for the (I'm assuming majority) people who make keyboards with lower/upper case mechanics where the shifted layer is just main with upper case letters.

Looking at EsMessagEase for example, declaration of the layout is:

val KB_ES_MESSAGEASE: KeyboardDefinition =
    KeyboardDefinition(
        title = "Español MessagEase",
        modes =
            KeyboardDefinitionModes(
                main = KB_ES_MESSAGEASE_MAIN,
                shifted = KB_ES_MESSAGEASE_SHIFTED,
                numeric = KB_ES_MESSAGEASE_NUMERIC,
            ),
    )

My thinking went along the lines of guardrails to ensure that we're doing things "the proper way" with any given keyboard.

If the new convention is that shifted is capitaliseKeyboardLetters(main), why not make it so we don't have to declare shifted in the first place?
So we either pass an argument to KeyboardDefinition() to indicate it works this way (I'd assume the default to be the old way, so old keyboards keep working fine), or we have a new, different function that makes shifted automatically.

Changing how KeyboardDefinition() works feels out of scope for this PR, and I'm not sure if @dessalines would want a different function that does something very similar, so what I'm proposing may end up being done later or not at all. If refactoring how the function works is desirable, I'm okay with taking on that work after this PR is done.

edit: thinking a bit more, we could just add a replacement for KeyboardDefinitionModes() that automatically calculates the shifted variant, but I think having a replacement for KeyboardDefinition() that takes title, main and numeric is cleaner.

@BlueDrink9
Copy link
Contributor Author

oh yup, that's good thinking. Definitely out of scope of ths PR though

@zworek
Copy link
Contributor

zworek commented Feb 29, 2024

I've also noticed that this whole big keyboard config file is really prone to bugs, and I have some WIP branch that would allow to create 3x3 keyItemC based on simple(human-readable) text config. Something like this

val SOME_POLISH_TEXT_LAYOUT =
    """
        +---+---+---+
        |   | Ń |Ł  |
        | A-|+N!|?I |
        |¿ĄV|\L/|X=€|
        +---+---+---+
        |{Ó%|QUP|| }|
        |(WK|COB|MR)|
        |[Ć_|GDJ|@ ]|
        +---+---+---+
        |~ Y|"H'|F&x|
        |<ZŹ|ŻET|ŚS>|
        | Ę |,.:|;  |
        +---+---+---+
    """
        .trimIndent()
        .replace("¿", "\$") // \\\$ would brake ascii table formatting so some kind of replacement'¿' is used instead

    
val KB_PL_EXAMPLE = makeVariant(
    SPECIAL_KEYITEMS_AND_SPACE_KB, // 4 x 4 with shifted mode, and whole right column and space bar
    getKeyboardFromTextLayout(SOME_POLISH_TEXT_LAYOUT) // upper-left 3 x 3
)

getKeyboardFromTextLayout would create proper swipes and add certain colors to center key/alphanumeric/special characters (while ignoring all spaces).

I have a working example but still need to clean up some code 🧑‍🏭
In order for this to work, I also had to loosen the requirement for equal structure in makeVariant. This allows for even easier extension of keyboard created by getKeyboardFromTextLayout to add some custom functionality if needed.

I've noticed you're both worried about mutability - is there some evidence of it? Because I haven't noticed any side effects 🤷

@BlueDrink9
Copy link
Contributor Author

BlueDrink9 commented Feb 29, 2024

I think my bug is caused by something other than mutibility.

Fyi, dessalines has expressed elsewhere a strong opposition to text-based (or rather, non-typed) layout setup.

@BlueDrink9
Copy link
Contributor Author

Also, would this handle asymmetric/non grid layouts?

@zworek
Copy link
Contributor

zworek commented Mar 1, 2024

Also, would this handle asymmetric/non grid layouts?

At this moment, it only handles a 3x3 grid. I probably could extend it to any MxN grid of squares if needed, but I don't know if it's a good option because:

  • Your makeVariant has to somehow merge those buttons, and at this moment it doesn't take into account various key widths etc (simply operates on lists).
  • it's just a helper, that would make most default layouts easier to maintain / produce. It doesn't stop you from creating layouts "the old way".
  • I've also created function getKeyItemCFromText which could be more useful for those more creative layouts.
    It returns KeyItemC and it could easily consume some string input like this
QUP
COB
GDJ
  • Maybe we could create some other helpers to move the created keyboard down/right(translate/addUpLeftMargin) by adding some DUMMY_KEYS or others to slice it into smaller parts before merging.. but with this functionality, as of now, I'd stick with the YAGNI principle 😄

Fyi, dessalines has expressed elsewhere a strong opposition to text-based (or rather, non-typed) layout setup.

@dessalines Are there some specific reasons for this? I think it could really reduce the amount of code and work needed for new layouts, and also make them more bullet-proof.

@dessalines
Copy link
Owner

Yes that's correct, I'm not going to support a 2nd untyped keyboard definition. Every keyboard, and all its fields, must be compile-time checked.

@BlueDrink9
Copy link
Contributor Author

If this is being loaded/converted into the relevant KeyC etc objects, that'd still be compile-time checking they work, right?

@dessalines
Copy link
Owner

Most likely no. You can always go from typed to untyped, but not the other way around.

zworek pushed a commit to zworek/thumb-key that referenced this pull request Mar 20, 2024
@zworek
Copy link
Contributor

zworek commented Mar 20, 2024

I'm not really sure at which point my code would lost this "compile-time checking"..

Finally I found some time to clean up my code so I've cherry-picked part of this PR and created #811 for further discussion.

@dessalines
Copy link
Owner

If you want to maintain a separate format, you'll have to do that in your own repo with your own script. I don't have time to do so.

Can re-open if the OP would like to continue work on this.

@dessalines dessalines closed this Mar 22, 2024
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.

4 participants