UIA handler: work directly with UIA interfaces introduced in Windows 8 and later#15823
Conversation
| HorizontalTextAlignment_Left = 0 | ||
| HorizontalTextAlignment_Centered = 1 | ||
| HorizontalTextAlignment_Right = 2 | ||
| HorizontalTextAlignment_Justified = 3 |
There was a problem hiding this comment.
If you are touching this part of code, please make it in an enum
There was a problem hiding this comment.
This will break NVDAObjects.UIA text alignment code, so I would be careful with this one.
There was a problem hiding this comment.
I can add a "getattr" function at the end in case we do find add-ons using UIAHandler.Horizontal* attributes (and perhaps a good test case for structured pattern matching). Will add an entry to changelog file.
| # C901: 'IUIAutomationEventHandler_HandleAutomationEvent' is too complex | ||
| # Note: when working on IUIAutomationEventHandler_HandleAutomationEvent, look for opportunities to simplify | ||
| # and move logic out into smaller helper functions. | ||
| def IUIAutomationEventHandler_HandleAutomationEvent(self, sender, eventID): # noqa: C901 |
| def IUIAutomationFocusChangedEventHandler_HandleFocusChangedEvent(self,sender): | ||
| # This is updated no matter if this is a native element, the window is UIA blacklisted by NVDA, | ||
| # or the element is proxied from MSAA | ||
| lastFocusedUIAElement = None |
| # C901: 'IUIAutomationFocusChangedEventHandler_HandleFocusChangedEvent' is too complex | ||
| # Note: when working on IUIAutomationFocusChangedEventHandler_HandleFocusChangedEvent, | ||
| # look for opportunities to simplify and move logic out into smaller helper functions. | ||
| def IUIAutomationFocusChangedEventHandler_HandleFocusChangedEvent(self, sender): # noqa: C901 |
| # C901: 'IUIAutomationPropertyChangedEventHandler_HandlePropertyChangedEvent' is too complex | ||
| # Note: when working on IUIAutomationPropertyChangedEventHandler_HandlePropertyChangedEvent, | ||
| # look for opportunities to simplify and move logic out into smaller helper functions. | ||
| def IUIAutomationPropertyChangedEventHandler_HandlePropertyChangedEvent( # noqa: C901 |
| self.MTAThreadInitEvent = threading.Event() | ||
| self.MTAThreadQueue = Queue() | ||
| self.MTAThreadInitException=None | ||
| self.MTAThreadInitException = None |
There was a problem hiding this comment.
This is an exception, and a Stack Overflow post says we can't use typing to annotate exceptions (at this time) unless NVDA code base has found a way to do so which I may not be aware.
There was a problem hiding this comment.
What's wrong with
| self.MTAThreadInitException = None | |
| self.MTAThreadInitException: Exception | None = None |
| self.MTAThreadInitEvent = threading.Event() | ||
| self.MTAThreadQueue = Queue() | ||
| self.MTAThreadInitException=None | ||
| self.MTAThreadInitException = None |
There was a problem hiding this comment.
What's wrong with
| self.MTAThreadInitException = None | |
| self.MTAThreadInitException: Exception | None = None |
| HorizontalTextAlignment_Justified=3 | ||
|
|
||
| class HorizontalTextAlignment(enum.IntEnum): | ||
| Left = 0 |
There was a problem hiding this comment.
Please use Screaming Snake Case.
| Left = 0 | |
| LEFT = 0 |
etc.
|
|
||
| def __init__(self): | ||
| super(UIAHandler,self).__init__() | ||
| super(UIAHandler, self).__init__() |
There was a problem hiding this comment.
| super(UIAHandler, self).__init__() | |
| super().__init__() |
| #Wait for the MTA thread to die (while still message pumping) | ||
| if windll.user32.MsgWaitForMultipleObjects(1,byref(MTAThreadHandle),False,200,0)!=0: | ||
| # Wait for the MTA thread to die (while still message pumping) | ||
| if windll.user32.MsgWaitForMultipleObjects(1, byref(MTAThreadHandle), False, 200, 0) != 0: |
There was a problem hiding this comment.
Could you add the walrus operator to find out what the result is and log it appropriately while at it?
I'm also pretty sure the possible results for MsgWaitForMultipleObjects have constants somewhere.
There was a problem hiding this comment.
Done - this is one of those good use cases of assignment expression (:=).
| oledll.ole32.CoInitializeEx(None, comtypes.COINIT_MULTITHREADED) | ||
| self.clientObject = CoCreateInstance( | ||
| UIA.CUIAutomation8._reg_clsid_, | ||
| interface=UIA.IUIAutomation, |
There was a problem hiding this comment.
| interface=UIA.IUIAutomation, | |
| interface=UIA.CUIAutomation8._com_interfaces_[0], |
There was a problem hiding this comment.
This will start with IUIA2 (Windows 8), and since minimum Windows version is 8.1, we can actually start with IUIA3 (index = 1). Wil add a comment to that effect.
|
|
||
| def __getattr__(attrName: str) -> Any: | ||
| """Module level `__getattr__` used to preserve backward compatibility.""" | ||
| if attrName == "HorizontalTextAlignment_Left" and NVDAState._allowDeprecatedAPI(): |
There was a problem hiding this comment.
I would personally do the if check for deprecated API first and then use a match statement for the constants.
There was a problem hiding this comment.
Agreed - this makes the code more readable.
lukaszgo1
left a comment
There was a problem hiding this comment.
I'm not sure if something has changed during the time when I was not following NVDA's development last year, but in the past we used not to accept mass linting changes like this (for one example see concluding comments of #12773). Currently when reviewing this code it is not as easy as it should be to say which part of the change is strictly necessary to address the main problem and which is purely code reformat. In fact only around 10% of the patch here has any relation to using Windows 8.1 interfaces. Has it been considered to split this PR into separate ones for linting, conversion into an enumeration and one for the main problem? At the minimum it would be good not to squash merge this, but split into the 3 (or more) coherent commits, so that blaming is not negatively affected. It would also be nice to decide and document if changes like this should be even done in the first place. My initial intuition is that they should not be, since we have discussion about more or less auto reformatting in progress, many other projects explicitly state that mass linting like this should not be smuggled together with unrelated code changes, and in general they make reviewing much more time consuming. This is not to say that I do think consistent code style is unimportant, just that it should be applied in an organized way and that code patches whose main point is not linting, should not consist of purely linting changes in almost their entirety.
|
Does this mean that an older piece of software which works in Windows8 or later almost by accident, may then become unusable?
Brian
…--
***@***.***
Sent via blueyonder.(Virgin media)
Please address personal E-mail to:-
***@***.***, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From:Joseph Lee
To:nvaccess/nvda
Cc:Push
Sent: Friday, November 24, 2023 6:59 AM
Subject: Re: [nvaccess/nvda] UIA handler: work directly with UIA interfaces introduced in Windows 8 and later (PR #15823)
@josephsl pushed 3 commits.
• e8848ac UIA handler: deprecate HorizontalTextAlignment variables, replaced with integer enum.
• 625d1f6 Whats' new: add note on UIA handler horizontal text alignment constnats to enum conversion.
• f612f78 UIA handler: add type hints.
—
View it on Gather or unsubscribe.
You are receiving this because you are subscribed to this thread.
--
***@***.***
Sent via blueyonder.(Virgin media)
Please address personal E-mail to:-
***@***.***, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From: Łukasz Golonka
To: nvaccess/nvda
Cc: Subscribed
Sent: Friday, November 24, 2023 6:19 PM
Subject: Re: [nvaccess/nvda] UIA handler: work directly with UIA interfaces introduced in Windows 8 and later (PR #15823)
@lukaszgo1 requested changes on this pull request.
I'm not sure if something has changed during the time when I was not following NVDA's development last year, but in the past we used not to accept mass linting changes like this (for one example see concluding comments of #12773). Currently when reviewing this code it is not as easy as it should be to say which part of the change is strictly necessary to address the main problem and which is purely code reformat. In fact only around 10% of the patch here has any relation to using Windows 8.1 interfaces. Has it been considered to split this PR into separate ones for linting, conversion into an enumeration and one for the main problem? At the minimum it would be good not to squash merge this, but split into the 3 (or more) coherent commits, so that blaming is not negatively affected. It would also be nice to decide and document if changes like this should be even done in the first place. My initial intuition is that they should not be, since we have discussion about more or less auto reformatting in progress, many other projects explicitly state that mass linting like this should not be smuggled together with unrelated code changes, and in general they make reviewing much more time consuming. This is not to say that I do think consistent code style is unimportant, just that it should be applied in an organized way and that code patches whose main point is not linting, should not consist of purely linting changes in almost their entirety.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
I have not read the code of this PR. But I totally second what @lukaszgo1 writes. If we go through code reformatting, it would be nice to have it all in a single independent commit that we can exclude with |
|
Hi, My thinking is that 2024.1 development milestone is a good opportunity to lint the surrounding code and the module. But if people believe that splitting the module-level lint is better, then I'm willing to remove lint commits from this PR. Thanks. |
|
Yes, please do split out the lint where its not relevant to this fix, it makes it difficult to tell what the code changes are in this PR. |
…cking for Windows 8 interface. As Windows 8.1 is the minimum Windows version, there is no need to check for Windows 8 UIA interface. Therefore traverse the interfaces backwards to locate the correct IUIAutomation interface (3/8.1, 4/10 1607, 5/10 1709, 6/10 1809).
Directly instantiate UIA 8 interface as Windows 8.1 is the minimum Windows version.
…ion interface version
Note from Leonard de Ruijter: start with the first supported interface in IUIAutomation8 interfaces. As NVDA supports Windows 8.1 and later, start with IUIAutomation3 (second item in the interfaces list).
be1434f to
c46eff9
Compare
Link to issue number:
None
Summary of the issue:
NVDA checks for Windows 7/8 UIA interfaces at startup when the minimum version is Windows 8.1.
Description of user facing changes
None
Description of development approach
At startup, NVDA's UIA handler thread will instantiate Widnows 8 UIA interface direclty as opposed tr doing it behind a try/except block. This also removes the check for UIA8 flag when instantiating later UIA interfaces. As part of this work, UIA handler main file was linted and some functions marked as too complex (according to Flake8).
Testing strategy:
Manual testing: making sure UIA works as expected (at least on Windows 11).
Known issues with pull request:
None
Code Review Checklist: