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

Improve Viewport and Viewport::WalkInBounds #17143

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 26, 2024

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 lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Apr 26, 2024
@lhecker
Copy link
Member Author

lhecker commented Apr 26, 2024

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 lhecker force-pushed the dev/lhecker/viewport-walking branch from 0cbd028 to 75e4f06 Compare April 26, 2024 21:48
@zadjii-msft
Copy link
Member

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)

bufferViewport.WalkInBounds(afterPos,
walkDir);
}
bufferViewport.WalkInBounds(afterPos, -dx);
Copy link
Member Author

Choose a reason for hiding this comment

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

@zadjii-msft Here's why the tests failed... I previously had this as:

bufferViewport.WalkInBounds(afterPos, dx);

but on closer inspection of the old code I noticed that the dx is meant inverse to an offset. I don't quite understand the old code to say why that is, but it does work now by simply using -dx.

I guess I'm being overly defensive over my PR here, but I do think it's fairly solid after all. 😅 (To be clear I don't mind merging it in 1.22 only.)

@j4james
Copy link
Collaborator

j4james commented May 2, 2024

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

@carlos-zamora
Copy link
Member

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

@lhecker
Copy link
Member Author

lhecker commented May 13, 2024

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

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

why was I scared of reviewing this?

}

return true;
const auto l = static_cast<ptrdiff_t>(_sr.left);
Copy link
Member

Choose a reason for hiding this comment

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

this makes me think "why were we doing it any other way before"

@lhecker lhecker enabled auto-merge June 23, 2024 02:20
@DHowett
Copy link
Member

DHowett commented Jul 2, 2024

I was scared too, can't figure out why

@lhecker lhecker added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit c9e2007 Jul 2, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/viewport-walking branch July 2, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants