Fix data race issues when switching causing the desktop focus to land on a completely different window #183
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses data race issues in the window focus switching mechanism to prevent desktop focus from landing on unintended windows. The changes improve thread safety and reduce monitoring overhead by replacing the continuous focus monitoring thread with targeted focus tracking during desktop switches.
- Removes continuous background thread that monitored focused windows every 200ms
- Replaces thread-unsafe Dictionary with ConcurrentDictionary for storing window focus state
- Adds explicit focus storage calls before desktop switching operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Source/Forms/AppForm.cs | Removes the call to start continuous focus monitoring thread |
| Source/App/App.cs | Implements thread-safe focus tracking with ConcurrentDictionary and targeted focus storage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| VDSwitchedSafe(); | ||
| } else { | ||
| //storeLastWinFocused(); | ||
| this.AppForm.BeginInvoke((Action)(() => { |
There was a problem hiding this comment.
BeginInvoke is asynchronous and may cause race conditions. The VDSwitchedSafe() call inside the invoke block could execute after subsequent desktop switches, leading to incorrect state updates. Consider using Invoke for synchronous execution or adding proper synchronization.
| this.AppForm.BeginInvoke((Action)(() => { | |
| this.AppForm.Invoke((Action)(() => { |
| if(Util.OS.IsWindow(lastWindowHandle)) { | ||
| IntPtr lastWindowHandle; | ||
| if (VDDToLastFocusedWin.TryGetValue(displayNumber, out lastWindowHandle)) { | ||
| System.Threading.Thread.Sleep(50); |
There was a problem hiding this comment.
Hard-coded sleep duration (50ms) is a magic number that makes the timing behavior unclear. Consider defining this as a named constant or making it configurable to improve maintainability.
|
@MadoScientist97 ignore the bot review - I just wanted to test this feature. Thanks for the PR - will test it this week. Best, -dan |
|
I do seem to be running into an issue in my work laptop where the focus gets lost, when switching back but it doesn't jump around to random workspaces anymore. Might work on this in the weekend |
2 participants
Hi @dankrusi I know this is like after months, but there were some race issues with how the window focus switching was maintained and handled, I have some potential fixes for that, should help improve stability.