Move cursor to left margin for IL and DL controls

[j4james]

Summary of the Pull Request

According to the DEC STD 070 manual, the cursor position should be moved to the left margin after the execution of the IL and DL escape sequences. This PR updates the code to implement that behaviour.

PR Checklist

• Closes IL and DL commands should move cursor to left margin #2534
• CLA signed. If not, go over here and sign the CLA
• Tests updated
• 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: IL and DL commands should move cursor to left margin #2534

Detailed Description of the Pull Request / Additional comments

I've modified the DoSrvPrivateModifyLinesImpl function (which is where the IL and DL controls are ultimately handled), to update the cursor position - setting the column to 0 - after the screen has been scrolled. Note that this has to happen inside the IsCursorInMargins check, since the cursor is not supposed to be moved if it was outside the margin boundaries.

Also note that this should really be set to the left margin specified by DECSLRM, but we don't yet support that control, so for now it's hardcoded to 0.

When it came to actually setting the cursor position, I found there were a number of options to choose from, with quite different behaviours. Some operations use a variation of SetConsoleCursorPosition, some use AdjustCursorPosition, and some call the SCREEN_INFORMATION::SetCursorPosition method directly, or even just Cursor::SetPosition. It wasn't always clear to why a particular method was chosen.

I ultimately decided on SCREEN_INFORMATION::SetCursorPosition with the TurnOn parameter set to false, since that seemed the simplest option that also matched the behaviour of a carriage return. I'm not certain that's the best choice, though, so I'm open to other suggestions.

Validation Steps Performed

There were a number of existing IL and DL tests that assumed the cursor position was not meant to move, and validated that behaviour. I've updated all of those tests to confirm that cursor position is now moved to column 0 when those escape sequences are executed.

[zadjii-msft]

This seems like the way I'd do it. Great work as always :)

[miniksa]

Also note that this should really be set to the left margin specified by DECSLRM, but we don't yet support that control, so for now it's hardcoded to 0.

Should this be filed as a backlog future work item?

[miniksa]

When it came to actually setting the cursor position, I found there were a number of options to choose from, with quite different behaviours. Some operations use a variation of SetConsoleCursorPosition, some use AdjustCursorPosition, and some call the SCREEN_INFORMATION::SetCursorPosition method directly, or even just Cursor::SetPosition. It wasn't always clear to why a particular method was chosen.

This certainly needs a follow up work item. I'm reviewing some of the code in each of these to try to come up with reasoning why you should use one over the other... and I'm realizing that this is probably the source of our "the cursor is blinking weirdly or is on when it shouldn't be while text is outputting" problems.

As far as I can tell, you've chosen the correct one with SCREEN_INFORMATION::SetCursorPosition because that one is responsible for updating the delay status on the cursor so it stays off or on instead of blinking wildly as it is moving rapidly around the screen when things are happening. The cursor is supposed to temporarily suspend blinking when there is activity (by the delay).

I am certain the inconsistency in application of which cursor-setting-function is being used is responsible for graphical glitches.

[miniksa]

When it came to actually setting the cursor position, I found there were a number of options to choose from, with quite different behaviours. Some operations use a variation of SetConsoleCursorPosition, some use AdjustCursorPosition, and some call the SCREEN_INFORMATION::SetCursorPosition method directly, or even just Cursor::SetPosition. It wasn't always clear to why a particular method was chosen.

This certainly needs a follow up work item. I'm reviewing some of the code in each of these to try to come up with reasoning why you should use one over the other... and I'm realizing that this is probably the source of our "the cursor is blinking weirdly or is on when it shouldn't be while text is outputting" problems.

As far as I can tell, you've chosen the correct one with SCREEN_INFORMATION::SetCursorPosition because that one is responsible for updating the delay status on the cursor so it stays off or on instead of blinking wildly as it is moving rapidly around the screen when things are happening. The cursor is supposed to temporarily suspend blinking when there is activity (by the delay).

I am certain the inconsistency in application of which cursor-setting-function is being used is responsible for graphical glitches.

I created #2739 for this one.

I left it up to @j4james and @zadjii-msft to determine if they want to track a future work item for DECSLRM.

[ghost]

🎉Windows Terminal Preview v0.5.2661.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[DHowett-MSFT]

This went out for conhost in insider build 19002! Thanks 😄