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

Lazily register for custom UIA properties and annotations #14248

Merged
merged 5 commits into from
Jan 18, 2023

Conversation

lukaszgo1
Copy link
Contributor

@lukaszgo1 lukaszgo1 commented Oct 13, 2022

Link to issue number:

None. Related to #12971 and #12843 comment

Summary of the issue:

Registering for custom UIA annotations and properties can only be done after NVDAHelper is initialized. In normal usage this is not a problem, however since registration occurs on import it is impossible to write unit tests or generate developer documentation for anything which imports from NVDAObjects.UIA. The error when generating developer documentation is as follows:

WARNING: autodoc: failed to import module 'MSHTML' from module 'NVDAObjects.IAccessible'; the following exception was raised:
Traceback (most recent call last):
  File "D:\my_repos\nvda\.venv\lib\site-packages\sphinx\ext\autodoc\importer.py", line 58, in import_module
    return importlib.import_module(modname)
  File "C:\Python37\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "D:\my_repos\nvda\source\NVDAObjects\IAccessible\MSHTML.py", line 29, in <module>
    from NVDAObjects.UIA import UIA, UIATextInfo
  File "D:\my_repos\nvda\source\NVDAObjects\UIA\__init__.py", line 962, in <module>
    class UIA(Window):
  File "D:\my_repos\nvda\source\NVDAObjects\UIA\__init__.py", line 963, in UIA
    _UIACustomProps = UIAHandler.customProps.CustomPropertiesCommon.get()
  File "D:\my_repos\nvda\source\UIAHandler\customProps.py", line 75, in get
    cls._instance = cls()
  File "D:\my_repos\nvda\source\UIAHandler\customProps.py", line 83, in __init__
    uiaType=UIAutomationType.INT,
  File "<string>", line 6, in __init__
  File "D:\my_repos\nvda\source\UIAHandler\customProps.py", line 54, in __post_init__
    self.id = NVDAHelper.localLib.registerUIAProperty(
AttributeError: 'NoneType' object has no attribute 'registerUIAProperty'

Description of user facing changes

This should affect only developers - classes holding custom UIA properties and annotations are no longer singletons.

Description of development approach

Rather than making UIAHandler.customAnnotations.CustomAnnotationTypesCommon, UIAHandler.customProps.CustomPropertiesCommon and the Excel specific annotations and properties singletons, we now register for custom properties and annotations first time they're needed rather than on import. To avoid unnecessary interactions with UIA registered id's are cached for lifetime of NVDA. While at it I've also moved imports below the module level docstring since that is the more standard place.

Testing strategy:

  • Compiled developer documentation - made sure that no errors from not initialized NVDAHelper are shown
  • Smoke tested custom UIA properties by ensuring that item positions can be reported in Windows Explorer

Known issues with pull request:

None known

Change log entries:

For Developers:

  • UIAHandler.customAnnotations.CustomAnnotationTypesCommon, UIAHandler.customProps.CustomPropertiesCommon, NVDAObjects.UIA.excel.ExcelCustomProperties and NVDAObjects.UIA.excel.ExcelCustomAnnotationTypes are no longer singletons - their get method has been removed.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@lukaszgo1 lukaszgo1 requested a review from a team as a code owner October 13, 2022 22:07
@lukaszgo1 lukaszgo1 requested a review from seanbudd October 13, 2022 22:07
@AppVeyorBot
Copy link

See test results for failed build of commit 4e089fd1eb

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

These changes generally look good.
I'm concerned that removing get is an API breakage.
If this is an API breakage, please follow the steps in devDocs/deprecations.md to retain the deprecated methods if possible,
It may be possible to make the get functions return the class itself?

@seanbudd seanbudd marked this pull request as draft October 14, 2022 01:45
@seanbudd
Copy link
Member

Rather than initializing these implicitly, would it be possible to explicitly initialize these through an initializer being called?

@seanbudd
Copy link
Member

Is this PR abandoned?
Just bumping as it contains proposed API breaking changes, and the cut off for 2023.1 is approaching.

@seanbudd seanbudd added this to the 2023.1 milestone Jan 16, 2023
@seanbudd seanbudd marked this pull request as ready for review January 18, 2023 03:45
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

I think this is fine to be merged, now that we have reached a breaking release.

@seanbudd seanbudd merged commit b874768 into nvaccess:master Jan 18, 2023
@lukaszgo1 lukaszgo1 deleted the uiaCustomLazyRegistration branch February 21, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants