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

Avoid freeze in Windows API Level 0 and 1 console when UIA support is enabled #14703

Merged
merged 21 commits into from
Apr 5, 2023

Conversation

ABuffEr
Copy link
Contributor

@ABuffEr ABuffEr commented Mar 7, 2023

Link to issue number:

Fixes #14689

Summary of the issue:

Due to a bug in API Level 0 and 1 consoles (see microsoft/terminal/#5243), sometimes NVDA freezes during dictionary processing of console text, containing a huge amount of blank lines.

Description of user facing changes

None.

Description of development approach

If current focus object has apiLevel 0 or 1, and difference between regular text and text with rightside blank lines stripped is greater than 100, we use cleaned text for dictionary processing.

Testing strategy:

Manual, with #14689 str.

Known issues with pull request:

None.

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.

@ABuffEr ABuffEr requested a review from a team as a code owner March 7, 2023 13:07
@ABuffEr ABuffEr requested a review from seanbudd March 7, 2023 13:07
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
Better issue comment

Co-authored-by: Bill Dengler <[email protected]>
@AppVeyorBot
Copy link

See test results for failed build of commit ff21508d8b

source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
@ABuffEr ABuffEr changed the title Avoid freeze in Windows API Level 0 console when UIA support is enabled Avoid freeze in Windows API Level 0 and 1 console when UIA support is enabled Mar 7, 2023
@ABuffEr ABuffEr marked this pull request as draft March 8, 2023 13:56
@ABuffEr ABuffEr marked this pull request as ready for review March 8, 2023 14:00
@codeofdusk
Copy link
Contributor

Honestly, I'm not the biggest fan of this approach (or, for that matter, putting any more work into the API <2 console). There are changes coming down the pipeline that I'm not at liberty to discuss in detail (I would if I could), so I'd suggest holding off on any changes for now.

CC @DHowett.

@seanbudd
Copy link
Member

Honestly, I'm not the biggest fan of this approach (or, for that matter, putting any more work into the API <2 console).

Shouldn't NVDA still support the API <2 console? Can you explain why not?
It seems like this small change will improve a freeze quite easily.

@ABuffEr
Copy link
Contributor Author

ABuffEr commented Mar 28, 2023

@cary-rowen, can you test the last try-build when available?

@AppVeyorBot
Copy link

See test results for failed build of commit 5900fdb1ba

@ABuffEr
Copy link
Contributor Author

ABuffEr commented Mar 30, 2023

Ok, I found a way that can accomplish @codeofdusk requests.
I can import (into function) ConsoleUIATextInfo class, that shared by apiLevel 1 and 0 implementations, and then check istance of textInfo returned by review.getObjectPosition(globalVars.focusObject).
I could additionally check even apiLevel on focusObject, if you want, but it has not particularly sense at the moment

source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
ABuffEr and others added 3 commits April 3, 2023 18:52
Rename to _IGNORE_TRAILING_WHITESPACE_LENGTH to keep it private

Co-authored-by: Sean Budd <[email protected]>
Better comment for late import

Co-authored-by: Sean Budd <[email protected]>
ABuffEr and others added 2 commits April 4, 2023 16:02
Readability: rename and move IGNORE_TRAILING_WHITESPACE_LENGTH into function

Co-authored-by: Sean Budd <[email protected]>
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.

Thanks @ABuffEr

@seanbudd
Copy link
Member

seanbudd commented Apr 5, 2023

@ABuffEr - you omitted the change log section from the PR template. Do you think this warrants a change log entry?
What do you think of this in bug fixes:
"When forcing UIA support with certain terminal and consoles, a bug is fixed which caused a freeze and the log file to be spammed. (#14689)"

Copy link
Contributor

@codeofdusk codeofdusk left a comment

Choose a reason for hiding this comment

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

Approved pending my suggested comment clarifications (for consistency in style of references)

source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
source/speechDictHandler/__init__.py Outdated Show resolved Hide resolved
@ABuffEr
Copy link
Contributor Author

ABuffEr commented Apr 5, 2023

"When forcing UIA support with certain terminal and consoles, a bug is fixed which caused a freeze and the log file to be spammed. (#14689)"

Ok for me.

ABuffEr and others added 4 commits April 5, 2023 07:53
@AppVeyorBot
Copy link

See test results for failed build of commit 7bad1d47d5

@seanbudd seanbudd merged commit 14bc6bd into nvaccess:master Apr 5, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Apr 5, 2023
@ABuffEr
Copy link
Contributor Author

ABuffEr commented Apr 9, 2023

Hi @seanbudd,
unfortunately this PR seems to cause troubles in some situations. I'm testing a new solution acting from _get_text directly in ConsoleUIATextInfo (new PR soon). Can you revert this? Thanks, and sorry for inconvenience.

@codeofdusk
Copy link
Contributor

If you're able to fix the bug by patching ConsoleUIATextInfo directly, I too would feel much more comfortable with this.

@codeofdusk
Copy link
Contributor

I've opened #14815.

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.

nvda logs a lot of '\r\n\r\n' if UIA support for Windows console is enabled
5 participants