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

Korean #899

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Korean #899

wants to merge 7 commits into from

Conversation

tenextractor
Copy link

It seems to work, but more testing and code cleanup is needed

@tenextractor tenextractor marked this pull request as ready for review November 30, 2024 08:35
@plumpdolphin
Copy link

Awesome work!

I just had time to go through, build and test your solution and it seems to work very well! With the layouts you created (here), I think the input is really smooth and natural! It deviates slightly from other popular Korean keyboards in two ways; this is not inherently a bad thing, but I figure I should mention them to add context.

Notes on Implementation

  1. Your implementation works really well for syllables to be partially erased, such as if you accidentally write the wrong 받침/final consonants or hit the wrong vowel, it allows you to back up mid-syllable block and continue on which I think it really nice! On some implementations I've seen, the default behavior is to delete the entire syllable block even if you have not finished it which can be break the logical flow for the user and waste precious keystrokes. Yours performs excellently in that regard.

  2. The combiner logic for the starting consonants, such as ㄱ, ㄷ, ㅅ, etc being combined into ㄲ, ㄸ, ㅆ respectively on subsequent presses works perfect to my testing! The deviation from what I've seen in the Samsung keyboard (my reference) is that this combining logic is different between Dubeolsik/두벌식 and Danmoeum/단모음. In the case of Samsung's board, the doubling on subsequent presses only occurs on Danmoeum/단모음 and not on Dubeolsik/두벌식. Instead the shift key or a long press selection must be used to access (ㅃ, ㅉ, ㄸ, ㄲ, ㅆ). This allows much easier typing for text slangs like ㄱㄱ (고고), ㄷㄷ (덜덜), ㅂㅂ (바이 바이), etc. While this is by no means a necessity, as it is perfectly usable as is, I think it could be a valuable quality-of-life improvement for daily use of the more common Dubeolsik/두벌식 layout.

Note: I did have trouble at first getting the layouts to appear within the application settings, and I noted it was because the layouts were not included in the assets/layouts/mapping.yaml file. I am not familiar with how proposed layouts are migrated from the futo-keyboard-layouts repository to become default layouts, so this process may be automated. Just wanted to throw that out there!

Conclusion

This seems like usable with exactly what you have already. Feel free to ask questions if you'd like to try to implement the combiner feature separation that I mentioned. Otherwise, this is very functional as is and could be of great use, as-is. I think any further functionality or cleanup could be done down the line, but it'd be awesome to have this in the hands of users soon!

Fortunately/unfortunately, I have no say-so here other than a personal endorsement and a thumbs-up. So consider this my unofficial ACK 👍

@tenextractor
Copy link
Author

Thanks a lot for your feedback and appreciation.
Right now, a combiner can't be restricted to a specific layout, so the KoreanCombiner is active for all layouts. This means that I wasn't able to add different behavior for Dubeolsik and Danmoeum, and both use the same combiner. Also, I have noticed that the iPhone Korean keyboard combines ㄱㄱ -> ㄲ, while Gboard does not, so I thought it wouldn't be too big of a problem for both layouts to combine ㄱㄱ. In the future when we have the ability to use different combiners with different layouts, we can have a separate Dubeolsik combiner that does not combine ㄱㄱ. Until then, we either have to use this, or fully disable ㄱㄱ -> ㄲ even in Danmoeum.

@plumpdolphin
Copy link

Sounds very reasonable! It seems it would likely require and update to the Combiner class's getCombiningStateFeedback() method to accept an optional context value as well as additional context to be passed to the combiner via the CombinerChain class. I haven't gone deeper than that, such as how to define how to specify the context for the given layout, any standardization of the context/configuration between combiners, but it could get a little hairy for sure.

As far as using it vs disabling combining in Danmoeum: since it is practically imperative that doubling occurs on Danmoeum to match the common typing style, while on Dubeolsik the doubling is at most slightly uncomfortable. This seems like a good option for now!

I did think of one other option potentially: doubled consonants, if you add a space without a subsequent vowel, it could be be expanded into two consecutive consonants. For example + Space = ㄱㄱ . To my knowledge, there are no texting slangs that require double consonants, only singles. This would allow the same use of shorthands without breaking conventional keystroke order for either layout. I'm not sure if that's been used before in other keyboards, but it seems promising! But it is by no means a requirement, especially if it behaves identical on the iPhone keyboard then the precedent is there for current functionality, then it's a matter of common preference.

@@ -58,6 +59,7 @@ public CombinerChain(final String initialText) {
mCombiners = new ArrayList<>();
// The dead key combiner is always active, and always first
mCombiners.add(new DeadKeyCombiner());
mCombiners.add(new KoreanCombiner());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding it by default for all layouts, you can now add it as an enum here

And specify in the layout that you want to use that combiner by the enum name you defined

val combiners: List<CombinerKind> = listOf(CombinerKind.DeadKey)

You can specify different combiners for different layouts now too, if they need to have different combining rules

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for this. This changes everything

Copy link
Author

Choose a reason for hiding this comment

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

Done

@tenextractor
Copy link
Author

Now there are 2 combiners: KoreanCombineInitials which combines ㄱㄱ -> ㄲ, and Korean which does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants