Skip to content
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

Introduce IUIAutomation5 interface and ability to announce notifications in Windows 10 Fall Creators Update and later #8045

Merged
merged 13 commits into from Mar 28, 2018

Conversation

josephsl
Copy link
Collaborator

@josephsl josephsl commented Feb 28, 2018

Link to issue number:

#7984, #8009

Summary of the issue:

Introduce IUIAutomation5 interface and ability to announce notifications in Windows 10 Fall Creators Update an later

Description of how this pull request fixes the issue:

In Windows 10 Fall Creators Update and later, apps can inform screen readers to announce specific notification text. This is done via IUIAutomationNotificationEventHandler::HandleNotificationEvent, which is part of IUIAutomation5 interface. Since NVDA only uses IUIAutomation3, update UIA subsystem to use IUIAutomation5. Also, introduce ability to use more recent IUIAutomation interfaces on various Windows 10 releases (such as IUIAutomation4 in Anniversary Update/build 14393).

Testing performed:

Testing done via Windows 10 App Essentials add-on: using recent versions of various app (such as Calculator, Microsoft Store and others) that uses this new event to announce texts.

Known issues with pull request:

None so far.

Change log entry:

New feature: In Windows 10 Fall Creators Update and later, NVDA can announce notifications from apps such as Calculator and Windows Store.

…ported. Re nvaccess#8009.

Also includes nvaccess#7984: in Windows 10 Version 1709 (Fall Creators Update), UIA notification event is introduced for the benefit of screen readers so they can announct needed text. This is part of UIA 5 interface, which NVDA does not support yet.
The UIA notification event accepts additional parameters such as notification kind, notification processing constant and activit ID. This is useful for controlling how things should be announced and when. For now, the handler will do nothing.
Also, when initializing UIA handler, use UIA 5, and then fall back to UIA 4.
…or additional kwargs for event execution. Re nvaccess#8009.

Treat UIA notification event just like others except for two things:
* A new UIA_notification event will be fired.
* The new event will populate kwargs based on arguments for the actual event handling function (sender, displayString, etc.). This means app modules, global plugins, objects and others must include these kwargs when defining this event.
…ndows release. Re nvaccess#8009.

A new function named getIUIAInterface (along with a private companion for Windows 10) is introduced to obtain highest supported IUIAutomation interface. Depending on Windows release, it'll return various interfaces. In case of Windows 10, a companion function will return appropriate interface based on build ranges.
nvaccess#7984.

If IUIAutomation5 is in use, add notificaiton event handler, otherwise COM error is thrown.
Also, instead of relying solely on Python MRO to retrieve UIA interface, save the string somewhere for reference purposes and for future use.
…vaccess#8009.

The base implementation for UIA notification handler will just speak whatever notification it receives. Subclasses are more than welcome to add further processing routines.
@LeonarddeR
Copy link
Collaborator

Hey Joseph,

  1. It seems you defined the getIUIAInterface function, but you never use it in your code below.
  2. As noted on Gitter, I'm honestly not a fan of the build version checking, as it does not work when running from source. Furthermore, I'm not convinced of whether it is significantly faster than just querying interfaces from 5 up to 2. Some timing on my end revealed that querying interfaces is actually very fast.

@michaelDCurran
Copy link
Member

michaelDCurran commented Feb 28, 2018 via email

@michaelDCurran
Copy link
Member

Do you know if this causes any double speaking where Microsoft may have fired both liveRegionChange and notification for the same thing?

@josephsl
Copy link
Collaborator Author

josephsl commented Feb 28, 2018 via email

@josephsl
Copy link
Collaborator Author

Hi,

@LeonarddeR: the Win10 version is the one to be used from binary builds and eventually from source once we move to Python 3. The generic version is meant for add-ons and to show that NVDA still supports Windows 7 SP1. The reason for this design is to keep it compatible with what we have at the moment: falling back to IUIAutomation2 if newer versions are not available. As @michaelDCurran pointed out, this is meant to be used at startup (both for Core and add-ons).

Thanks.

@michaelDCurran
Copy link
Member

michaelDCurran commented Feb 28, 2018 via email

@josephsl
Copy link
Collaborator Author

Hi,

In the end, the getIUIAInterface function won't be added, as a for loop will be used to try various versions (modifying specs accordingly).

Thanks.

…8009.

Comments from @LeonarddeR and @michaelDCurran: build ranges aren't useful, subsequently proven to be true. As an alternative, query interfaces from newest to oldest, which does allow NVDA to use IUIAutomation5 interface from source.
Also, this means getIUIAInterface function is no longer necessary.
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't ask for my review, but I'm trying to update my UIA knowledge, so that's why I looked into this.

@@ -1368,6 +1369,16 @@ def event_UIA_systemAlert(self):
# Ideally, we wouldn't use getBrailleTextForProperties directly.
braille.handler.message(braille.getBrailleTextForProperties(name=self.name, role=self.role))

def event_UIA_notification(self, sender=None, notificationKind=None, notificationProcessing=None, displayString=None, activityId=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could leave out the sender kwarg. See my comment below.

return
import NVDAObjects.UIA
obj=NVDAObjects.UIA.UIA(UIAElement=sender)
eventHandler.queueEvent("UIA_notification",obj, sender=sender, notificationKind=NotificationKind, notificationProcessing=NotificationProcessing, displayString=displayString, activityId=activityId)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the sender is equal to obj.UIAElement, I think it is a redundant kwarg. For consistency reasons, if one whould like to access the UIAElement for this event within a subclass, people can use the UIAElement property on the object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @LeonarddeR here. Sender should not be explicitly provided to the notification event as it is redundant.

self.clientObject=self.clientObject.QueryInterface(IUIAutomation2)
log.info("UIAutomation: %s"%self.clientObject.__class__.__mro__[1].__name__)
# #8009: use appropriate interface based on highest supported interface.
for interface in (IUIAutomation5, IUIAutomation4, IUIAutomation3, IUIAutomation2):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be put these interfaces in a constant?

except COMError:
pass
# Have this handy because we want to add things corresponding to the interface version.
IUIAVersion = self.clientObject.__class__.__mro__[1].__name__
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you loop through these interfaces (which I think is a good thing to do), you might as well do something like this, avoiding digging into the mro:

for interface in interfaces:
    QueryInterface
except:
    pass # Or continue? I guess they are identical in result here
else:
    IUIAVersion = interface.__name__
    ...

Since you just left the loop, you can even leave out the else statement, but using an else for the try statement looks somewhat cleaner to me. It's a matter of preference, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UiaVersion would also have to be set to IUIAutomation when isUIA8 is false. if you do it the way @LeonarddeR suggests

@@ -206,6 +211,9 @@ def MTAThreadFunc(self):
self.clientObject.AddPropertyChangedEventHandler(self.rootElement,TreeScope_Subtree,self.baseCacheRequest,self,UIAPropertyIdsToNVDAEventNames.keys())
for x in UIAEventIdsToNVDAEventNames.iterkeys():
self.clientObject.addAutomationEventHandler(x,self.rootElement,TreeScope_Subtree,self.baseCacheRequest,self)
# #7984: add support for notification event (IUIAutomation5, part of Windows 10 build 16299 and later).
if IUIAVersion >= "IUIAutomation5":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, even though Python uses lexicographical comparison for strings and therefore this is code that does the write thing, I'd feel safer if you'd avoid this. As @michaelDCurran pointed out in #8045 (comment), all UIA interfaces inherit from the one before, so you could do an instance check on the client object for IUIAutomation5.

self.clientObject=self.clientObject.QueryInterface(interface)
break
except COMError:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an else on the for loop that raises RuntimeError("No UIAutomation client interface supported")

except COMError:
pass
# Have this handy because we want to add things corresponding to the interface version.
IUIAVersion = self.clientObject.__class__.__mro__[1].__name__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UiaVersion would also have to be set to IUIAutomation when isUIA8 is false. if you do it the way @LeonarddeR suggests

@josephsl
Copy link
Collaborator Author

josephsl commented Mar 1, 2018

Hi,

In the end, isinstance checking will be done. I don't think an "else" is necessary except if we need to break out of the loop with that method.

Additional testing done by running the launcher (based on source code commits) on a Windows 8.1 system, and things work as expected. Thanks.

@Brian1Gaff
Copy link

Brian1Gaff commented Mar 1, 2018 via email

@josephsl
Copy link
Collaborator Author

josephsl commented Mar 1, 2018

Hi,

If the new interface includes useful features that screen reader users need to see/hear, then yes, we need to add support for it.

Thanks.

return
import NVDAObjects.UIA
obj=NVDAObjects.UIA.UIA(UIAElement=sender)
eventHandler.queueEvent("UIA_notification",obj, sender=sender, notificationKind=NotificationKind, notificationProcessing=NotificationProcessing, displayString=displayString, activityId=activityId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @LeonarddeR here. Sender should not be explicitly provided to the notification event as it is redundant.

Reviewed by @michaelDCurran (NV Access): sender argument is redundant.
michaelDCurran added a commit that referenced this pull request Mar 5, 2018
# Conflicts:
#	source/NVDAObjects/UIA/__init__.py
#	source/_UIAHandler.py
michaelDCurran added a commit that referenced this pull request Mar 11, 2018
@michaelDCurran michaelDCurran merged commit 35c0840 into nvaccess:master Mar 28, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.2 milestone Mar 28, 2018
@josephsl josephsl deleted the iuiautomation5 branch September 16, 2019 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants