-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Revert Auto-Type change that caused race condition #12738
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a race condition in the Auto-Type functionality by reverting a previous change and restructuring how the initial start delay is handled. The key insight is that the initial delay must be executed synchronously before grabbing the active window reference, rather than being queued as an action in the actions list.
Key Changes:
- Moved initial Auto-Type delay execution from the action queue to direct execution before window capture
- Introduced file-scoped constants
s_minWaitDelay(100ms) ands_maxWaitDelay(10s) for consistent delay validation - Updated all delay validation logic to use the new constants instead of local variables
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12738 +/- ##
========================================
Coverage 64.42% 64.42%
========================================
Files 378 378
Lines 39845 39844 -1
========================================
Hits 25667 25667
+ Misses 14178 14177 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
85f7276 to
f48fd6b
Compare
| int currentTypingDelay = qBound(0, config()->get(Config::AutoTypeDelay).toInt(), maxTypeDelay); | ||
| int cumulativeDelay = qBound(0, config()->get(Config::AutoTypeStartDelay).toInt(), maxWaitDelay); | ||
| // Take into account the initial delay which is added before any actions are performed | ||
| int cumulativeDelay = qBound(s_minWaitDelay, config()->get(Config::AutoTypeStartDelay).toInt(), s_maxWaitDelay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you store this in a variable above? Even if the calculation is unlikely to change, we should not have this complex expressions in two places when it must evaluate to exactly the same result always. Looks like a bug-to-be to me otherwise.
|
Oh, we're having this random test failure again: This seems to be happening when the second lapses during the generation of the test pattern matrix and the actual test execution: Line 157 in 967dc59
My recommended fix would be simply to remove the second from the pattern. This could still happen at the minute mark, but it's way less likely. |
Testing strategy
Type of change