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

Navigating by line in Microsoft Edge is now up to 3x faster in the Windows 10 Creaters Update #6994

Merged
merged 10 commits into from Apr 12, 2017

Conversation

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Mar 21, 2017

Speed improvements gained by making use of UI Automation cache requests when using UIA NVDAObjects to generate controlFields, by making use of the new IUIAutomationTextRange3 caching APIs, and by leveraging the fact that Edge now better conforms to a standard UI Automation text implementation thus the inefficient Edge-specific content fetching algorithm is no longer needed.

…w for instantiating a UIA NVDAObject with a UIAElement containing prefetched UI Automation property values via a UI Automation CacheRequest, available for the remainder of the core cycle the NVDAObject was instantiated in. This is useful when instantiating an NVDAObject purely to generate a control field, where the caller knows what properties are needed.

All code within UIA NVDAObject and its subclasses should use the new _getUIACacheablePropertyValue method to fetch UI Automation properties as this will check the cache first.

Make use of the new Win 10 RS2 bulk-fetch UIA APIs available on IUIAutomationTextRange3 such as getChildrenBuildCache, getEnclosingElementBuildCache and getAttributeValues. When not available, compatibility code falls back to the older APIs, providing the same cached results, but over multiple calls.

 UIATextInfo._getTextWithFieldsForUIARange and related content fetching code has been updated to use cacheRequests where possible. E.g. getChildrenBuildCache, getEnclosingElementBuildCache etc.
…use it in Windows builds less 15048. Windows builds above this now can use the generic UI Automation textWithFields code. Along with the added caching, these changes provide up to a 3x speed-up in reading lines in Microsoft Edge.
@michaelDCurran michaelDCurran requested a review from jcsteh Mar 21, 2017
@jcsteh
jcsteh approved these changes Mar 21, 2017
@@ -347,8 +429,8 @@ def _getTextWithFieldsForUIARange(self,rootElement,textRange,formatConfig,includ
@type alwaysWalkAncestors: bool
@param recurseChildren: If true, this function will be recursively called for each child of the given text range, clipped to the bounds of this text range. Formatted text between the children will also be yielded. If false, only formatted text will be yielded.
@type recurseChildren: bool
@param _rootElementRange: Optimization argument: the actual text range for the root element, as it is usually already known when making recursive calls.
@type rootElementRange: L{UIAHandler.IUIAutomationTextRange}
@param _rootElementClipped: Indicates of textRange represents all of the given rootElement, or is clipped at the start or end.

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

"Indicates of"? Do you mean "Indicates if"?

@@ -557,6 +638,46 @@ def updateSelection(self):

class UIA(Window):

def _get__coreCycleUIAPropertyCacheElementCache(self):
"""A dictionary per core cycle that is ready to map UIA property IDs to UIAElements with that property already cached."""

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Can you add a brief explanation of why there might be multiple cache elements; e.g. the initial cache for ControlFields and the cache used to fetch states efficiently?


def _getUIACacheablePropertyValue(self,ID,ignoreDefault=False,onlyCached=False):
"""
Fetches the value for a UI Automation property from an element cache available in this core cycle. If not cached and L{onlyCache} is False then a new value will be fetched.

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

typo: onlyCache should be onlyCached.

Fetches the value for a UI Automation property from an element cache available in this core cycle. If not cached and L{onlyCache} is False then a new value will be fetched.
"""
elementCache=self._coreCycleUIAPropertyCacheElementCache
# If we have a UIAElement who's own cache contains the property, fetch the value from there

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

who's -> whose

else:
raise ValueError("UIA property value not cached")
# cache and return the value
#valueCache[key]=value

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Looks like you had a value cache at some point. This commented line needs to be removed and the comment above it adjusted.

acceleratorKey = self._getUIACacheablePropertyValue(UIAHandler.UIA_AcceleratorKeyPropertyId)
# Same case as access key.
if acceleratorKey:
shortcuts.append(acceleratorKey)
pass

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Not necessary any more.

@@ -66,7 +69,7 @@ def _get_isCollapsed(self):
# Therefore class a range as collapsed if it has no text
return not bool(self.text)

def getTextWithFields(self,formatConfig=None):
def old_getTextWithFields(self,formatConfig=None):

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Is this no longer needed? If not, it should be removed.

@@ -18,6 +18,7 @@

from comtypes.gen.UIAutomationClient import *


This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Extraneous blank line.

@@ -119,3 +120,71 @@ def iterUIARangeByUnit(rangeObj,unit):
if tempRange.CompareEndpoints(UIAHandler.TextPatternRangeEndpoint_End,rangeObj,UIAHandler.TextPatternRangeEndpoint_End)<0:
tempRange.MoveEndpointByRange(UIAHandler.TextPatternRangeEndpoint_End,rangeObj,UIAHandler.TextPatternRangeEndpoint_End)
yield tempRange.clone()

def getEnclosingElementWithCacheFromUIATextRange(textRange,cacheRequest):

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

I think a docstring would be good here. Only has to point out that it uses getEnclosingElementBuildCache or falls back to getting the element and then building the cache if getEnclosingElementBuildCache is not supported.

e=e.buildUpdatedCache(self._cacheRequest)
return e

def getChildrenWithCacheFromUIATextRange(textRange,cacheRequest):

This comment has been minimized.

@jcsteh

jcsteh Mar 21, 2017
Contributor

Docstring as above.

michaelDCurran added a commit that referenced this pull request Mar 21, 2017
@josephsl
Copy link
Collaborator

@josephsl josephsl commented Mar 22, 2017

Hi,

Possibly related to this PR: when attempting to retrieve obj.location in places such as Skype Preview's Download file button (when someone sends a file) and reported by folks using Audio Themes 3D add-on, obj.location traceback says:

Traceback (most recent call last):
File "", line 1, in
File "baseObject.pyc", line 34, in get
File "baseObject.pyc", line 110, in getPropertyViaCache
File "NVDAObjects\UIA_init
.pyc", line 1147, in _get_location
AttributeError: 'tuple' object has no attribute 'left'

Thanks.

michaelDCurran added a commit that referenced this pull request Mar 22, 2017
@michaelDCurran
Copy link
Member Author

@michaelDCurran michaelDCurran commented Mar 22, 2017

…than currentBoundingRectangle returns a tuple of left,top,width,height, rather than a specific rectangle object with members of left,top,right and bottom.
@michaelDCurran michaelDCurran force-pushed the edgeSpeedup branch from 7c61bef to 46cd6e6 Mar 22, 2017
michaelDCurran added a commit that referenced this pull request Mar 22, 2017
@michaelDCurran
Copy link
Member Author

@michaelDCurran michaelDCurran commented Mar 22, 2017

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Mar 23, 2017

As reported in this nvda-devel thread, this breaks if UIA is disabled. You can easily test this by setting config.conf["UIA"]["enabled"] = False, saving config and restarting.

@Brian1Gaff
Copy link

@Brian1Gaff Brian1Gaff commented Mar 23, 2017

@michaelDCurran michaelDCurran requested a review from jcsteh Mar 23, 2017
michaelDCurran added a commit that referenced this pull request Mar 24, 2017
…ure version of Windows, either in cache requests, or provide defaults for them. Therefore: catch COMError when trhing to add properties to a controlFieldCacheRequest (E.g. sizeInSet on Windows 7), and also reintroduce try blocks around some of the property fetches in UIA NVDAObject's states property, much resembling what it used to be, though still using _getUIACacheablePropertyValue.

Fixes #7013
@michaelDCurran
Copy link
Member Author

@michaelDCurran michaelDCurran commented Mar 27, 2017

@jcsteh: another review please. Fixes #7013

Copy link
Contributor

@jcsteh jcsteh left a comment

I follow the motivation behind a30c217, but I'm a bit confused about some of the changes. Why remove onlyCached from all of these property queries? Doesn't that mean we will try to make queries for them all? I guess those queries will fail at the client library before they even get executed? If so, is there any case where onlyCached is useful any more? If not, perhaps it should be removed to avoid confusion.

@jcsteh
jcsteh approved these changes Mar 27, 2017
Copy link
Contributor

@jcsteh jcsteh left a comment

As discussed via voice, approving with the removal of the onlyCached argument to _getUIACacheablePropertyValue.

michaelDCurran added a commit that referenced this pull request Mar 27, 2017
@michaelDCurran michaelDCurran merged commit c1799ee into master Apr 12, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.2 milestone Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants