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

UI Automation in Windows Console: rename is21H1Plus to isImprovedTextRangeAvailable #12094

Merged
merged 3 commits into from
May 17, 2021

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Feb 24, 2021

Link to issue number:

microsoft/terminal#9239

Summary of the issue:

Windows 10 21H1 has been released to insiders with significantly reduced scope (i.e. excluding the UIA changes it was originally set to include), making our internal names misleading. Additionally, there are plans to "undock" conhost from Windows, removing the guarantee that a given Windows version will run a particular version of the console (it's technically already possible to run newer console on older Windows, I do it for testing).

Description of how this pull request fixes the issue:

Renames:

  • NVDAObjects.UIA.winConsoleUIA.is21H1Plus -> NVDAObjects.UIA.winConsoleUIA.isImprovedTextRangeAvailable
  • NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfo -> NVDAObjects.UIA.winConsoleUIA.ConsoleUIATextInfo (Start class name with upper case)
  • NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfoPre21H1 -> NVDAObjects.UIA.winConsoleUIA.ConsoleUIATextInfoWorkaroundEndInclusive (because much of the implementation deals with the fact that before Refactor UiaTextRange For Improved Navigation and Reliability microsoft/terminal#4018 text ranges had both end points inclusive, necessitating workarounds for expand, collapse, compareEndPoints, setEndPoint, etc)

Also moves the text override to the workaround textInfo as #10036 is no longer reproducible in new console.

Testing performed:

Verified that console still works as intended. These names were used internally only.

Known issues with pull request:

None.

Change log entry:

Changes for developers:

- ``NVDAObjects.UIA.winConsoleUIA.is21H1Plus`` renamed ``NVDAObjects.UIA.winConsoleUIA.isImprovedTextRangeAvailable``. (#12094)
- ``NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfo`` renamed to start class name with upper case. (#12094)
- ``NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfoPre21H1`` renamed ``NVDAObjects.UIA.winConsoleUIA.ConsoleUIATextInfoWorkaroundEndInclusive``. (#12094)
  - The implementation works around both end points being inclusive (in text ranges) before microsoft/terminal#4018
  - Workarounds for ``expand``, ``collapse``, ``compareEndPoints``, ``setEndPoint``, etc

Code Review Checklist:

This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x: [ ] becomes [x].
You can also check the checkboxes after the PR is created.

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@codeofdusk
Copy link
Contributor Author

@codeofdusk
Copy link
Contributor Author

Essentially, GetVisibleRanges is a convenient way to check whether microsoft/terminal#4018 is available, as that work fixed many bugs in the UIA implementation.

@feerrenrut
Copy link
Contributor

Is there any other way to check for that? If not, then please just change the names and docs here to be really explicit about what you are trying to determine and why.

Perhaps a name like IsImprovedUIATextRangeAvailable with a docstring including all the notes you have given me in comments here, and also pointing to that PR.

@codeofdusk
Copy link
Contributor Author

Is there any other way to check for that?

Sadly I don't think so, CC @miniksa and @carlos-zamora.

@josephsl
Copy link
Collaborator

josephsl commented May 6, 2021 via email

@miniksa
Copy link

miniksa commented May 6, 2021

Is there any other way to check for that?

Sadly I don't think so, CC @miniksa and @carlos-zamora.

We are strongly advised by the Windows compatibility experts that detecting functionality is always supreme to detecting version, so we would have to say that we prefer the existing method of testing for the functionality to know you're on an appropriate version.

However, if you must go beyond that, you might be able to use VerifyVersionInfo of which I have a sample here https://github.com/microsoft/terminal/blob/22fd06e19b3e564d448d60ebf34ffd63764f8080/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp#L1158-L1168. I'm obviously guilty of ignoring their strong recommendation and checking the version there... but VerifyVersionInfo is preferred by the compat team to getting the number and doing the comparison yourself with one of the GetVersion* APIs.

Do note that generally speaking, the Windows compatibility team has installed a number of measures to make it very difficult for applications to get the version of the operating system and force them to feature detect instead. So you may get a "version lie" out of that API or any of the others that is crafted to the maximum version that NVDA is pre-manifested to support.

@codeofdusk codeofdusk requested a review from a team as a code owner May 6, 2021 16:20
@codeofdusk codeofdusk requested a review from seanbudd May 6, 2021 16:20
@codeofdusk
Copy link
Contributor Author

I've updated comments to express intent more clearly, but haven't yet changed any names yet.

Something like isImprovedTextRangeAvailable as a replacement for is21H1Plus sounds good, but what should the new name for ConsoleUIATextInfoPre21H1 be?

@feerrenrut
Copy link
Contributor

@codeofdusk for brevity you can probably drop the "consoleUIA" part since it is in the "winConsoleUIA" module already. Then I'd either mention its a work around for broken "GetVisibleRanges" perhaps TextInfoWorkaroundBrokenGetVisibleRanges. Other options could be to refer to what the replacement it is based like TextInfoUsingRangeFromPoint

I think TextInfoWorkaroundBrokenGetVisibleRanges is more clear, despite being more ugly. Hopefully the ugly helps to highlight that not everything can be relied on when this is there.

@codeofdusk codeofdusk changed the title UI Automation in Windows Console: rename is21H1Plus to isPostUIARefactor UI Automation in Windows Console: rename is21H1Plus to isImprovedTextRangeAvailable May 7, 2021
@codeofdusk
Copy link
Contributor Author

@feerrenrut I settled on TextInfoWorkaroundEndInclusive (see description).

@codeofdusk codeofdusk requested a review from feerrenrut May 7, 2021 12:47
@AppVeyorBot
Copy link

See test results for failed build of commit 64348e2b3b

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Let me know if you will change the TextInfo. Either way I think this can be merged.

source/NVDAObjects/UIA/winConsoleUIA.py Outdated Show resolved Hide resolved
source/NVDAObjects/UIA/winConsoleUIA.py Outdated Show resolved Hide resolved
@codeofdusk
Copy link
Contributor Author

Ready for a merge whenever you are.

@codeofdusk codeofdusk marked this pull request as draft May 13, 2021 03:44
@codeofdusk codeofdusk marked this pull request as ready for review May 13, 2021 04:18
"""Fixes expand/collapse on end inclusive UIA text ranges, uses rangeFromPoint
instead of broken GetVisibleRanges for bounding, and implements word
movement support."""
class TextInfoWorkaroundEndInclusive(TextInfo):
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with using TextInfo as a name is that code like this becomes quite ambiguous for a reader. I think it is unnecessarily ambiguous. Consistency is worthwhile if the convention scales, I'm not sure this convention really makes anything better, other than allowing the developer a way out of coming up with a descriptive name.

Code is read many more times than it is written. Better to optimize for the reader IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I say ambiguous, I mean that you have to be aware that there is a class within this module called TextInfo, and check that the import doesn't do from textInfos import TextInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the name is now ConsoleUIATextInfo (with capitalization consistent with the rest of NVDA).

In the long term (i.e. once microsoft/terminal#6986 has been resolved), I hope to transition to just using the standard UIATextInfo implementation for consoles where isImprovedTextRangeAvailable... but that's in a follow-up PR.

feerrenrut
feerrenrut previously approved these changes May 14, 2021
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

I'll look to merge this on Monday.

@feerrenrut feerrenrut added this to the 2021.1 milestone May 14, 2021
@feerrenrut feerrenrut changed the base branch from master to beta May 17, 2021 05:20
@feerrenrut feerrenrut dismissed their stale review May 17, 2021 05:20

The base branch was changed.

feerrenrut
feerrenrut previously approved these changes May 17, 2021
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @codeofdusk. This is being merged into Beta, it will be merged back into alpha/master shortly.

@feerrenrut
Copy link
Contributor

A note @codeofdusk, this requires a change log. Code is only "internal use" if the class / function is prefixed with an underscore. Otherwise it is considered an API change and should be documented.

@codeofdusk
Copy link
Contributor Author

codeofdusk commented May 17, 2021

Something like this in dev changes:

  • WinConsoleUIA.is21H1Plus has been renamed to WinConsoleUIA.isImprovedTextRangeAvailable and ConsoleUIATextInfoPre21H1 has been renamed to ConsoleUIATextInfoWorkaroundEndInclusive due to Microsoft changes
  • consoleUIATextInfo has been renamed to ConsoleUIATextInfo for consistency in capitalization

@feerrenrut
Copy link
Contributor

I have updated the description for you.

@codeofdusk
Copy link
Contributor Author

LGTM.

@feerrenrut feerrenrut merged commit 5c157ba into nvaccess:beta May 17, 2021
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

6 participants