Summary of the Pull Request

When executing an escape sequence that might cause the viewport to scroll, a check is made to see whether the cursor position is in the bounds of the scroll margins, otherwise the scroll operation shouldn't happen. In a couple of cases (in the RI, DL, and IL escape sequence), these boundary tests were incorrect, so the scroll operation would sometimes not occur when it should have. This PR fixes those boundary tests.

Tested manually, and with the joe editor which was previously failing, and with some additional screen buffer unit tests.

PR Checklist

• Closes Screen margins and scrolling regions are acting strange for the JOE editor #1366
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Screen margins and scrolling regions are acting strange for the JOE editor #1366

Detailed Description of the Pull Request / Additional comments

The one problem was the use of Viewport::IsInBounds method to check whether the cursor was inside the margins. That method only works on the first column of the screen, because the horizontal margins are always set to 0. So that needed to be replaced with a test that only checked the y offset against the vertical margins.

The other problem was not first checking whether the margins had actually been set. When the margins aren't set, the top and bottom boundaries are both 0, so that needs to be handled as a special case, otherwise it'll only match positions on the first line of the screen.

Having made these fixes in the DoSrvPrivateReverseLineFeed and DoSrvPrivateModifyLinesImpl methods, I thought it might also be a good idea to refactor the boundary tests into a shared method in the SCREEN_INFORMATION class, to try and make the code more readable. I could also then make use of that method to simplify the DoSrvMoveCursorVertically implementation a little.

I had initially planned to refactor the AdjustCursorPosition method as well, but I decided against that in the end, because the code is a lot more complicated in that case, and I didn't want to risk potential performance issues given the frequency of its use.

In any event, I've committed the refactoring as a separate step, so I can always revert that if you don't think it's a good idea.

Validation Steps Performed

I've added a few screen buffer units tests that make sure the RI, DL, and IL escape sequences basically work, and also specifically check the conditions that were previously failing.

I've also run a few of my own manual tests which check various margin boundary conditions. And I've made sure that the problem with joe editor that was reported in issue #1366 is now working.