Avoid freeze in Windows API Level 0 and 1 console when UIA support is enabled by ABuffEr · Pull Request #14703 · nvaccess/nvda

ABuffEr · 2023-03-07T13:07:03Z

Link to issue number:

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.

Better issue comment Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

AppVeyorBot · 2023-03-07T14:58:52Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/dc46a2l1fkmd5bwf/artifacts/output/nvda_snapshot_pr14703-27813,ff21508d.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 27.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 23.2,
FINISH_END 0.2

See test results for failed build of commit ff21508d8b

codeofdusk · 2023-03-16T21:36:44Z

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 · 2023-03-28T05:24:44Z

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.

Accept seanbudd suggestion. Co-authored-by: Sean Budd <seanbudd123@gmail.com>

…reduced to 100

ABuffEr · 2023-03-28T18:10:15Z

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

AppVeyorBot · 2023-03-28T18:45:16Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/oxwjfvvv4nf45cn8/artifacts/output/nvda_snapshot_pr14703-27944,5900fdb1.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.1,
FINISH_END 0.1

See test results for failed build of commit 5900fdb1ba

ABuffEr · 2023-03-30T11:22:10Z

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

Rename to _IGNORE_TRAILING_WHITESPACE_LENGTH to keep it private Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Better comment for late import Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Readability: rename and move IGNORE_TRAILING_WHITESPACE_LENGTH into function Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd

Thanks @ABuffEr

seanbudd · 2023-04-05T00:06:23Z

@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)"

codeofdusk

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

ABuffEr · 2023-04-05T05:50:05Z

"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.

Better comment about ConsoleUIATextInfo Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

Better comment describing solved issue Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

AppVeyorBot · 2023-04-05T07:07:56Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/a3ox8hijnlm0ee6n/artifacts/output/nvda_snapshot_pr14703-28031,7bad1d47.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.9,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.1,
FINISH_END 0.1

See test results for failed build of commit 7bad1d47d5

…fixIssue14689

ABuffEr · 2023-04-09T17:52:06Z

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 · 2023-04-09T18:20:44Z

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

…pport is enabled (nvaccess#14703)" This reverts commit 14bc6bd.

codeofdusk · 2023-04-09T18:22:52Z

I've opened #14815.

Avoid freeze in Windows API Level 0 console when UIA support is enabled

6097c5a

ABuffEr requested a review from a team as a code owner March 7, 2023 13:07

ABuffEr requested a review from seanbudd March 7, 2023 13:07

codeofdusk suggested changes Mar 7, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/speechDictHandler/__init__.py Outdated

Update source/speechDictHandler/__init__.py

bc553c0

Better issue comment Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

Use apiLevel instead of config

8332fc6

codeofdusk suggested changes Mar 7, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

Fix lint

7ce1543

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 marked this pull request as draft March 8, 2023 13:56

ABuffEr marked this pull request as ready for review March 8, 2023 14:00

seanbudd reviewed Mar 28, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

ABuffEr and others added 3 commits March 28, 2023 19:42

Update source/speechDictHandler/__init__.py

06bd4d0

Accept seanbudd suggestion. Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Moved IGNORE_TRAILING_WHITESPACE_LENGTH outside of the function, and …

6a0e6a7

…reduced to 100

Fix lint

306970b

codeofdusk reviewed Mar 29, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

New approach: check isinstance of ConsoleUIATextInfo

b0846e4

seanbudd reviewed Apr 3, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/speechDictHandler/__init__.py Outdated

ABuffEr and others added 3 commits April 3, 2023 18:52

Update source/speechDictHandler/__init__.py

f59b8c8

Rename to _IGNORE_TRAILING_WHITESPACE_LENGTH to keep it private Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Update source/speechDictHandler/__init__.py

acffc63

Better comment for late import Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Fix _IGNORE_TRAILING_WHITESPACE_LENGTH into function

d8651d8

seanbudd reviewed Apr 4, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

ABuffEr and others added 2 commits April 4, 2023 16:02

Update source/speechDictHandler/__init__.py

bf542b7

Readability: rename and move IGNORE_TRAILING_WHITESPACE_LENGTH into function Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Removed global _IGNORE_TRAILING_WHITESPACE_LENGTH

d6f86fd

Use api.getFocusObject instead of globalVars.focusObject

54aa703

seanbudd approved these changes Apr 5, 2023

View reviewed changes

codeofdusk approved these changes Apr 5, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/speechDictHandler/__init__.py Outdated

ABuffEr and others added 4 commits April 5, 2023 07:53

Update source/speechDictHandler/__init__.py

ba204f5

Better comment about ConsoleUIATextInfo Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

Update source/speechDictHandler/__init__.py

46bb1a9

Better comment describing solved issue Co-authored-by: Bill Dengler <codeofdusk@gmail.com>

Merge remote-tracking branch 'origin/master' into fixIssue14689

c2bddab

update changes

657f601

ABuffEr and others added 3 commits April 5, 2023 15:27

Fix comment lint

0c0d82f

Merge remote-tracking branch 'origin/master' into fixIssue14689

702754b

Merge branch 'fixIssue14689' of https://github.com/ABuffEr/nvda into …

db88735

…fixIssue14689

seanbudd merged commit 14bc6bd into nvaccess:master Apr 5, 2023

nvaccessAuto added this to the 2023.2 milestone Apr 5, 2023

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Apr 9, 2023

Revert "Avoid freeze in Windows API Level 0 and 1 console when UIA su…

ffb3e0f

…pport is enabled (nvaccess#14703)" This reverts commit 14bc6bd.

codeofdusk mentioned this pull request Apr 9, 2023

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

Merged

ABuffEr mentioned this pull request Apr 15, 2023

NVDA gets frozen permanently to the point where restarting NVDA is the only way to restore it #14831

Closed

Uh oh!

Conversation

ABuffEr commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Mar 7, 2023

Uh oh!

Uh oh!

codeofdusk commented Mar 16, 2023

Uh oh!

Uh oh!

seanbudd commented Mar 28, 2023

Uh oh!

ABuffEr commented Mar 28, 2023

Uh oh!

AppVeyorBot commented Mar 28, 2023

Uh oh!

Uh oh!

ABuffEr commented Mar 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Apr 5, 2023

Uh oh!

codeofdusk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ABuffEr commented Apr 5, 2023

Uh oh!

AppVeyorBot commented Apr 5, 2023

Uh oh!

ABuffEr commented Apr 9, 2023

Uh oh!

codeofdusk commented Apr 9, 2023

Uh oh!

codeofdusk commented Apr 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ABuffEr commented Mar 7, 2023 •

edited

Loading