-
Notifications
You must be signed in to change notification settings - Fork 113
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
SR_keyEventType may fail to interpret a CGEvent-based NSEvent. #129
Comments
AFAIK NSEvent.init(cgEvent:) returns an optional, do you check somewhere that the conversation was successful? Can you catch that case before conversion to NSEvent and dump the CGEvent (e.g. via Perhaps macOS may erase (or not know?) key code that caused the change. I wonder what's the value of |
@lwouis Were you able to resolve the issue? |
@Kentzo sorry about the delay! Here are the relevant logs:
Here is the code that logs: os_log("keyboardHandler1: %{public}@, %{public}@, %{public}@",
CGEvent.tapIsEnabled(tap: eventTap!).description, type.rawValue.description, cgEvent.flags.rawValue.description)
// 10: .keydown, 11: .keyUp, 12: .flagsChanged
if type == .keyDown || type == .keyUp || type == .flagsChanged {
let event__ = NSEvent(cgEvent: cgEvent)
os_log("keyboardHandler2: %{public}@, %{public}@", event__?.modifierFlags.rawValue.description ?? "nil", event__?.keyCode.description ?? "nil")
// 256: base modifiers, 1048840: command // 48: tab, 55: command
if let event_ = NSEvent(cgEvent: cgEvent),
// workaround: NSEvent.characters is not safe outside of the main thread; this is not documented by Apple
// see https://github.com/Kentzo/ShortcutRecorder/issues/114#issuecomment-606465340
let event = NSEvent.keyEvent(with: event_.type, location: event_.locationInWindow, modifierFlags: event_.modifierFlags,
timestamp: event_.timestamp, windowNumber: event_.windowNumber, context: nil, characters: "",
charactersIgnoringModifiers: "", isARepeat: type == .flagsChanged ? false : event_.isARepeat, keyCode: event_.keyCode) {
let appWasBeingUsed = App.app.appIsBeingUsed
// ShortcutRecorder handles only exact matches for modifiers-only .up shortcuts. We want to activate holdShortcut even if other modifiers are still pressed
// see https://github.com/lwouis/alt-tab-macos/issues/230
let holdShortcutAction = GeneralTab.shortcutActions["holdShortcut"]!
let holdShortcut = holdShortcutAction.shortcut!
if holdShortcut.keyCode == .none && type == .flagsChanged && event.sr_keyEventType == .up &&
event.modifierFlags.isDisjoint(with: holdShortcut.modifierFlags) {
_ = holdShortcutAction.actionHandler!(holdShortcutAction)
}
The code does: let event__ = NSEvent(cgEvent: cgEvent) so os_log("keyboardHandler2: %{public}@, %{public}@", event__?.modifierFlags.rawValue.description ?? "nil", event__?.keyCode.description ?? "nil") relevant part being: Here if Thus it seems that the
This is logged: os_log("keyboardHandler1: %{public}@, %{public}@, %{public}@",
CGEvent.tapIsEnabled(tap: eventTap!).description, type.rawValue.description, cgEvent.flags.rawValue.description) which produced:
Here we see that the |
What value for If the information about the key is not included into event the only other option is to track modifierFlags and diff manually. Please share the value of |
…event when keyCode is incorrect SRAXGlobalShortcutMonitor intercepts all keyboard events which allows to track changes of the modifier flags more or less reliably and thus diff previous and current values. Refs #129
Please test the workaround I just added. |
My current understanding was that it's macOS which is strangely generating buggy events (with a
Could you please tell me a bit more about what this new commit goal is and how it tries to achieve it? |
Since event tap installed by SRAXGlobalShortcutMonitor intercepts all keyboard events, it's possible to maintain previous state of modifier flags. That allows to calculate the difference in modifier flags even if keyCode is 0 and pick the right event type. |
…event when keyCode is incorrect SRAXGlobalShortcutMonitor intercepts all keyboard events which allows to track changes of the modifier flags more or less reliably and thus diff previous and current values. Refs #129
…event when keyCode is incorrect SRAXGlobalShortcutMonitor intercepts all keyboard events which allows to track changes of the modifier flags more or less reliably and thus diff previous and current values. Refs #129
Even though SR is storing state to remember modifiers state, i'm worried about side-stepping the fact that the OS sends a bogus event. Like what is actually happening on the physical keyboard when the OS sends that event? Did the user even press anything in real life? Maybe it would be better to ignore these events rather than react to them with stored state. Without knowing why these exist, i don't know it it's wise to side-step them. What do you think? I'm nervous to include update the app for this because none of these scenario happen on my machine, so i won't be able to test/observe the effects and will have to release the app to production and potentially receive many complains that this somehow broke on other people computers. Shortcuts issue are pretty much the only remaining bug in AltTab after around 300 tickets down. They are really sneaky as they appear only on other people machines, and it's hard for me to work off of the few logs people share. |
That's fair, let's conduct further testing. Can you add the following code in your tap handler where your receive CGEvent from the system: let hasKeyCode = cgEvent.getIntegerValueField(.keyboardEventKeycode) != 0
let documentsURL = FileManager.default.url(for: .documentDirectory,
in: userDomainMask,
appropriateFor: nil,
shouldCreate: true)
let bogusCGEventURL = documentsURL.appendingPathComponent("CGEvent-\(cgEvent.timestamp)-\(hasKeyCode ? "" : "bogus-")\(ProcessInfo.processInfo.globallyUniqueString).data",
isDirectory: false)
(cgEvent.data! as! NSData).write(to: bogusCGEventURL, atomically: true)
os_log("cgEvent is written to %{public}@", bogusCGEventURL) Then after analyzing the data we will be able to see whether my workaround is sound. (of course, feel free to alter the dir and name however you like) |
I haven't gone around to add this snippet yet. For context, this issue only happen on other people computers, not mine. This means I have to make a local build with your changes, give it to them, hope that they are reactive enough to actually come back to me with results, and be able to explain what shortcut they pressed so we know what the logs are reflecting. This is not very easy, as I've experienced in the past. Today I was looking at an issue where a user says the app crashes as soon as they press the modifier key. I checked their crash report, and interestingly I see:
Looking into that
It seems to me like There is no clear documentation from apple stating what is thread safe or not, but we have seen issues in the past like the What do you think about this theory? |
Being not thread safe is one thing, being main thread only is another. Apple's API by default is not thread safe but not main thread ony unless documented otherwise. It's sometimes hard to unwind documentation because an API that's otherwise not documented as main-thread-only may have such dependency somewhere inside. CGEvent.h has the following to say about Event Taps:
You thus should make sure that
Perhaps you're initializing the monitor from a non-main thread with a run loop of the main thread? |
I think you misunderstood my theory. I'm not saying that there is an issue with access to the The issue, I believe, lies with what's being processed in that run loop. Namely, there are calls to Moreover, I'm suggesting that perhaps certain code paths lead to that crash, while other paths (depending on data-races due to the internal state stored by Cocoa) may be the reason why the |
You're likely right. I will have to remove NSEvent-related API from the SRAXGlobalShortcutMonitor's handler. |
…dler. NSEvent seems to depend on being use from the main thread, which is unncessary limiting for the monitor. Refs #129
…ndler. NSEvent seems to depend on being use from the main thread, which is unncessary limiting for the monitor. Refs #129
Please take a look (it's a force push). |
…ndler NSEvent seems to depend on being used from the main thread, which is unncessary limiting for the monitor. Refs #129
@lwouis Do you have any feedback before I merge this? |
Hey @Kentzo! Very sorry about my lack of feedback. I haven't been able to test this yet. Tickets have been pilling up like crazy over at AltTab, and I haven't had time to:
We have also unveiled that the biggest issue with keyboard input not working was in fact Secure Input. The issue outlined in this ticket is minor in comparison. I've been experimenting with private APIs which work-around Secure Input. That would however mean I can't use I'm really sorry for the lack on reactivity on my part. I'm a bit overwhelmed by the volume of tickets I'm getting. I closed 25 tickets in the past 2 weeks. I also added a crash report system, so now I have a whole bunch of crash reports to analyze in AppCenter. I'll keep you updated regarding my work on the Secure Input workaround. That's a big usability issue for users. Some users just turn on their VPN and bam the event taps don't receive anything anymore. I can't use the Carbon APIs either as they have limitations. I have to go for private APIs, or add some UI to notify users that AltTab is disabled because another app is using Secure Events. If I go for the UI, I'll migrate to |
I can always add an additional target to the project with whatever private mangling needed. |
I assumed you would not be open to include private APIs, as that would prevent users from using the framework for a MAS-friendly app. Or maybe there is a way to flag it so it adds that code only with certain xcode flags, so that project that want it can have it, and other project can avoid the private API linking and calling? |
Can be done via either a compile-time flag or a separate, extended, target. |
…ndler NSEvent seems to depend on being used from the main thread, which is unncessary limiting for the monitor. Refs #129
@lwouis I saw that you closed a related issue in your project. Do you think any of your changes can be re-integrated with ShortcutRecorder? |
Sorry I should have updated this ticket already. There are 2 topics i think here. keyboard input processing outside of the main threadUsing the This approach was great, however it may be that some CGEvent or NSEvent objects have bugs when processed outside the main thread. I haven't been able to go to the bottom of this though. What I've done for AltTabThe big issue with the CGEvent API is that is doesn't read inputs when SecureInput is enabled, which for some users can be all the time from a corporate VPN or antivirus, effectively breaking AltTab for this users completely. Thus i moved away from this API to the deprecated carbon APIs. I'm using a mix of multiple carbon APIs to process shortcuts, even modifiers-only shortcuts. I think the takeaway for SR, if any, would be to mimic my strategy to implement modifiers-only shortcuts using the carbon APIs. I can detail it more if you're interested |
Please give me the details. I believe that ShortcutMonitor is generic enough for any underlying API. |
The main carbon API is It may seem like the easiest way to do shortcut recording would be to listen to keyDown/keyUp + modifiersChanged. The issue here is that keyDown/keyUp stop sending events when SecureInput is enabled by some app, or the OS itself (e.g. many users reported a long-standing bug where macOS We can't use only hotKeyPressed though, as its API doesn't accept no keyCode, so we can't have modifiers-only shortcuts, which I wanted to support (#114). The trick is to combine hotKeyPressed with modifiersChanged. Another layer of complexity is that I want the shortcuts to also work when the app is active, but the To recap:
Things to keep in mind:
The full implementation code is pretty short, maybe 100 lines. I suggest you check it as reference if you want more details like exact parameters for the API calls, or how I clean up the handlers if they are no longer necessary. I hope that helps! |
Do you know if InstallEventHandler requires the accessibility permission for modifiersChanged? |
I didn't test clearly during development, as AltTab needs the permission anyway to operate, but I noted:
So, not sure really, but it seems to depend on what exactly is being observed. More tests would be needed to make sure what requires permissions exactly. |
Hi @Kentzo!
So, we've been trying to identify a keyboard issue with AltTab's community for a while now. Today, I analyzed new logs from a user, and I see this message in these logs:
This message is coming from ShortcutRecorder. After digging a bit, I found the log line:
ShortcutRecorder/Library/SRShortcutAction.m
Line 395 in efde820
I'm quite puzzled as to how this is happening. From the caller (AltTab), we call get a
CGEvent
fromCGEvent.tapCreate
. Then we construct aNSEvent
based on thatCGEvent
:Then we create a new
NSEvent
based off thatNSEvent
(to workaround the issue we had with calling.characters
outside the main thread; I think you remember this workaround):At this point we call
event.sr_keyEventType
to learn if a modifier key was went up or down. Within this method, the log line is logged.I can't imagine why the
keyCode
would be0
. From the NSEvent header, it doesn't even seem like 0 should be a possible value:Do you have any idea how this scenario would be happening?
Originally posted by @lwouis in #114 (comment)
The text was updated successfully, but these errors were encountered: