Per foreground uia property change events#8742
Conversation
I assume you intended to say Windows 7 here? |
|
You're right. I've corrected the text.
|
| # Windows 10 RS5 provides new performance features for UI Automation including event coalescing and connection recovery. | ||
| # Enable all of these where available. | ||
| if isinstance(self.clientObject,IUIAutomation6): | ||
| if False: #isinstance(self.clientObject,IUIAutomation6): |
There was a problem hiding this comment.
Do you intend this to stay? I guess a decision about coalescing has to be made here:
- Either use event coalescing with global property change event registration or coalescing with per foreground events. I'd like to opt in for the latter
- I assume this change can still considered somewhat risky. For easier debugging, could it help to add some additional config parameters to the UIA section? Like: "useEventCoalescingWhenAvailable" (defaults to True) and useGlobalUIAPropertyChangeEvents (defaults to False). This way, we can easily change defaults if one or both of the features causes problems and we don't have to necessarily revert the whole pr.
| while True: | ||
| func=self.MTAThreadQueue.get() | ||
| if func: | ||
| func() |
There was a problem hiding this comment.
Could a try/except handler help here? While you use exception handling in the only func that is currently queued, an unexpected failure can just bring down the whole thread.
| # The system should forget the old event registration itself. | ||
| pass | ||
| self.currentForegroundUIAElement=pendingForegroundUIAElement | ||
| try: |
There was a problem hiding this comment.
Is event registration required if not handler.isNativeUIAElement(pendingForegroundUIAElement)?
There was a problem hiding this comment.
Yes. UIA can be implemented per-window. Just because the foreground window does not support UIA, does not mean that one of its descendants does not.
| try: | ||
| self.clientObject.AddPropertyChangedEventHandler(self.currentForegroundUIAElement,TreeScope_Subtree,self.baseCacheRequest,self,UIAPropertyIdsToNVDAEventNames.keys()) | ||
| except COMError: | ||
| log.error("Could not register for UIA property change events for new foreground") |
There was a problem hiding this comment.
Re me previous comment, I assume this is only problematic for native UIA windows?
There was a problem hiding this comment.
problematic for any descendant native UIA windows, yes. Again, the foreground window itself may not natively support UIA. However, the bridged UIA element we get for the foreground window in this case is still enough for UIA core to register propertyChange events for the subtree.
| # The old UIAElement died as the window was closed. | ||
| # The system should forget the old event registration itself. | ||
| pass | ||
| self.currentForegroundUIAElement=pendingForegroundUIAElement |
There was a problem hiding this comment.
It might make sense to set pendingForegroundUIAElement to None here. My concern with the current implementation is that if onForegroundChange fails multiple times in a row to set the pendingForegroundUIAElement, the pendingForegroundUIAElement still refers to an old element for which unregistration is attempted multiple times as well.
There was a problem hiding this comment.
Err... I would argue rather that currentForegroundUIAElement should be set to None if registration fails, not pendingForegroundUIAElement.
PendingForegroundUIAElement is always replaced with a new element before _foregroundChange is queued. CurrentForegroundUIAElement if it is left set will try to be unregistered multiple times.
There was a problem hiding this comment.
Yes, you're actually correct here. Sorry for that, but it at least triggered you to do the right thing here :)
|
Oops. That False change should not have been committed. In deed all try
builds I have given out previously did not have that change.
the plan is to leave event coalescing enabled along with per-foreground
propertyChange events.
|
…bugging purposes.
| newForeground=obj | ||
| api.setForegroundObject(newForeground) | ||
| import UIAHandler | ||
| if UIAHandler.isUIAAvailable: |
There was a problem hiding this comment.
Perhaps you want to check if UIAHandler.handler is not None. The current check will break NVDA if you call UIAHandler.terminate() for some reason.
…Handler's onForegroundChange as UIAHandler may be terminated.
…MTA thread, and ensure that propertyChange events are not unregistered from the same element multiple times if a new registration fails.
LeonarddeR
left a comment
There was a problem hiding this comment.
I'm ok with this, that is, if you believe that adding config parameters is not necessary at this point. I guess this just has to be tested in the wild to see whether it has any side effects we're currently not aware of.
|
I have confirmed that UIA propertyChange events are successfully registering per foreground change on Windows 7.
|
|
I've experienced like ten complete crashes of explorer.exe while rapidly navigating into folders since this pr is merged. It would be good to know whether there are more people suffering from this. This is Windows 10.0.17134.280. @michaelDCurran: Could we somehow check whether UIA elements died before throwing them at removePropertyChangedEventHandler or ADDPropertyChangedEventHandler? |
|
hi,
i don't suffer from this problem on windows 10 1809 last fast ring build.
17757
i don't suffer from this crashes
|
|
hi.
i am on 1803 with latest updates.
i don't see these crashes
2018-09-14 19:31 GMT+04:00, zstanecic <notifications@github.com>:
… hi,
i don't suffer from this problem on windows 10 1809 last fast ring build.
17757
i don't suffer from this crashes
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#8742 (comment)
--
with best regards beqa
|
|
It does require some very rapid entering and backspacing as well as quick typing to utilize first letter navigation in folders. |
|
But if you are just moving through folders, there would be no foreground
change.
When exactly does explorer crash?
Once you alt+tab?
|
|
There is no alt+tab involved at all. It mostly crashes when I open a folder and start typing pretty quickly after that: Also, there are quite a lot of debug warnings in the log:
|
|
hi all.
i can also confirm crash of file explorer but there is another scenario.
i have openned two windows with two folders and have 20 or more files
in one and need to cut/paste to second folder.
i am pasting files one by one,, not all at once.
after pasting some of these files, file explorer unexpectedly crashes.
it is happening on windows 10 1803 and 1809
2018-09-14 23:23 GMT+04:00, Leonard de Ruijter <notifications@github.com>:
… There is no alt+tab involved at all. It mostly crashes when I open a folder
and start typing pretty quickly after that:
Also, there are quite a lot of debug warnings in the log:
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#8742 (comment)
--
with best regards beqa
|
|
I am reverting this pr for now. Annoyingly so far I have not been able to reproduce the crash myself. |
This reverts commit 4730a9f.
|
try to reproduce what i wrote.
cut from one and paste two second folder 20-30 small files one by one
and you will see.
2018-09-17 10:06 GMT+04:00, Michael Curran <notifications@github.com>:
… I am reverting this pr for now. Annoyingly so far I have not been able to
reproduce the crash myself.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#8742 (comment)
--
with best regards beqa
|
|
Given the recent concerns about UIA performance issues, now we are at Python 3 and several Windows 10 releases ahead, would it be worthwile to reconsider this pull request? Cc @jcsteh @feerrenrut |
|
While I think this is generally a good idea if it can be done reliably, note that this would not have fixed the issues with 1Password (#10508). In that case, the events were being fired by 1Password, which was in the foreground. Fixing that case generically requires NVDA to only listen to property change events for the focus, the focus ancestry and special cases like progress bars. |
|
For whatever it's worth, I tried re-introducing this PR to latest |
Link to issue number:
Fixes #7345
Summary of the issue:
Sometimes UI Automation focus events stop firing across the Operating System, if a background app is currently firing a lot of MSAA events and at the same time is slow to respond to window messages. An example would be trying to use File Explorer while another app is showing a progress bar while processing audio.
Description of how this pull request fixes the issue:
NVDA now registers for UIA propertyChange events per foreground change, rather than globally at NVDA startup.
Testing performed:
Multiple users have been testing try build of this code on both Windows 7 and Windows 10. They have reported that the UIA focus events no longer disappear, and in some cases general performance has been improved.
Known issues with pull request:
None known so far.
Change log entry:
Bug fixes:
NVDA no longer fails to track focus in File Explorer and other applications using UI Automation when another app is busy (such as batch processing audio).