Conversation
|
Hi @CyrilleB79 - could you test with this try build? |
There was a problem hiding this comment.
Pull request overview
This PR addresses a secure-desktop-only NVDA settings dialog instability/crash triggered by rapid category switching (issue #19634). It adds a small debounce layer around category changes when NVDA is running on the secure desktop to reduce rapid UI rebuilds that can lead to control tearing and crashes.
Changes:
- Added a 50ms debounce for settings category changes on the secure desktop via
wx.CallLater. - Introduced secure-desktop detection (
isRunningOnSecureDesktop) to conditionally apply the debounce logic. - Ensured debounce timer/state is cleaned up when the settings dialog is destroyed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class NVDASettingsDialog(MultiCategorySettingsDialog): | ||
| # Translators: This is the label for the NVDA settings dialog. | ||
| title = _("NVDA Settings") | ||
| _categoryChangeDebounceMs = 50 |
There was a problem hiding this comment.
This is a constant, right?
| _categoryChangeDebounceMs = 50 | |
| _CATEGORY_CHANGE_DEBOUNCE_MS: Final[int] = 50 |
| if isRunningOnSecureDesktop(): | ||
| # Secure desktop can cause issues with rapidly changing categories, | ||
| # so we debounce category changes to avoid this. (#19634) | ||
| self._pendingCategoryIndex = evt.GetIndex() |
There was a problem hiding this comment.
It might be nice to store the time of the last category change, so we only delay repeated rapid category changes. What do you think?
There was a problem hiding this comment.
I think it's worth creating a generic pattern in utils to do this. I'm going to create a decorator that makes a function delayed on a timer for subsequent rapid fires.
seanbudd
added
the
conceptApproved
|
I won't probably not be able to test before the end of this week. Though, since the PR is in Draft state, do I still need to test it now or should I wait for #19702? |
|
Either way is appreciated. Testing now confirms that debouncing fixes the problem, testing later confirms our next debouncing algorithm works still |
Link to issue number:
Blocked by #19702
Fixes #19634
Summary of the issue:
Windows throws errors when rapidly requesting a window handle on the secure desktop.
This cause visual tearing and a crash of NVDA.
Description of user facing changes:
Avoid crash and tearing of controls
Description of developer facing changes:
Add custom handling of changing settings categories for secure mode.
Description of development approach:
This pull request introduces a debounced category change mechanism in the
NVDASettingsDialogto improve stability and user experience, particularly when running on a secure desktop.NVDASettingsDialogwhen running on a secure desktop, using a timer to prevent rapid or duplicate category switches. This helps avoid potential issues with UI responsiveness or event handling in secure desktop environments. [1] [2]isRunningOnSecureDesktopfromutils.securityto determine when to apply the debounce logic.onCategoryChangemethod and new internal variables for better code clarity and maintainability. [1] [2]Destroymethod to prevent resource leaks.An alternative approach with error suppression was tried, but visual tearing still occurred.
https://github.com/nvaccess/nvda/compare/try-gui-crash-2?expand=1
Testing strategy:
Tested STR in #19634
Known issues with pull request:
Other instances may exist of rapidly requesting a window handle.
Should this be reported to wxWidgets or microsoft?
Should we create a generic monkey patch or something?
From basic investigating, the only real way to cause this could be through updating controls in the settings dialog, or the config profiles dialog.
Code Review Checklist: