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: Remove "Text area", replace isAtLeastWin10, and code cleanup #9761

Merged
merged 4 commits into from Jun 17, 2019

Conversation

@codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Jun 17, 2019

Link to issue number:

Builds on #9614.

Summary of the issue:

Currently:

  • If the system language is not English, the name of the UIA console is not localized.
  • The windows 10 detection function always returns True if the currently-running version is at least the supplied one. For example, this makes it difficult to enable UIA consoles for Windows 10 1809 and later except 1903.
  • Since #9673, the last-typed word was sometimes still announced after pressing enter with "speak typed words" enabled, including deleted characters in some cases.

Description of how this pull request fixes the issue:

  • The name "Text area" is no longer reported for UIA consoles.
  • The isAtLeastWin10 function has been renamed to isWin10. It now takes an optional atLeast keyword argument, which defaults to True. The caret movement workarounds have been explicitly enabled for 1903 (not 1903 and later) in anticipation of a possible future Microsoft fix.
  • speech.curWordChars is once again explicitly cleared when script_flushQueuedChars is called.
  • winConsoleUIA.winConsoleUIA has been renamed to winConsoleUIA.WinConsoleUIA to remain consistent with other NVDA classes.
  • Added clarifying comments and method docstrings.

Testing performed:

Tested that the console is still functional and that isWin10 returns expected results on Windows 10 1903.

Known issues with pull request:

None.

Change log entry:

== Changes for Developers ==

  • Added a new isWin10 function to the winVersion module which returns whether or not this copy of NVDA is running on (at least) the supplied release version of Windows 10 (such as 1903). (#9761)
@codeofdusk
Copy link
Contributor Author

@codeofdusk codeofdusk commented Jun 17, 2019

Copy link
Member

@michaelDCurran michaelDCurran left a comment

I would like to see this pr just focus on actual code changes and comment clarification, rather than code style.
I.e.

  • Suppressing the name of the control
  • Changing the windows 10 logic
  • Renaming WinConsoleUIA
  • Clarifying comments
    This makes it a lot easier to review and test.
    Plus, some of your style changes I find hard to agree with. For example, moving the final bracket up onto the line with the last argument. That is not consistant with other functions/lists split over multiple lines.
    Perhaps we can revisit code style later on in this project, but if there was a major issue with styling in the first place, the previous prs would not have been approved. Getting style right in the first place is very good practice, but changing it after the fact tends to introduce bugs and makes it harder to track the history of lines in commits. Though this is just my personal opinion :)

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jun 17, 2019

Have you ran with this pr (with the most recent changes) for a while? I just want to ensure this has had enough testing before I merge it.

@codeofdusk
Copy link
Contributor Author

@codeofdusk codeofdusk commented Jun 17, 2019

Have you ran with this pr (with the most recent changes) for a while? I just want to ensure this has had enough testing before I merge it.

I've tested on my machine (during development) and everything seemed fine to me...

@michaelDCurran michaelDCurran merged commit d4e81ac into nvaccess:master Jun 17, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 17, 2019
@codeofdusk
Copy link
Contributor Author

@codeofdusk codeofdusk commented Jun 17, 2019

@michaelDCurran Could you please update what's new for these PRs? (remove the old entry from 2019.2 changes and add the one from this description)

michaelDCurran added a commit that referenced this issue Jun 18, 2019
@codeofdusk
Copy link
Contributor Author

@codeofdusk codeofdusk commented Jun 18, 2019

@michaelDCurran Please move the change from bug fixes to changes for developers. Thanks.

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jun 18, 2019

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

3 participants