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(mac): both option keys generate right alt if no left alt mapping #12458

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

Conversation

sgschantz
Copy link
Contributor

@sgschantz sgschantz commented Sep 23, 2024

When a keyman keyboard does not define any rules for the Left Alt key, then both the left and right option keys on the Mac keyboard should result in a Right Alt key combination being generated.

Fixes: #875
Fixes: #933

User Testing

  • TEST_RALT_KEYBOARD_WITH_BOTH_OPTION_KEYS: Khmer Angkor uses only RALT rules
  1. Install this version of Keyman and switch to the Khmer Angkor keyboard
  2. Press left-option-Z
  3. Confirm that this generates the character '<'
  4. Press right-option-Z
  5. Confirm that this generates the character '<'
  • TEST_RALT_KEYBOARD_WITH_OPTION_KEY_COMBOS: Armenian Mnemonic uses RALT with SHIFT
  1. Install and switch to the Armenian Mnemonic keyboard
  2. Press left-option-9
  3. Confirm that this generates the subscript character '₉'
  4. Press shift-left-option-9
  5. Confirm that this generates the superscript character '⁹'
  6. Press shift-right-option-9
  7. Confirm that this generates the same superscript character '⁹'
  • TEST_LALT_KEYBOARD_WITH_BOTH_OPTION_KEYS: Devanagari Typewriter uses only LALT rules
  1. Install and switch to the Devanagari Typewriter (SIL) keyboard
  2. Press control-left-option-COMMA
  3. Confirm that this generates the DEVANAGARI DOUBLE DANDA character '॥'
  4. Press control-right-option-COMMA
  5. Confirm that this generates the comma character ','

@sgschantz sgschantz added the mac/ label Sep 23, 2024
@sgschantz sgschantz added this to the A18S12 milestone Sep 23, 2024
@sgschantz sgschantz self-assigned this Sep 23, 2024
@github-actions github-actions bot added the feat label Sep 23, 2024
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Sep 23, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Sep 23, 2024

User Test Results

Test specification and instructions

  • TEST_RALT_KEYBOARD_WITH_BOTH_OPTION_KEYS (PASSED) (notes)
  • TEST_RALT_KEYBOARD_WITH_OPTION_KEY_COMBOS (PASSED) (notes)
  • TEST_LALT_KEYBOARD_WITH_BOTH_OPTION_KEYS (PASSED) (notes)

Test Artifacts

@sgschantz sgschantz marked this pull request as ready for review September 27, 2024 09:52
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

This LGTM, aside from concerns on memory management. Good work

@@ -67,6 +64,38 @@ -(void)changeKeyboardWithKmxFilePath:(NSString*) path {
}
}

-(CoreKeyboardInfo*) getKeyboardInfoForKmxFile:(NSString*)kmxFile {
NSArray *keyArray = [self getKeyArray];
CoreKeyboardInfo* info = [[CoreKeyboardInfo alloc] init:kmxFile keyArray: keyArray];
Copy link
Member

Choose a reason for hiding this comment

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

Is keyArray and its members leaked here?

NSMutableArray *keyArray = [[NSMutableArray alloc] initWithCapacity:0];
km_core_keyboard_key *keyList;

km_core_status result = km_core_keyboard_get_key_list(self.coreKeyboard, &keyList);
Copy link
Member

Choose a reason for hiding this comment

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

Is keyList leaked here?

@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.118-alpha-local" build on the macOS Sonoma 14.5 version. here, I am sharing my observation.

  • TEST_RALT_KEYBOARD_WITH_BOTH_OPTION_KEYS (passed):
  1. Installed the "Keyman 18.0.118-alpha-local" build.
  2. Installed the Khmer Angkor keyboard
  3. Select the Khmer Angkor keyboard
  4. Press left-option-Z
  5. Verified that this generates the character '<'
  6. Press right-option-Z
  7. Verified that this generates the character '<'
  • TEST_RALT_KEYBOARD_WITH_OPTION_KEY_COMBOS (passed):
  1. Installed the "Keyman 18.0.118-alpha-local" build.
  2. Installed the "Armenian Mnemonic" keyboard
  3. Select the Armenian Mnemonic keyboard
  4. Press left-option-9
  5. Verified that this generates the subscript character '₉'
  6. Press shift-left-option-9
  7. Verified that this generates the superscript character '⁹'
  8. Press shift-right-option-9
  9. Verified that this generates the same superscript character '⁹'
  • TEST_LALT_KEYBOARD_WITH_BOTH_OPTION_KEYS (passed):
  1. Installed the "Keyman 18.0.118-alpha-local" build.
  2. Installed the "Devanagari Typewrite" keyboard
  3. Select the Devanagari Typewriter (SIL) keyboard
  4. Press control-left-option-COMMA
  5. Verified that this generates the DEVANAGARI DOUBLE DANDA character '॥'
  6. Press control-right-option-COMMA
  7. Verified that this generates the comma character ','

It works well. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
3 participants