feat(mac): invoking Keyman Core from Keyman Engine for Mac 🍕 #8403
Conversation
User Test ResultsTest specification and instructions
Test Artifacts
|
…Core action processing
sgschantz
requested review from
SabineSIL,
mcdurdin and
rc-swag
as code owners
| @@ -105,6 +101,7 @@ - (BOOL)createEventTap { | |||
| return YES; | |||
| } | |||
|
|
|||
| // TODO: investigate -- event handler for OSK only? | |||
There was a problem hiding this comment.
- TODO: investigate -- is
eventTapFunctionevent handler for OSK only?
| /** | ||
| * Keyman is copyright (C) SIL International. MIT License. | ||
| * | ||
| * CachedContext.h |
There was a problem hiding this comment.
| * CachedContext.h | |
| * KMContext.h |
| -(instancetype)init NS_DESIGNATED_INITIALIZER; | ||
| -(instancetype)initWithString:(NSString*)initialContext; | ||
| -(void)resetContext:(NSString*)newContext; | ||
| -(void)addSubtring:(NSString*)string; |
There was a problem hiding this comment.
| -(void)addSubtring:(NSString*)string; | |
| -(void)addSubstring:(NSString*)string; |
| -(void)resetContext:(NSString*)newContext { | ||
| [_context setString:[KMContext trimToMaximumContext:newContext]]; | ||
| self.invalid = NO; | ||
| NSLog(@" ***CachedContext resetContext, resulting context = %@", _context); |
There was a problem hiding this comment.
These NSLog functions should be enabled for debug only. Preference is a global debug logging function so we don't scatter #ifdef DEBUG everywhere.
There was a problem hiding this comment.
I can use the function tied to the AppDelegate:
if (self.AppDelegate.debugMode) {
NSLog(@"containedInNoncompliantAppLists: for app %@: %@", clientAppId, isAppNonCompliant?@"yes":@"no");
}
There was a problem hiding this comment.
Is it possible to turn that into a helper function so we can keep the log calls to a single line?
|
|
||
| -(void)clearContext { | ||
| _context.string = @""; | ||
| NSLog(@" ***CachedContext clearContext, resulting context = %@", _context); |
| if (contextLength > 0) { | ||
| NSUInteger characterIndex = contextLength-1; | ||
| NSRange characterRange = [self.context rangeOfComposedCharacterSequenceAtIndex:characterIndex]; | ||
| NSLog(@"KMContext deleteLastUnicodeCharacter, index = %lu, rangeOfLastCharacter = %@\n", characterIndex, NSStringFromRange(characterRange)); |
| NSLog(@"KMContext deleteLastUnicodeCharacter, index = %lu, rangeOfLastCharacter = %@\n", characterIndex, NSStringFromRange(characterRange)); | ||
| [self.context deleteCharactersInRange:characterRange]; | ||
| } | ||
| NSLog(@" ***CachedContext deleteLastCodePoint, resulting context = %@", _context); |
There was a problem hiding this comment.
I'm not actually changing the logging for KMContext now because I expect to be deleting it and relying on the context stored in core instead.
| if (newText.length > 0) { | ||
| [self addSubtring:newText]; | ||
| } | ||
| NSLog(@" ***CachedContext replaceSubstring, resulting context = %@", _context); |
| return self; | ||
| } | ||
|
|
||
| +(NSString*)trimToMaximumContext:(NSString*)initialContext { |
There was a problem hiding this comment.
This function doesn't account for surrogate pairs
| NSLog(@" ***CachedContext deleteLastCodePoint, resulting context = %@", _context); | ||
| } | ||
|
|
||
| -(void)replaceSubstring:(NSString*)newText count:(int)count { |
There was a problem hiding this comment.
Is count number of codepoints or number of code units? We should document that
| NSLog(@" ***CachedContext deleteLastCodePoint, resulting context = %@", _context); | ||
| } | ||
|
|
||
| -(void)replaceSubstring:(NSString*)newText count:(int)count { |
There was a problem hiding this comment.
Is count number of codepoints or number of code units? We should document that
| } | ||
|
|
||
| -(void)addSubtring:(NSString*)string { | ||
| [self.context appendString:string]; |
There was a problem hiding this comment.
This could make the context too long so we need to retrim afterwards
|
|
||
| -(void)addSubtring:(NSString*)string { | ||
| [self.context appendString:string]; | ||
| NSLog(@" ***CachedContext addSubtring, resulting context = %@", _context); |
| case MarkerAction: | ||
| [self appendMarker]; | ||
| break; |
There was a problem hiding this comment.
A marker has a value between 1 and 0xFFFE, so you need to store UC_NONCHARACTER + the marker value.
| /* | ||
| -(instancetype)initWithActions:(NSArray*)actions context: (KMContext *)context event: (NSEvent *)event client:(id) client; | ||
| */ |
There was a problem hiding this comment.
| /* | |
| -(instancetype)initWithActions:(NSArray*)actions context: (KMContext *)context event: (NSEvent *)event client:(id) client; | |
| */ |
There was a problem hiding this comment.
removed context which is not needed by KMCoreActionHandler
| return action.actionType == EndAction; | ||
| } | ||
|
|
||
| /* |
| } | ||
| return self; |
There was a problem hiding this comment.
| } | |
| return self; | |
| } | |
| return self; |
|
|
||
| // Creates a VK map to convert Mac VK codes to Windows VK codes | ||
| - (void)initVirtualKeyMapping { | ||
| VirtualKeyMap[MVK_A] = VK_KEY_A; // A |
There was a problem hiding this comment.
Separate PR? alphabetize this because the current order is more than a little screwy!
|
|
||
| // TODO: create once | ||
| // create option list | ||
| km_kbp_option_item coreEnvironment[6] = {0}; |
There was a problem hiding this comment.
Why 6? Probably needs to be a define somewhere
| coreOptionArray[3].scope = KM_KBP_OPT_ENVIRONMENT; | ||
| coreOptionArray[3].key = KM_KBP_KMX_ENV_BASELAYOUTGIVESCTRLRALTFORRALT; | ||
| //coreOptionArray[3].value = KeyboardGivesCtrlRAltForRAlt() ? u"1" : u"0"; | ||
| coreOptionArray[3].value = u""; // const char16_t*, encoded as UTF-16 |
There was a problem hiding this comment.
| coreOptionArray[3].value = u""; // const char16_t*, encoded as UTF-16 | |
| coreOptionArray[3].value = u"0"; // const char16_t*, encoded as UTF-16 |
mac/Keyman4MacIM/Keyman-template.dmg
Outdated
There was a problem hiding this comment.
does the dmg need to be checked in?
There was a problem hiding this comment.
Yes, this template determines the size of the dmg, and we were getting a build failure because it was too small after adding the core library dependencies. This change was to increase the size of the template dmg by 2MB.
…Options function name overwritten in merge
| typedef enum {BackspacesOnly, | ||
| CharactersOnly, | ||
| BackspaceBeforeCharacter, | ||
| CharacterBeforeBackspace, |
There was a problem hiding this comment.
I still don't think this can happen?
|
|
||
| @interface KMCoreActionHandler () | ||
|
|
||
| typedef enum {BackspacesOnly, |
There was a problem hiding this comment.
Can we apply the formatting fixes?
| if (self.appDelegate.lowLevelEventTap != nil) { | ||
| NSEvent *eventWithOriginalModifierFlags = [NSEvent keyEventWithType:event.type location:event.locationInWindow modifierFlags:self.appDelegate.currentModifierFlags timestamp:event.timestamp windowNumber:event.windowNumber context:event.context characters:event.characters charactersIgnoringModifiers:event.charactersIgnoringModifiers isARepeat:event.isARepeat keyCode:event.keyCode]; | ||
| actions = [self.kme processEvent:eventWithOriginalModifierFlags]; | ||
| [self.appDelegate logDebugMessage:@"processEventWithKeymanEngine, using AppDelegate.currentModifierFlags %lu, instead of event.modifiers = %lu", (unsigned long)self.appDelegate.currentModifierFlags, (unsigned long)event.modifierFlags]; | ||
| } |
There was a problem hiding this comment.
these lines are very long, can you reformat?
There was a problem hiding this comment.
will refactor during later cleanup
| if (self.appDelegate.contextChangedByLowLevelEvent) | ||
| { |
There was a problem hiding this comment.
| if (self.appDelegate.contextChangedByLowLevelEvent) | |
| { | |
| if (self.appDelegate.contextChangedByLowLevelEvent) { |
There was a problem hiding this comment.
Thanks for the thorough re-review. I had missed a chunk of those. A few I am going to deal with when I use the core action API change and with one other cleanup. I need to address one review comment on modifier flags and then I think it will be ready.
| km_core_status result = km_core_context_get(context, &contextItemsArray); | ||
| if (result==KM_CORE_STATUS_OK) { | ||
| for (int i = 0; i < contextLength; i++) { | ||
| km_core_context_item contextItem = contextItemsArray[i]; | ||
| if (contextItem.type == KM_CORE_CT_CHAR) { | ||
| const unichar unicodeChar = contextItem.character; | ||
| NSString *charString = [NSString stringWithCharacters:&unicodeChar length:1]; | ||
| [contextString appendString:charString]; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this correct for surrogate pairs (i.e. >= U+10000)?
There was a problem hiding this comment.
unichar says it's UTF-16 so this wouldn't be correct. Line 277 is a narrowing of the type from 32 to 16 bits.
in C++ i have the Utf32CharToUtf16() macro to convert utf-32 to a 1-or-2-code unit string
There was a problem hiding this comment.
can you use https://developer.apple.com/documentation/swift/unicode/scalar instead ?
There was a problem hiding this comment.
Thanks for catching this! I had created a helper function for this conversion but neglected to use it here. I've added a unit test to ensure this works for a context string containing emojis.
…SMP text; also other clean up in response to review comments
Initial steps in integration of Keyman Engine with Core:
TODO
User Testing
Install and select the Amharic keyboard. Run Pages and verify that
TEST_IPA:
Switch to the "IPA (SIL)" keyboard. Run Pages, type n> and verify that the result is ŋ
TEST_KOREAN:
Switch to "Korean KORDA Jamo (SIL)" keyboard. Run Pages and type han, space, geul, space Verify that the result is "한글"
TEST_CONTEXT:
Switch to the "IPA (SIL)" keyboard. Run Pages
and verify that the output is
aŋ
m