terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Improve Viewport and Viewport::WalkInBounds

Open lhecker opened this issue 1 year ago • 5 comments

This removes all of the 2D iteration machinery. Imagine the text buffer as a Cell[w][h] grid. Clearly, this is identical to a Cell[w*h] array, which shows that copying between overlapping ranges only needs either forward or backward copying, and not left/right/top/down.

With WalkDir removed, WalkInBounds can be rewritten with basic arithmetic which allows pos to be an exclusive end coordinate.

lhecker avatar Apr 26 '24 21:04 lhecker

Section 11.3.6.8 DECCRA seems okay in vttest: image

Not sure if there's anything else I can do to test the DetermineWalkDirection function.

lhecker avatar Apr 26 '24 21:04 lhecker

I'd be reluctant to merge this for 1.21 based solely on the fact that these are used everywhere and I'm guessing that bugs in the delta will be very subtle. But I'd be very happy to merge early in 1.22 when we've got more runway to validate.

(I'm also open to being overruled)

zadjii-msft avatar Apr 29 '24 11:04 zadjii-msft

FWIW, I've run a number of my own rectangle and scrolling tests on this branch, and I couldn't see any regressions.

j4james avatar May 02 '24 13:05 j4james

@lhecker Make sure you test this with Narrator. The bounding rects aren't updating for me.

carlos-zamora avatar May 06 '24 19:05 carlos-zamora

@carlos-zamora I believe this is a race condition in the UIA implementation. I found that it also reproduces in the stable builds (here: v1.20; repeatedly open cmd and hold any key immediately): image

The new-ish Terminal::_assertLocked() catches this bug: The culprit is the constructor which doesn't lock the console when asking for the viewport and cursor position.

lhecker avatar May 13 '24 15:05 lhecker

I was scared too, can't figure out why

DHowett avatar Jul 02 '24 14:07 DHowett