search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

[Mac] add support for if(), set(), reset(), save() #1838

Closed kamholz closed 4 years ago

kamholz commented 4 years ago

This addresses #1832 by implementing support for if(), set(), reset(), and save() on macOS. These now behave as expected and there is no more garbage output.

In order to support reset() and save(), I had to add another array of stores to KMXFile (with saved values) and implement copyWithZone on KMCompStore, so that it's easy to duplicate the initial array of stores.

keyman-server commented 4 years ago

Linux: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Mac: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Keymanweb:This pull request is from an external repo will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

iOS: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Windows:This pull request is from an external repo will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Keymanweb:This pull request is from an external repo will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Windows:This pull request is from an external repo will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Keymanweb:This pull request is from an external repo will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

keyman-server commented 4 years ago

Android:This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

mcdurdin commented 4 years ago

Thank you for your pull request! This is great :)

I've had a quick review but may not have time to do a full review for a week because I am about to hop on a plane to fly to Canada for a conference.

We don't currently run the full set of keyboard unit tests on macOS, so would you be able to test these changes against the tests in common/engine/keyboardprocessor/tests/unit/kmx? The .kmx files there are in the repo to avoid having to build them on systems without kmcomp, so you should be able to rely on them working.

The macOS engine is not really factored for running tests automatically and in the roadmap we plan to replace this engine with the common Keyman Core engine (in common/engine) so I don't think we need to try automating the tests, just running them manually and confirming that they pass is fine.

One potential issue that I can see is that it doesn't appear that CODE_SAVEOPT is persisting the store across user sessions, which is the intention of the option. If that's a huge change, I'd still be inclined to accept this PR as the functionality it introduces is better than no functionality. Let me know.

kamholz commented 4 years ago

Sure, I'm happy to test against those keyboards. I didn't know about Keyman Core -- great that this will all be consolidated eventually, but this provides some of the functionality meanwhile, as you say.

I'll look into what it would take to persist save() across user sessions. I'm learning as I go -- this was my first time coding in Objective C -- but maybe we can hook into the same mechanism for persisting preferences.

keyman-server commented 4 years ago

Mac: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

kamholz commented 4 years ago

The tests revealed a bug for keyboards where the right-hand side of a rule contains only some combination of set/reset/save and no explicit nul. I solved this by generating a Q_NUL action for such cases, so the engine knows the key was processed.

It doesn't look like a simple fix to make save() persist. I guess we'd have to re-write the .kmx file where it's stored? I don't really know.

I also found some dead key issues while testing. Submitting another PR momentarily that fixes those.

keyman-server commented 4 years ago

Mac: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

kamholz commented 4 years ago

OK, now supports persisting across sessions as well. Tested.

keyman-server commented 4 years ago

Mac: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

mcdurdin commented 4 years ago

I note that an exception came through for 12.0 (debug build) which means it most likely came from you?

Not sure if you have already addressed this but wanted to make sure it didn't go through to the keeper.

# Crashlytics - plaintext stacktrace downloaded by Marc Durdin at Sat, 22 Jun 2019 16:34:35 GMT
# URL: https://fabric.io/keyman/mac/apps/keyman.inputmethod.keyman/issues/24d7c0c0f9a78dc27a15d22f86514de3?time=last-ninety-days/sessions/600bcdf445dd4eb3adf1a8454505c865_DNE_0_v2
# Organization: Keyman-Debug
# Platform: mac_os_x
# Application: Keyman
# Version: 12.0
# Bundle Identifier: keyman.inputmethod.Keyman
# Issue ID: 24d7c0c0f9a78dc27a15d22f86514de3
# Session ID: 600bcdf445dd4eb3adf1a8454505c865_DNE_0_v2
# Date: 2019-06-21T14:59:00Z
# OS Version: 10.14.5 (18F132)
# Device: MacBook Pro Retina, 13", Early 2015
# RAM Free: 3.2%
# Disk Free: 11.9%

#0. Crashed: com.apple.main-thread
0  libobjc.A.dylib                0x7fff5bbba697 objc_msgSend + 23
1  CoreFoundation                 0x7fff314943a5 +[__NSSingleEntryDictionaryI __new:::] + 80
2  CoreFoundation                 0x7fff314a3534 -[NSDictionary initWithObjectsAndKeys:] + 767
3  KeymanEngine4Mac               0x1091baa42 -[KMEngine processKey:event:] (KMEngine.m:550)
4  KeymanEngine4Mac               0x1091b82f6 -[KMEngine processGroup:event:] (KMEngine.m:351)
5  KeymanEngine4Mac               0x1091b6afe -[KMEngine processEvent:] (KMEngine.m:120)
6  Keyman                         0x108ecec50 -[KMInputMethodEventHandler handleKeymanEngineActions:in:] (KMInputMethodEventHandler.m:320)
7  Keyman                         0x108ed087d -[KMInputMethodEventHandler handleEvent:client:] (KMInputMethodEventHandler.m:526)
8  Keyman                         0x108ee0bdc -[KMInputController handleEvent:client:] (KMInputController.m:54)
9  InputMethodKit                 0x10905d45a -[IMKServer handleEvent_Common:characterIndex:edge:clientWrapper:controller:] + 2296
10 InputMethodKit                 0x109051fbd __63-[IMKServer handleEvent:characterIndex:edge:asyncClient:reply:]_block_invoke_2 + 690
11 ViewBridge                     0x7fff58f0af8e +[NSServiceViewController withHostProcessIdentifier:invoke:] + 41
12 InputMethodKit                 0x109051d00 __63-[IMKServer handleEvent:characterIndex:edge:asyncClient:reply:]_block_invoke + 143
13 InputMethodKit                 0x10905e7da __IMKXPCPerformBlockOnMainThread_block_invoke + 25
14 CoreFoundation                 0x7fff314a5164 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ + 12
15 CoreFoundation                 0x7fff31468887 __CFRunLoopDoBlocks + 394
16 CoreFoundation                 0x7fff314685e4 __CFRunLoopRun + 2772
17 CoreFoundation                 0x7fff314678be CFRunLoopRunSpecific + 455
18 HIToolbox                      0x7fff3075396b RunCurrentEventLoopInMode + 292
19 HIToolbox                      0x7fff307536a5 ReceiveNextEventCommon + 603
20 HIToolbox                      0x7fff30753436 _BlockUntilNextEventMatchingListInModeWithFilter + 64
21 AppKit                         0x7fff2eaed987 _DPSNextEvent + 965
22 AppKit                         0x7fff2eaec71f -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1361
23 AppKit                         0x7fff2eae683c -[NSApplication run] + 699
24 Keyman                         0x108ec9309 main (main.m:21)
25 Keyman                         0x108ebf8b4 start + 52

--

#0. Crashed: com.apple.main-thread
0  libobjc.A.dylib                0x7fff5bbba697 objc_msgSend + 23
1  CoreFoundation                 0x7fff314943a5 +[__NSSingleEntryDictionaryI __new:::] + 80
2  CoreFoundation                 0x7fff314a3534 -[NSDictionary initWithObjectsAndKeys:] + 767
3  KeymanEngine4Mac               0x1091baa42 -[KMEngine processKey:event:] (KMEngine.m:550)
4  KeymanEngine4Mac               0x1091b82f6 -[KMEngine processGroup:event:] (KMEngine.m:351)
5  KeymanEngine4Mac               0x1091b6afe -[KMEngine processEvent:] (KMEngine.m:120)
6  Keyman                         0x108ecec50 -[KMInputMethodEventHandler handleKeymanEngineActions:in:] (KMInputMethodEventHandler.m:320)
7  Keyman                         0x108ed087d -[KMInputMethodEventHandler handleEvent:client:] (KMInputMethodEventHandler.m:526)
8  Keyman                         0x108ee0bdc -[KMInputController handleEvent:client:] (KMInputController.m:54)
9  InputMethodKit                 0x10905d45a -[IMKServer handleEvent_Common:characterIndex:edge:clientWrapper:controller:] + 2296
10 InputMethodKit                 0x109051fbd __63-[IMKServer handleEvent:characterIndex:edge:asyncClient:reply:]_block_invoke_2 + 690
11 ViewBridge                     0x7fff58f0af8e +[NSServiceViewController withHostProcessIdentifier:invoke:] + 41
12 InputMethodKit                 0x109051d00 __63-[IMKServer handleEvent:characterIndex:edge:asyncClient:reply:]_block_invoke + 143
13 InputMethodKit                 0x10905e7da __IMKXPCPerformBlockOnMainThread_block_invoke + 25
14 CoreFoundation                 0x7fff314a5164 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ + 12
15 CoreFoundation                 0x7fff31468887 __CFRunLoopDoBlocks + 394
16 CoreFoundation                 0x7fff314685e4 __CFRunLoopRun + 2772
17 CoreFoundation                 0x7fff314678be CFRunLoopRunSpecific + 455
18 HIToolbox                      0x7fff3075396b RunCurrentEventLoopInMode + 292
19 HIToolbox                      0x7fff307536a5 ReceiveNextEventCommon + 603
20 HIToolbox                      0x7fff30753436 _BlockUntilNextEventMatchingListInModeWithFilter + 64
21 AppKit                         0x7fff2eaed987 _DPSNextEvent + 965
22 AppKit                         0x7fff2eaec71f -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1361
23 AppKit                         0x7fff2eae683c -[NSApplication run] + 699
24 Keyman                         0x108ec9309 main (main.m:21)
25 Keyman                         0x108ebf8b4 start + 52

#1. com.twitter.crashlytics.mac.MachExceptionServer
0  Keyman                         0x108f067be CLSProcessRecordAllThreads + 193
1  Keyman                         0x108f06bb9 CLSProcessRecordAllThreads + 1212
2  Keyman                         0x108ef6131 CLSHandler + 45
3  Keyman                         0x108ef167c CLSMachExceptionServer + 1241
4  libsystem_pthread.dylib        0x7fff5d58a2eb _pthread_body + 126
5  libsystem_pthread.dylib        0x7fff5d58d249 _pthread_start + 66
6  libsystem_pthread.dylib        0x7fff5d58940d thread_start + 13

#2. com.apple.NSEventThread
0  libsystem_kernel.dylib         0x7fff5d4cb22a mach_msg_trap + 10
1  libsystem_kernel.dylib         0x7fff5d4cb76c mach_msg + 60
2  CoreFoundation                 0x7fff31468bee __CFRunLoopServiceMachPort + 328
3  CoreFoundation                 0x7fff3146815c __CFRunLoopRun + 1612
4  CoreFoundation                 0x7fff314678be CFRunLoopRunSpecific + 455
5  AppKit                         0x7fff2eaf56a6 _NSEventThread + 175
6  libsystem_pthread.dylib        0x7fff5d58a2eb _pthread_body + 126
7  libsystem_pthread.dylib        0x7fff5d58d249 _pthread_start + 66
8  libsystem_pthread.dylib        0x7fff5d58940d thread_start + 13

#3. com.apple.NSURLConnectionLoader
0  libsystem_kernel.dylib         0x7fff5d4cb22a mach_msg_trap + 10
1  libsystem_kernel.dylib         0x7fff5d4cb76c mach_msg + 60
2  CoreFoundation                 0x7fff31468bee __CFRunLoopServiceMachPort + 328
3  CoreFoundation                 0x7fff3146815c __CFRunLoopRun + 1612
4  CoreFoundation                 0x7fff314678be CFRunLoopRunSpecific + 455
5  CFNetwork                      0x7fff303db380 -[__CoreSchedulingSetRunnable runForever] + 210
6  Foundation                     0x7fff336c16d2 __NSThread__start__ + 1194
7  libsystem_pthread.dylib        0x7fff5d58a2eb _pthread_body + 126
8  libsystem_pthread.dylib        0x7fff5d58d249 _pthread_start + 66
9  libsystem_pthread.dylib        0x7fff5d58940d thread_start + 13

#4. JavaScriptCore bmalloc scavenger
0  libsystem_kernel.dylib         0x7fff5d4ce86a __psynch_cvwait + 10
1  libsystem_pthread.dylib        0x7fff5d58d56e _pthread_cond_wait + 722
2  libc++.1.dylib                 0x7fff5a5c8a0a std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&) + 18
3  JavaScriptCore                 0x7fff34944122 void std::__1::condition_variable_any::wait<std::__1::unique_lock<bmalloc::Mutex> >(std::__1::unique_lock<bmalloc::Mutex>&) + 82
4  JavaScriptCore                 0x7fff3494820b bmalloc::Scavenger::threadRunLoop() + 139
5  JavaScriptCore                 0x7fff34947a39 bmalloc::Scavenger::threadEntryPoint(bmalloc::Scavenger*) + 9
6  JavaScriptCore                 0x7fff349493a7 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(bmalloc::Scavenger*), bmalloc::Scavenger*> >(void*) + 39
7  libsystem_pthread.dylib        0x7fff5d58a2eb _pthread_body + 126
8  libsystem_pthread.dylib        0x7fff5d58d249 _pthread_start + 66
9  libsystem_pthread.dylib        0x7fff5d58940d thread_start + 13

#5. Thread
0  libsystem_kernel.dylib         0x7fff5d4ccbfe __workq_kernreturn + 10
1  libsystem_pthread.dylib        0x7fff5d5896e6 _pthread_wqthread + 634
2  libsystem_pthread.dylib        0x7fff5d5893fd start_wqthread + 13
3  (Missing)                      0x54485244 (Missing)

#6. Thread
0  libsystem_kernel.dylib         0x7fff5d4ccbfe __workq_kernreturn + 10
1  libsystem_pthread.dylib        0x7fff5d589636 _pthread_wqthread + 458
2  libsystem_pthread.dylib        0x7fff5d5893fd start_wqthread + 13
3  (Missing)                      0x0 (Missing)
kamholz commented 4 years ago

Yes, that was me. Keyman crashed a few times while I was developing fixes. Not aware of any crashing code in the pull requests.

keyman-server commented 4 years ago

Mac: This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

kamholz commented 4 years ago

Last commit fixes case where a store value is saved and restored across sessions, then reset() is called. It would have reset to the default value rather than the saved value.

kamholz commented 4 years ago

By the way, it would be pretty easy to detect whether a kmx file has any set/reset/save commands and only copy store to storeSaved in that case. Is that worth it or too much trouble? I don't know how much the memory savings would be, or how important that is compared to the (minor) cost of scanning the output fields in each key.

mcdurdin commented 4 years ago

By the way, it would be pretty easy to detect whether a kmx file has any set/reset/save commands and only copy store to storeSaved in that case. Is that worth it or too much trouble?

I'm inclined to think it would not be worth the added complexity (because that increases risk of bugs, etc.) as the cost is not significant, especially as most keyboards are typically less than 100kb. Optimisation can wait until we know it's needed.

mcdurdin commented 4 years ago

-1/2 035 Tests basic option rules with save reset+set+reset [again, a mostly bad test which cannot execute half of the rules] [0 and 3 work, but 1 cannot be reached, nor 2 as only 1 sets 2]

Yes, these tests have a different input state which we cannot currently test on macOS. I'm inclined to not worry about them at this point.

mcdurdin commented 4 years ago

Keyman for Mac 11.0.2 contains this fix.