Skip to content

Avoid freeze removing blank lines from IMPROVED and END_INCLUSIVE UIA consoles#14822

Merged
seanbudd merged 5 commits into
nvaccess:masterfrom
ABuffEr:fixIssue14689V2
Apr 18, 2023
Merged

Avoid freeze removing blank lines from IMPROVED and END_INCLUSIVE UIA consoles#14822
seanbudd merged 5 commits into
nvaccess:masterfrom
ABuffEr:fixIssue14689V2

Conversation

@ABuffEr

@ABuffEr ABuffEr commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #14689

Summary of the issue:

IMPROVED (apiLevel 1) and END_INCLUSIVE (apiLevel 0) UIA consoles produce a lot of blank lines (more than 17000) in some output, causing a NVDA freeze during speech dictionary processing.

Description of user facing changes

None.

Description of development approach

I added a _get_text method to ConsoleUIATextInfo class (for apiLevel 1 consoles), stripping all \r\n on the rightside of output from _get_text of superclass (implicitly used previously).
For apiLevel 0 consoles (ConsoleUIATextInfoWorkaroundEndInclusive class), I simply considered that the _get_text already present calls _get_text from superclass, that is, ConsoleUIATextInfo, so text is already cleaned.

Testing strategy:

Manual, with #14689 str, in apiLevel 1 console.

Known issues with pull request:

Hopefully none, this time.

Change log entries:

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.

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 marked this pull request as ready for review April 11, 2023 16:42
@ABuffEr ABuffEr requested a review from a team as a code owner April 11, 2023 16:42
@ABuffEr ABuffEr requested a review from seanbudd April 11, 2023 16:42
@ABuffEr

ABuffEr commented Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

CC: @codeofdusk

@codeofdusk codeofdusk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this: it's a small, targeted fix.

That said, I thought constraining ourselves to the visible text worked around the microsoft/terminal#6986 issues? CC @carlos-zamora

@codeofdusk

Copy link
Copy Markdown
Contributor

Also, consider a situation in which the console has (actual) empty lines at the end of output. If you call rstrip, it is impossible to distinguish between these actual empty text lines and the blank cells in the buffer.

@ABuffEr

ABuffEr commented Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

Also, consider a situation in which the console has (actual) empty lines at the end of output. If you call rstrip, it is impossible to distinguish between these actual empty text lines and the blank cells in the buffer.

Yes, I considered it. But, for reasons I don't know exactly, even with no prompt string (that's beyond a condition of reasonable tradeoff, imho) I see blank lines of last output (I mean, I find them exploring with object navigation). I think exploration and voice work with slightly different sources.
However, I tested in apiLevel 0 console, not 1; I have Windows terminal too, but that falls under UIATextInfo without patches.

Comment thread source/NVDAObjects/UIA/winConsoleUIA.py Outdated
Comment thread source/NVDAObjects/UIA/winConsoleUIA.py Outdated
# #14689: IMPROVED and END_INCLUSIVE UIA consoles have many blank lines,
# which slows speech dictionary processing to a halt
res = super()._get_text()
return res.rstrip("\r\n")

@seanbudd seanbudd Apr 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a reason why we are always stripping here, rather than checking the length of the tail like before e.g. 100 chars?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Performance, and the fact it's a more targeted solution (even into striping only \r\n). I noticed that _get_text is called more times even when you write a single character in apiLevel 0 terminal (with no voice output). However, I can introduce it, if you think it's more reasonable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am surprised there is a performance hit for checking the length of the tail, stripping is surely the most expensive operation.
My concern with this approach is that intentionally empty buffers that are just a few newlines might not be reported correctly.
For reference, you can probably cache _get_text, however that might cause other performance issues (not always getting the very latest text for a given core cycle).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added previous check with IGNORE_TRAILING_WHITESPACE_LENGTH.

ABuffEr and others added 2 commits April 12, 2023 14:44
@seanbudd seanbudd merged commit 0731e28 into nvaccess:master Apr 18, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Apr 18, 2023
@seanbudd

Copy link
Copy Markdown
Member

Thanks @ABuffEr

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

4 participants