chore(mac): use core actions struct by sgschantz · Pull Request #10066 · keymanapp/keyman

sgschantz · 2023-11-23T07:58:34Z

Instead of processing an array of action structs, get a single struct from core that encapsulates all the impacts of the most recently processed key.

Fixes #7916
Fixes #7542
Fixes #7526
Fixes #7021
Fixes #5012
Fixes #10116

User Testing

TEST_AMHARIC_STICKIES:

Open the Stickies app on the Mac and create a new note
Switch to the gff_amharic keyboard
Type selam
Verify that the output is ሰላም

TEST_AMHARIC_TEXSTUDIO_9506:

Open texstudio on the Mac and create a new document
Switch to the gff_amharic keyboard
Type selam
Verify that the output is ሰላም

TEST_YIDDISH_STICKIES:

Open the Stickies app on the Mac and create a new note
Switch to the Yiddish Pasekh keyboard
Type key
Confirm that ouput is קײ
Type rey
Verify that ouput is רײ

TEST_YIDDISH_GOOGLE_DOCS_7916:

Open an empty document in Google Docs
Switch to the Yiddish Pasekh keyboard
Type key
Confirm that ouput is קײ
Type rey
Verify that ouput is רײ

TEST_KENYA_BTL_STICKIES:

Open the Stickies app on the Mac and create a new note
Switch to the Kenya (BTL) keyboard
Type b^ug
Verify that ouput is bûg

TEST_KENYA_BTL_GOOGLE_CHROME_7542:

Open Google Chrome on the Mac
Switch to the Kenya (BTL) keyboard
Click on the URL field and type b^ug
Verify that ouput is bûg

TEST_HEBREW_ENTER_STICKIES:

Open the Stickies app on the Mac and create a new note
Switch to the Galaxie Hebrew (Positional) keyboard
Type mm
Verify that ouput is ממ
Type return
Verify that ouput has changed to מם

TEST_POLYTONIC_GREEK_CHROME_BACKSPACE_7526:

Open Google Chrome and select the URL text field
Switch to the keyboard 'Polytonic Greek (SIL)'
Type abc backspace backspace backspace
Verify that the output changes as follows: ἀ, ἀβ, ἀβχ, ἀβ, ἀ, [empty]

TEST_POLYTONIC_GREEK_BACKSPACE_7021:

Open the Stickies app and create a new note
Switch to the keyboard 'Polytonic Greek (SIL)'
Type antos
Verify that pressing backspace will delete one character at a time
Five backspaces will delete the entire word

…om keyman core

keymanapp-test-bot · 2023-11-23T07:58:40Z

User Test Results

Test specification and instructions

✅ TEST_AMHARIC_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed gff_amharic keyboard. 2. Opened a Note app. 3. Typed 'selam'. 4. Verified the output shows ሰላም in the note app. (notes)
🟩 TEST_AMHARIC_TEXSTUDIO_9506 (SKIPPED): bug(mac): not replacing on Texstudio (QT6) for Amharic #9506 can still happen -- need to document exact steps to reproduce as it is sometimes successful
✅ TEST_YIDDISH_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed Yiddish Pasekh keyboard keyboard. 2. Opened a Stickey app. 3. Typed 'key'. 4. Verified the output shows קײ in the stickey app. 5. Typed 'rey'. Verified the output shows רײ the app. (notes)
✅ TEST_YIDDISH_GOOGLE_DOCS_7916 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed Yiddish Pasekh keyboard keyboard. 2. Opened Google doc. 3. Typed 'key'. 4. Verified the output shows קײ in the document. 5. Typed 'rey'. Verified the output shows רײ the document. I noticed that the alignment of the document has been changed from right to left. (?) (notes)
✅ TEST_KENYA_BTL_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Kenya (BTL)' keyboard. 2. Opened Stickies app. 3. Typed 'b^ug'. 4. Verified the output shows 'bûg' in the note. (notes)
✅ TEST_KENYA_BTL_GOOGLE_CHROME_7542 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Kenya (BTL)' keyboard. 2. Opened Chrome browser. 3. Typed 'b^ug' in the URL text field. 4. Verified the output shows 'bûg' in the URL text field. (notes)
✅ TEST_HEBREW_ENTER_STICKIES (PASSED): Retested as per Shawn's suggestion and here is my observation: 1. After pressing the key, I was able to see the expected output on the screen. (notes)
✅ TEST_POLYTONIC_GREEK_CHROME_BACKSPACE_7526 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Polytonic Greek (SIL)' keyboard. 2. Opened Chrome browser. 3. Typed 'abc'. 4. Verified the output shows ἀβχ in the URL text field. 5. Then, typed 'backspace backspace backspace' shows an empty space in the URL text field. (notes)
✅ TEST_POLYTONIC_GREEK_BACKSPACE_7021 (PASSED): 1. Opened Stickies app. 2. Installed 'Polytonic Greek (SIL)' keyboard. Typed 'antos'. 3. Verified that pressing backspace deleted one character at a time. 4. Verified that pressing Five backspaces deleted the entire word.

bharanidharanj · 2023-12-08T10:43:06Z

Test Results

TEST_AMHARIC_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed gff_amharic keyboard. 2. Opened a Note app. 3. Typed 'selam'. 4. Verified the output shows ሰላም in the note app.

bharanidharanj · 2023-12-08T10:51:04Z

TEST_YIDDISH_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed Yiddish Pasekh keyboard keyboard. 2. Opened a Stickey app. 3. Typed 'key'. 4. Verified the output shows קײ in the stickey app. 5. Typed 'rey'. Verified the output shows רײ the app.

bharanidharanj · 2023-12-08T10:57:06Z

TEST_YIDDISH_GOOGLE_DOCS_7916 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed Yiddish Pasekh keyboard keyboard. 2. Opened Google doc. 3. Typed 'key'. 4. Verified the output shows קײ in the document. 5. Typed 'rey'. Verified the output shows רײ the document. I noticed that the alignment of the document has been changed from right to left. (?)

bharanidharanj · 2023-12-08T11:23:10Z

TEST_HEBREW_ENTER_STICKIES (FAILED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed Galaxie Hebrew (Positional) keyboard. 2. Opened Stickey app. 3. Typed 'mm'. 4. Verified the output shows ממ in the app. 5. Typed 'return'. Noticed that it shows different output in the app.

bharanidharanj · 2023-12-08T11:33:39Z

TEST_POLYTONIC_GREEK_CHROME_BACKSPACE_7526 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Polytonic Greek (SIL)' keyboard. 2. Opened Chrome browser. 3. Typed 'abc'. 4. Verified the output shows ἀβχ in the URL text field. 5. Then, typed 'backspace backspace backspace' shows an empty space in the URL text field.

..Typed 'abc' letters

..Pressing Backspace three times

TEST_POLYTONIC_GREEK_BACKSPACE_7021 (PASSED): 1. Opened Stickies app. 2. Installed 'Polytonic Greek (SIL)' keyboard. Typed 'antos'. 3. Verified that pressing backspace deleted one character at a time. 4. Verified that pressing Five backspaces deleted the entire word.

bharanidharanj · 2023-12-08T11:54:27Z

TEST_KENYA_BTL_STICKIES (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Kenya (BTL)' keyboard. 2. Opened Stickies app. 3. Typed 'b^ug'. 4. Verified the output shows 'bûg' in the note.
TEST_KENYA_BTL_GOOGLE_CHROME_7542 (PASSED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'Kenya (BTL)' keyboard. 2. Opened Chrome browser. 3. Typed 'b^ug' in the URL text field. 4. Verified the output shows 'bûg' in the URL text field.

bharanidharanj · 2023-12-08T12:03:35Z

Test Results

TEST_AMHARIC_TEXSTUDIO_9506 (FAILED): Tested with the attached PR build Keyman 17.0.202-alpha-test-10066 on macOS Sonoma 14.1.2 and here is my observation: 1. Installed 'gff_amharic' keyboard. 2. Opened a new document. 3. Typed 'selam' in the document. 4. Verified it displays the wrong output.

mcdurdin

This is looking fantastic. Most of my comments are nits, but a couple of things I don't fully understand.

mac/Keyman4Mac/Keyman4Mac/AppDelegate.m

mcdurdin · 2023-12-12T05:29:43Z

mac/Keyman4MacIM/Keyman4MacIM/KeySender.m

-(void) dealloc {
-  CFRelease(self.eventSource);
+- (void)sendBackspaceforEventSource:(CGEventSourceRef)eventSource {
+  [self.appDelegate logDebugMessage:@"KeySender sendBackspaceforSourceEvent"];


Suggested change

[self.appDelegate logDebugMessage:@"KeySender sendBackspaceforSourceEvent"];

[self.appDelegate logDebugMessage:@"KeySender sendBackspaceforEventSource"];

mcdurdin · 2023-12-12T05:31:14Z

mac/Keyman4MacIM/Keyman4MacIM/KeySender.m

+    CGEventRef ev = CGEventCreateKeyboardEvent (source, virtualKey, true); //down
+    if (postEvent)
+        postEvent(ev);
+    CFRelease(ev);
+    ev = CGEventCreateKeyboardEvent (source, virtualKey, false); //up
+    if (postEvent)
+        postEvent(ev);
+    CFRelease(ev);


This seems like it has a precondition of if(!postEvent) return;?

Suggested change

CGEventRef ev = CGEventCreateKeyboardEvent (source, virtualKey, true); //down

if (postEvent)

postEvent(ev);

CFRelease(ev);

ev = CGEventCreateKeyboardEvent (source, virtualKey, false); //up

if (postEvent)

postEvent(ev);

CFRelease(ev);

if(!postEvent) {

// TODO: add logging? how could this happen?

return;

}

CGEventRef ev = CGEventCreateKeyboardEvent (source, virtualKey, true); //down

postEvent(ev);

CFRelease(ev);

ev = CGEventCreateKeyboardEvent (source, virtualKey, false); //up

postEvent(ev);

CFRelease(ev);

It should never happen, but I'll change to add logging.

mcdurdin · 2023-12-12T05:32:18Z

mac/Keyman4MacIM/Keyman4MacIM/KeySender.m

+  // Returns the frontmost app, which is the app that receives key events.
+  NSRunningApplication *app = NSWorkspace.sharedWorkspace.frontmostApplication;
+  pid_t processId = app.processIdentifier;
+  NSString *appName = app.localizedName;


is localizedName the most helpful string for debug logging? Might not be better to use an identifier of some kind?

changed to use bundleId

mcdurdin · 2023-12-12T05:32:45Z

mac/Keyman4MacIM/Keyman4MacIM/TextApiCompliance.m

-                     [clientAppId isEqual: @"com.adobe.InDesign"] ||
-                     [clientAppId isEqual: @"com.adobe.Photoshop"] ||
-                     [clientAppId isEqual: @"com.adobe.AfterEffects"] ||
+                     // Adobe apps automatically detected as non-compliant


Do we have an issue number for this?

No, but I'll include the date in my comment to show when I removed these from the hard-coded list.

mcdurdin · 2023-12-12T05:39:22Z

mac/Keyman4Mac/Keyman4Mac/AppDelegate.m

-        for (CoreAction *action in actions) {
-            if (action.isCharacter) {
-                NSString *output = action.content;
+        CoreKeyOutput *coreKeyOutput = [kme processEvent:mEvent];


I'm curious, because this function seems to ignore backspaces entirely?

This is the old sample app which I have tried to ignore other than making sure it still builds. However, I often confuse it for Keyman IM when it comes up in search results, so I should eventually either confirm that it is actually useful or remove it from the project.

Ah, yes, I make the same mistake too. At the very least, let's rename it!

mcdurdin · 2023-12-12T05:42:58Z

mac/Keyman4MacIM/Keyman4MacIM/KMInputMethodEventHandler.m

+}
+
+-(void) persistOptions:(NSDictionary*)options{
+  if(options.count>0) {


Do we need this test? Because the loop will be a no-op anyway

mcdurdin · 2023-12-12T05:43:25Z

mac/Keyman4MacIM/Keyman4MacIM/KMInputMethodEventHandler.m

+    case On:
+      //TODO: handle this
+      break;
+    case Off:
+      //TODO: handle this
+      break;
+    case Unchanged:
+      // do nothing
+      break;


Do we have an issue to track this? If so, can you put it into the TODO comments?

created #10244

mcdurdin · 2023-12-12T05:43:44Z

mac/Keyman4MacIM/Keyman4MacIM/KMInputMethodEventHandler.m

+    for (int i = 0; i < output.codePointsToDeleteBeforeInsert; i++)
    {


Suggested change

for (int i = 0; i < output.codePointsToDeleteBeforeInsert; i++)

{

for (int i = 0; i < output.codePointsToDeleteBeforeInsert; i++) {

mcdurdin · 2023-12-12T05:44:38Z

mac/KeymanEngine4Mac/KeymanEngine4MacTests/KMEngineTests.m

 }

- (void)testCoreProcessEvent_eventDeleteWithElNuerKmx_EmptiesContextReturnsDelete {
+// TODO: enable after resolving issue getting options from Keyman Core


Are you tracking this?

Yes, I will enable after merging to ensure that the core changes cause this test to pass.

sgschantz · 2023-12-13T02:48:44Z

TEST_AMHARIC_TEXSTUDIO_9506 (SKIP): bug(mac): not replacing on Texstudio (QT6) for Amharic #9506 can still happen -- need to document exact steps to reproduce as it is sometimes successful

sgschantz · 2023-12-13T05:59:34Z

@bharanidharanj Can you provide more detail on what the text the note contained with each step for TEST_HEBREW_ENTER_STICKIES? I don't understand how that whole line of text could have appeared after you pressed return, and I am not seeing anything similar.

bharanidharanj · 2023-12-13T06:32:34Z

@bharanidharanj Can you provide more detail on what the text the note contained with each step for TEST_HEBREW_ENTER_STICKIES? I don't understand how that whole line of text could have appeared after you pressed return, and I am not seeing anything similar.

Hi @sgschantz Sorry for the confusion! When I followed the steps you mentioned in the test, I thought "Type Return" meant "Type the word Return" on the input screen. So, I typed the text "Return" and it was displayed on the screen. Now, I tried with the key and got the expected result.

sgschantz · 2023-12-13T07:36:12Z

@keymanapp-test-bot retest TEST_HEBREW_ENTER_STICKIES

Please retest and simply press the return key as the last step

bharanidharanj · 2023-12-13T07:45:15Z

Test Results

TEST_HEBREW_ENTER_STICKIES (PASSED): Retested as per Shawn's suggestion and here is my observation: 1. After pressing the key, I was able to see the expected output on the screen.

mcdurdin

LGTM, one tiny tweak which is probably unlikely to ever arise but should be done anyway 😁

mac/Keyman4MacIM/Keyman4MacIM/KeySender.m

add guard clause Co-authored-by: Marc Durdin <marc@durdin.net>

keyman-server · 2023-12-14T18:05:05Z

Changes in this pull request will be available for download in Keyman version 17.0.230-alpha

sgschantz added 7 commits November 3, 2023 08:43

consuming actions struct from core

ada7403

formatting changes

75a99a9

disable persist options unit test, enforce emit keystroke returned fr…

d20f636

…om keyman core

remove single backspace code and rely on emit from core

c966a62

remove action array optimizer and handler obsoleted by core improvements

ad34af7

remove some of the legacy backspace code

6879af0

pass through delete keystroke to avoid re-sending event

b909540

sgschantz added the mac/ label Nov 23, 2023

sgschantz self-assigned this Nov 23, 2023

keymanapp-test-bot bot added this to the A17S26 milestone Nov 23, 2023

github-actions bot added the chore label Nov 23, 2023

darcywong00 modified the milestones: A17S26, A17S27 Nov 27, 2023

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Dec 5, 2023

mcdurdin mentioned this pull request Dec 8, 2023

refactor(core): KM_CORE_IT_INVALIDATE_CONTEXT shouldn't be no-op, or there should be other api to handle it #10182

Closed

8 tasks

eventTap changes, add delay to queued text event

27b1236

sgschantz mentioned this pull request Dec 8, 2023

bug(mac): amharic keyboard is not working as expected on chrome in sonoma #10116

Closed

8 tasks

sgschantz marked this pull request as ready for review December 8, 2023 07:38

sgschantz requested a review from SabineSIL as a code owner December 8, 2023 07:38

mcdurdin modified the milestones: A17S27, A17S28 Dec 8, 2023

keymanapp-test-bot bot added the user-test-failed label Dec 8, 2023

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Dec 8, 2023

mcdurdin requested changes Dec 12, 2023

View reviewed changes

sgschantz mentioned this pull request Dec 13, 2023

bug(mac): not replacing on Texstudio (QT6) for Amharic #9506

Closed

8 tasks

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Dec 13, 2023

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Dec 13, 2023

changes for review comments

71d4710

mcdurdin approved these changes Dec 14, 2023

View reviewed changes

mac/Keyman4MacIM/Keyman4MacIM/KeySender.m Outdated Show resolved Hide resolved

Update mac/Keyman4MacIM/Keyman4MacIM/KeySender.m

4b5495e

add guard clause Co-authored-by: Marc Durdin <marc@durdin.net>

sgschantz merged commit 55f69ae into master Dec 14, 2023

sgschantz deleted the chore/mac/use-core-actions-struct branch December 14, 2023 04:58

sgschantz mentioned this pull request Apr 17, 2024

bug(mac): EMUFI Ge'ez Manuscript Incorrect Symbol Replacement #9559

Closed

8 tasks

sgschantz mentioned this pull request Apr 21, 2025

bug(mac): double space output the last character with a space instead of just a space [khmer_angkor] #7363

Closed

	[self.appDelegate logDebugMessage:@"KeySender sendBackspaceforSourceEvent"];
	[self.appDelegate logDebugMessage:@"KeySender sendBackspaceforEventSource"];

		for (int i = 0; i < output.codePointsToDeleteBeforeInsert; i++)
		{

Uh oh!

Conversation

sgschantz commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Uh oh!

keymanapp-test-bot bot commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Uh oh!

bharanidharanj commented Dec 8, 2023

Test Results

Uh oh!

bharanidharanj commented Dec 8, 2023

Uh oh!

bharanidharanj commented Dec 8, 2023

Uh oh!

bharanidharanj commented Dec 8, 2023

Uh oh!

bharanidharanj commented Dec 8, 2023

Uh oh!

bharanidharanj commented Dec 8, 2023

Uh oh!

bharanidharanj commented Dec 8, 2023

Test Results

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgschantz commented Dec 13, 2023

Uh oh!

sgschantz commented Dec 13, 2023

Uh oh!

bharanidharanj commented Dec 13, 2023

Uh oh!

sgschantz commented Dec 13, 2023

Uh oh!

bharanidharanj commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgschantz commented Nov 23, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 23, 2023 •

edited

Loading

bharanidharanj commented Dec 13, 2023 •

edited

Loading