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

feat: add Colemak-DH key mapping #414

Merged
merged 8 commits into from
Feb 25, 2024

Conversation

mlouielu
Copy link
Contributor

Impl #413

  • Add Colemak-DH ANSI key mapping
  • Add Colemak-DH Orth key mapping

@mlouielu
Copy link
Contributor Author

The downside of this PR is the ABI break for keyboard enum, see #412.

@kanru kanru added this to the v1.0.0 milestone Dec 31, 2023
@kanru kanru added the feature label Dec 31, 2023
@mlouielu mlouielu force-pushed the add-colemak-dh-key-mappings branch from aca4637 to d7b0f55 Compare February 25, 2024 01:26
@mlouielu
Copy link
Contributor Author

Update PR for #412 so that it will not break the ABI.

@kanru
Copy link
Member

kanru commented Feb 25, 2024

You probably need to resolve the merge conflict so CI can run.

@mlouielu mlouielu force-pushed the add-colemak-dh-key-mappings branch from d7b0f55 to 4e865e5 Compare February 25, 2024 01:53
@mlouielu
Copy link
Contributor Author

Thanks, rebased.

@mlouielu mlouielu force-pushed the add-colemak-dh-key-mappings branch from 4e865e5 to 1c11ec8 Compare February 25, 2024 01:56
Copy link

codecov bot commented Feb 25, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.69%. Comparing base (1711d4e) to head (64ff57c).

Files Patch % Lines
src/editor/keyboard/colemak_dh_ansi.rs 75.00% 1 Missing ⚠️
src/editor/keyboard/colemak_dh_orth.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   79.66%   79.69%   +0.03%     
==========================================
  Files          59       61       +2     
  Lines       11444    11466      +22     
==========================================
+ Hits         9117     9138      +21     
- Misses       2327     2328       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kanru
Copy link
Member

kanru commented Feb 25, 2024

@mlouielu would you be interested to also add Colemak key mapping to the Rust implementation?

For now I think you only need to update here to let the tests pass.

#[derive(Clone, Copy, Debug, PartialEq)]
#[repr(C)]
pub enum KeyboardLayoutCompat {
/// TODO: docs
Default = 0,
/// TODO: docs
Hsu,
/// TODO: docs
Ibm,
/// TODO: docs
GinYieh,
/// TODO: docs
Et,
/// TODO: docs
Et26,
/// TODO: docs
Dvorak,
/// TODO: docs
DvorakHsu,
/// TODO: docs
DachenCp26,
/// TODO: docs
HanyuPinyin,
/// TODO: docs
ThlPinyin,
/// TODO: docs
Mps2Pinyin,
/// TODO: docs
Carpalx,
}
impl FromStr for KeyboardLayoutCompat {
type Err = ();
fn from_str(kb_str: &str) -> Result<Self, Self::Err> {
let layout = match kb_str {
"KB_DEFAULT" => Self::Default,
"KB_HSU" => Self::Hsu,
"KB_IBM" => Self::Ibm,
"KB_GIN_YIEH" => Self::GinYieh,
"KB_ET" => Self::Et,
"KB_ET26" => Self::Et26,
"KB_DVORAK" => Self::Dvorak,
"KB_DVORAK_HSU" => Self::DvorakHsu,
"KB_DACHEN_CP26" => Self::DachenCp26,
"KB_HANYU_PINYIN" => Self::HanyuPinyin,
"KB_THL_PINYIN" => Self::ThlPinyin,
"KB_MPS2_PINYIN" => Self::Mps2Pinyin,
"KB_CARPALX" => Self::Carpalx,
_ => todo!("handle error"),
};
Ok(layout)
}
}
impl Display for KeyboardLayoutCompat {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
KeyboardLayoutCompat::Default => f.write_str("KB_DEFAULT"),
KeyboardLayoutCompat::Hsu => f.write_str("KB_HSU"),
KeyboardLayoutCompat::Ibm => f.write_str("KB_IBM"),
KeyboardLayoutCompat::GinYieh => f.write_str("KB_GIN_YIEH"),
KeyboardLayoutCompat::Et => f.write_str("KB_ET"),
KeyboardLayoutCompat::Et26 => f.write_str("KB_ET26"),
KeyboardLayoutCompat::Dvorak => f.write_str("KB_DVORAK"),
KeyboardLayoutCompat::DvorakHsu => f.write_str("KB_DVORAK_HSU"),
KeyboardLayoutCompat::DachenCp26 => f.write_str("KB_DACHEN_CP26"),
KeyboardLayoutCompat::HanyuPinyin => f.write_str("KB_HANYU_PINYIN"),
KeyboardLayoutCompat::ThlPinyin => f.write_str("KB_THL_PINYIN"),
KeyboardLayoutCompat::Mps2Pinyin => f.write_str("KB_MPS2_PINYIN"),
KeyboardLayoutCompat::Carpalx => f.write_str("KB_CARPALX"),
}
}
}
impl TryFrom<u8> for KeyboardLayoutCompat {
type Error = ();
fn try_from(value: u8) -> Result<Self, Self::Error> {
Ok(match value {
0 => Self::Default,
1 => Self::Hsu,
2 => Self::Ibm,
3 => Self::GinYieh,
4 => Self::Et,
5 => Self::Et26,
6 => Self::Dvorak,
7 => Self::DvorakHsu,
8 => Self::DachenCp26,
9 => Self::HanyuPinyin,
10 => Self::ThlPinyin,
11 => Self::Mps2Pinyin,
12 => Self::Carpalx,
_ => return Err(()),

@mlouielu
Copy link
Contributor Author

Yep, doing that right now!

@mlouielu mlouielu force-pushed the add-colemak-dh-key-mappings branch from 7c032c9 to 67b92d3 Compare February 25, 2024 02:20
src/capi/io.rs Outdated
Comment on lines 247 to 248
KB::ColemakDhAnsi => (AnyKeyboardLayout::qwerty(), Box::new(Standard::new())),
KB::ColemakDhOrth => (AnyKeyboardLayout::qwerty(), Box::new(Standard::new())),
Copy link
Member

Choose a reason for hiding this comment

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

Note this will map Colemak to QWERTY. This doesn't block merging your PR because we don't really have a test yet and Rust is not release blocking.

If you want to fix it you need to add a new definition like this

https://github.com/chewing/libchewing/blob/7c032c9567ca1707ea668dddb2c416648ac6b124/src/editor/keyboard/qgmlwy.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try to add it, are there anyway to test if my impl is correct or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, I'm not sure if I can add it correctly, because colemak-dh orth keyboard layout can depend on the user, they only defined 40 keys in it.

image
(figure from: https://colemakmods.github.io/mod-dh/keyboards.html)

Copy link
Member

@kanru kanru Feb 25, 2024

Choose a reason for hiding this comment

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

You can add some tests to test/test-bopomofo.c test_KB()

If you have rust installed locally you can switch to build and test rust with

cmake --preset rust
cmake --build out/build/rust -t all test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate more on the KeyboardLayout, how will it affect others?

For example, Colemak-DH Orth didn't define - in the layout, what will happen if I replace them with Unknown? And, if the user has different layout than the KEYCODE_INDEX (e.g. I define my - at mod+t, not at a regular place), will that also affect the output?

Copy link
Member

Choose a reason for hiding this comment

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

btw, I'm not sure if I can add it correctly, because colemak-dh orth keyboard layout can depend on the user, they only defined 40 keys in it.

Yeah, the current design assumed the keyboard is a variation of the us-102 keyboard. Ortholinear keyboard should still work if you leave the other keys in the same place as us-102.

Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate more on the KeyboardLayout, how will it affect others?

KeyboardLayout is used to map key codes to bopomofo key symbols. Some bopomofo keyboard like Standard only look at the key index to determine it's ㄅ or ㄆ or ㄇ.

For example, Colemak-DH Orth didn't define - in the layout, what will happen if I replace them with Unknown? And, if the user has different layout than the KEYCODE_INDEX (e.g. I define my - at mod+t, not at a regular place), will that also affect the output?

I guess as long as you keep the - in the same place in the KEYCODE_INDEX it should be fine. No other code currently uses position dependent keymaps for other symbols.

Yeah, I realize the current design doesn't work well for ortholiner keyboard. I have plans to refactor the keymap module but this isn't the priority for now. If it doesn't work I'm can merge your PR just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in rust & add basic tests

@mlouielu mlouielu force-pushed the add-colemak-dh-key-mappings branch from 67b92d3 to 64ff57c Compare February 25, 2024 03:29
@kanru kanru added this pull request to the merge queue Feb 25, 2024
Merged via the queue into chewing:master with commit acd5bff Feb 25, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants