fix: fix focused window drift #4572
Conversation
src/logic/Windows.swift
Outdated
| /// Get the index of the second window that should be shown to the user | ||
| /// This is the standard behavior of switcher - select the second window (the first is usually the currently active window) |
There was a problem hiding this comment.
In some cases, the selected thumbnail is not the second one. For example, if there are no window at all, or if the the current active app has no open window, or maybe I think in some cases where the sorting order is not recently-used-first.
Is this taken into account with this MR?
There was a problem hiding this comment.
When there is no window, returning nil can handle it correctly. This fix is not related to that. Initially, I thought it was the issue and added the code, which should be rolled back.
src/ui/App.swift
Outdated
| } | ||
|
|
||
| func showUiOrCycleSelectionWithSource(_ shortcutIndex: Int, _ forceDoNothingOnRelease_: Bool, _ eventSource: EventSource) { | ||
| // Atomic operation protection - prevent concurrent calls |
There was a problem hiding this comment.
I don't think it's possible to have concurrent calls of this method. It should always be called from DispatchQueue.main. Calls should be sequentials.
There are cases of concurrency in the apps. There are always main queue + another queue. Once we get executed on the main queue, concurrency issues should be gone since that code should always be called from DispatchQueue.main, not from other queues. If it is called from other queues, it's a bug and should be fixed.
There was a problem hiding this comment.
I agree with your point that addressing the issue in advance is a very good measure. However, to prevent problems from arising, can we consider the work done here as similar to parameter validation—not trusting the input source? After all, the locking process doesn't significantly impact performance. What do you think?
There was a problem hiding this comment.
I understand what you mean. I think you mean that it doesn't hurt to add safety measures. This way, it can act as a second layer of safety, in case the first layer (not calling this code on background theads) fails.
One issue I have with this approach is that it creates complexity for the codebase readers. It make this area of code look like there is concurrency happening. The reader perusing this area may see a Lock, and think that we are doing concurrency here.
| } | ||
|
|
||
| /// Check if a duplicate event should be blocked based on event source and timing | ||
| private func shouldBlockDuplicateEvent(eventSource: EventSource, currentTime: Date) -> Bool { |
There was a problem hiding this comment.
I think that logging the event source is very useful!
I'm worried about the way we fix the issue here by trying to avoid multiple events from being processed. I think it's more desirable to avoid multiple events being generated in the first place, instead of blocking them later on
There was a problem hiding this comment.
I agree with your point, but based on the logs(#3117 (comment)) we've tracked so far, it might be an issue with macOS system event notifications. Therefore, I suggest temporarily fixing the problem by avoiding it to provide users with a better experience. Maybe one day we'll identify the root cause, and then it won't be too late to remove the logic code here.
There was a problem hiding this comment.
In these logs, it's not clear to me what happens. What is the source of calls to showUiOrCycleSelection?
In my investigation in #3117 I found one root issue: the OS seems to randomly absorb some keyboard shortcuts. Thus, the user will alt+tab, and release the tab key. At this point, AltTab doesn't get the event for the alt+tab shortcut being Up (since the tab key was released).
This explains the issue. It's tricky to deal with because I'm not sure how we can detect and workaround it.
Could you please try to describe what the fix in this MR does at a high level? I'm struggling a bit to understand the core idea of this fix. It seems to ignore some cycling events, based on the source of the event. Could you please try to explain the big idea at high-level?
Thank you!
There was a problem hiding this comment.
#3117 (comment) For this log, my action was simply pressing the Alt+Tab keys, then releasing the Tab key. However, the system received continuous event triggers, which was not my actual operation. Therefore, the fix here is to merge these events based on different event sources to reduce erroneous event triggers.
There was a problem hiding this comment.
Thank you for your message
If I understand correctly, you say that your issue is not the issue with Secure Input from #3117. You face a different issue.
In your case, you notice unwanted key repeat events. Are you sure that this is abnormal? You have shown here very fast key repeat settings. If you set the key to repeat quickly, after almost no delay, then it will happen. You will see key repeat events in all apps including AltTab.
We can't ignore key repeat events in AltTab. People want the key to repeat. For example, they hold tab, and want the selection to quickly shift to the right until they release tab
This seems like normal key repeat behavior. Do you observe something abnormal on your side?
Thank you
There was a problem hiding this comment.
Under the condition of setting up very fast key repeats, I haven't encountered any drift issues with the system's default switcher yet. Perhaps the default switcher has implemented some special handling. Additionally, I've observed that on Windows, when pressing Alt+Tab for the first time, there's a brief pause. If you continue to hold down the Tab key, the repeating behavior kicks in. This pause is used to differentiate between repeat events. The version I've fixed also takes this into MR.
For details, you can refer to the shouldBlockKeyRepeatEvent function.It does not ignore repeated events, but rather ignores faster repeated events, which users cannot perceive.
2 participants
try to fix the #3117