Fix the 'ArgumentOutOfRangeException' caused by top of the text being scrolled up-off the buffer #979

daxian-dbw · 2019-07-30T17:01:52Z

Major change includes:

Update ReallyRender to avoid writing everything every time when editing the text. Instead, we find the first different logical line, and starting writing from there.
Refactor ReallyRender by moving some logic to separate methods.
Support editing text even if the top of the text has been scrolled up-off the buffer.
Disallow cursor to be set to a position that is off the buffer.
Fix a bug in ConvertLineAndColumnToOffset so that a point can be translated to the offset correctly.

Note: I would like to add some more tests, but not yet clear what tests to add.

Edit long text in small buffer console (66 x 17, Windows)

Copy long text to small buffer console (66 x 17, Windows)

Function Get-IniContent copied from https://gallery.technet.microsoft.com/scriptcenter/ea40c1ef-c856-434b-b8fb-ebd7a76e8d91

Copy long text in Ubuntu terminal

Function Get-IniContent copied from https://gallery.technet.microsoft.com/scriptcenter/ea40c1ef-c856-434b-b8fb-ebd7a76e8d91

Edit long text in Ubuntu terminal

- Update 'ReallyRender' to avoid writing everything every time when editing the text. Instead, we find the first different logical line, and starting writing from there. - Support editing text even if the top of the text has been scrolled up-off the buffer. - Disallow cursor to be set to a position that is off the buffer. - Fix a bug in 'ConvertLineAndColumnToOffset' so that a point can be translated to the offset correctly.

daxian-dbw · 2019-07-30T22:28:36Z

@lzybkr The PR is ready for review. Please take a look when you have time. Thanks!

lzybkr · 2019-07-31T18:02:09Z

For some reason, I had to add this to build on my dev machine:

diff --git a/PSReadLine/PSReadLine.csproj b/PSReadLine/PSReadLine.csproj
index 1a87173..98597fb 100644
--- a/PSReadLine/PSReadLine.csproj
+++ b/PSReadLine/PSReadLine.csproj
@@ -8,6 +8,7 @@
     <FileVersion>2.0.0</FileVersion>
     <InformationalVersion>2.0.0-beta4</InformationalVersion>
     <TargetFrameworks>net461;netcoreapp2.1</TargetFrameworks>
+    <LangVersion>7.1</LangVersion>
   </PropertyGroup>
   <ItemGroup>
     <PackageReference Include="PowerShellStandard.Library" version="5.1.0-*" />

lzybkr · 2019-07-31T18:14:47Z

I haven't looked closely at the changes, but I'll try the changes out for awhile and see if I notice any issues.

The first two things I've noticed:

Shift+Home seems to cause problems. The selection doesn't work and a blank line gets added each time.
You can't scroll back to the beginning of the line unless you delete enough of the text.

These two issues might make it annoying for some people to undo, e.g. some Emacs users will use Ctrl+aCtrl+k to delete the whole line.

daxian-dbw · 2019-07-31T18:16:07Z

Weird. I don't need that line and the CI builds fine ... But I can add <LangVersion>latest</LangVersion> if needed.

daxian-dbw · 2019-07-31T18:21:39Z

Can you be more specific about the those two things?

Shift+Home causing problem --- in what cases? Like console buffer size and etc. I need those info to repro the issue.
can't scroll back to the beginning of the line --- again, in what cases? If the top of the text has already been scrolled up-off the buffer, then it's by design.

but I'll try the changes out for awhile and see if I notice any issues.

Yes please. I'm dogfooding my changes. You trying out it will definitely help to find issues.

lzybkr · 2019-07-31T19:43:26Z

Shift+Home - repro in Windows Terminal, type @" then Enter enough times so that the prompt is off the screen, then Shift+Home multiple times. It will beep and display a blank line (no continuation prompt).
Scroll back to the beginning of the line - any number of ways you might try: Home, arrow keys, etc. I can imagine it's hard to fix (PSReadLine wasn't designed for editing files), but it will cause difficulties for some users.

daxian-dbw · 2019-07-31T20:37:03Z

Thanks @lzybkr, I can repro the Shift+Home issue. I will look into it.

For the second issue, it's by design. As you mentioned, PSReadLine is not designed for editing files, at least not yet. It's not perfect, but the experience is much better than before this change.
Today, moving the cursor doesn't require rendering. If we want to make it more like vim in this case, some fundamental changes will be needed.

lzybkr · 2019-08-01T18:04:14Z

I can't load PSReadLine in Windows PowerShell anymore, I'm not sure if it's from the PR or a previous one though.

…at has been scrolled up-off the buffer

daxian-dbw · 2019-08-01T21:55:11Z

The rendering issue when shift-selecting text that had been scrolled up-off the buffer has been fixed.
When shift-selecting text that had been scrolled off the buffer, no text will be selected and PSReadLine will beep because the cursor cannot be moved to that range of text. When rendering, we will find that nothing needs to be render and skip writing anything.

I can't load PSReadLine in Windows PowerShell anymore, I'm not sure if it's from the PR or a previous one though.

Did you build targeting net461? Building against net461 works fine on my machine and also works in CI.

lzybkr · 2019-08-01T22:01:38Z

I used the binary built in CI (AppVeyor).

daxian-dbw · 2019-08-01T22:05:23Z

I guess you didn't unblock the zip file after downloading it. Can you please double check if that's the case?

lzybkr · 2019-08-01T22:20:02Z

Thanks for the hints, locally I did build the wrong target, and I did forget to unblock the file from AppVeyor.

daxian-dbw · 2019-08-16T21:21:31Z

@lzybkr Any feedback after using PSReadLine with this change for a while?

msftrncs · 2019-08-21T06:08:33Z

I've tested this PR just briefly. Noticed a couple small things, they may or not be related to this PR:

The integrated terminal in VSCode does not permit editing above the top of the current window, as if there was no buffer (but I can scroll-wheel up and see more, but cannot edit it). Editing worked as expected in a regular console when the input was longer than the window.
The <up-arrow> wouldn't take me up a line while editing if the line was so long it wrapped, and the cursor was at the end of the line, and had been deliberate placed there using <end> or <right arrow>.
Cursor positioning, if the cursor is arrowed to the end of a line, then up or down, it jumps to the end of the lines of each line it touches. Normally editors only do this if you press <end> first.

lzybkr

I just noticed I started a review but didn't finish it. I'm on vacation and will finish when I return.

PSReadLine/Render.cs

lzybkr · 2019-08-10T15:50:08Z

PSReadLine/Render.cs

+                        {
+                            // This could happen when adding a new line in the end of the very last line.
+                            // In this case, we scroll up by writing out a new line.
+                            _console.SetCursorPosition(left: bufferWidth - 1, top: bufferHeight - 1);


Based on the method name, I thought this method would be side effect free, so moving the cursor is surprising.

You are right. It's not just calculation, but also set the cursor to the about-to-start-rendering position.
How about rename the method to PrepareToRender?

lzybkr · 2019-08-10T16:23:35Z

PSReadLine/Render.cs

+                    // then it's possible we are facing this special case and thus would need to do additional checks later.
+
+                    minLineLength = renderLines.Length;
+                    if (cursorX == Options.ContinuationPrompt.Length && cursorY == 0)


I think I understand this check, but couldn't it be true by luck, e.g. you have a short prompt?

With the cursorY > _initialY check above, this won't happen, because of the cursorY == 0 check here implies _initialY < 0, meaning that this won't be the first physical line where the user prompt is at.
Now that the cursorY > _initialY check is removed, we need to check for _initialY < 0 here.

This piece of code has been changed, to cover the more general scenarios -- after editing, the cursor is supposed to be moved off the buffer.
Please take another look.

lzybkr · 2019-08-10T20:39:40Z

PSReadLine/Render.cs

-                        // newY could be less than 0 if 'PromptText' is manually set to be a long string.
-                        if (newY >= 0)
+                        // Additional check for the special case mentioned above: the cursor is supposed to be moved to the previous line.
+                        if (ConvertOffsetToPoint(_current).Y == -1)


The code feels wrong, but I'm not sure how precisely to address your concerns.

At any rate, in testing this out, you can get an exception if you delete multiple characters such that -1 is the wrong value, e.g. have a few blank lines at the start, get to this line of code, then type Alt+4Backspace to delete 4 characters at once.

Thanks for spotting the issue. I didn't know about the key binding to delete arbitrary number of characters. This changes the assumption of the code. I will rework the related part of the code.

This issue has been addressed, the logic is changed to cover the more general scenarios where the cursor is supposed to be moved to a position that is off the buffer after the editing.
In the following screenshot, the Hello world line and the following 3 empty lines are moved off the buffer, and then Alt+4Backspace works as expected.

However, as you might noticed, I removed the last empty line before moving the cursor to <0, 0> position. This is because if I don't remove the last line, Alt+4 scroll up the buffer to show the digit-argument: line, and will move the cursor one line above. In this case, the cursor will be moved to <0, -1>, and thus causes an exception.

This is not a problem introduced by this change. See the following screenshot with the beta.4 PSReadLine:

lzybkr · 2019-08-10T20:42:11Z

PSReadLine/Render.cs

-                if (isFirstLogicalLine)
+                // The editing was in the first logical line. We have to write everything in this case.
+                // Move the cursor to the initial position if we haven't done so.
+                if (_initialY < 0)


While this condition makes some logical sense, it should be documented on the field and all references to the field should be checked and tested to verify a negative value is safe because I don't think it could have happened before this PR.

I will go through all the uses of _initialY.

It will take some time to go through all uses of _initialY and verify those uses when _initialY is negative. I would like to do this in follow-up PRs. I opened #1015 to track this work.

because I don't think it could have happened before this PR.

This happens before this PR, but the code never handles this situation properly and that's causing the ArgumentOutOfRangeException issues.

lzybkr · 2019-08-10T22:32:33Z

PSReadLine/Render.cs

+                    _console.Write("\x1b[2J");
+                    _console.SetCursorPosition(0, _console.WindowTop);
+
+                    string newPrompt = GetPrompt();


You've mostly duplicated InvokePrompt, at least the bit starting with this line.

InvokePrompt calls back to ReallyRender. I don't want that here.

lzybkr · 2019-08-11T03:51:12Z

PSReadLine/Render.cs

@@ -28,6 +28,13 @@ public override string ToString()

    public partial class PSConsoleReadLine
    {
+        struct RenderedLineInfo


This struct is really just the return value of CalculateWhereAndWhatToRender, but you return 2 of them. Maybe you should merge them into 1 struct instead and give it a better name (it's too similar to RenderedLineData anyway.

Done. Use one struct to hold both the current and previous line info. Rename the struct to be LineInfoForRendering.

I don't see the updated code.

Sorry for the delay. The PR has been updated.

daxian-dbw · 2019-08-21T18:58:52Z

@msftrncs Thanks for playing with the changes! To reply to your comments:

The integrated terminal in VSCode does not permit editing above the top of the current window, as if there was no buffer (but I can scroll-wheel up and see more, but cannot edit it). Editing worked as expected in a regular console when the input was longer than the window.

The buffer of integrated terminal in VSCode doesn't work like the console host on windows. Anything scrolled off the screen is out of the buffer. You can check the [console]::BufferHeight to verify the buffer size.
For the text scrolled off the buffer, they are still visible by scrolling the wheel up, but they are out of the buffer and cannot be edited. What you (and I) want eventually is an experience like in VIM, but PSReadLine is not designed for file editing, yet. See #979 (comment)

The wouldn't take me up a line while editing if the line was so long it wrapped, and the cursor was at the end of the line, and had been deliberate placed there using or .

This behavior already exists today, without this PR. Can you please open an issue, if there isn't one already?

Cursor positioning, if the cursor is arrowed to the end of a line, then up or down, it jumps to the end of the lines of each line it touches. Normally editors only do this if you press first.

This behavior already exists today. I personally don't think this is an issue though.

msftrncs · 2019-08-23T05:48:04Z

I noticed something else now, playing, (no exceptions yet) if one resizes the console window while editing (really just viewing) a multiline history, things get out of sync. In this case I had a previous multiline item up, larger than the window, sitting the end, and made the window full screen. The text doesn't change, but then switching the history, between other multiline items, draws the items without clear the previous content. Then with a multiline history item back in the screen, restore the window size to normal, the item is now too long, but only the last 8 lines are shown at the top of the screen, and moving through the history at this point is now unusable as the screen is all out of sync.

msftrncs · 2019-08-25T03:31:14Z

Back to VS Code's terminal. I noticed that as I scroll through the history, when one is too long for VS Code's terminal's buffer, and then the next one (going backwards or `'up') would fit, the buffer isn't adjusted or the prompt redrawn, so that the whole history will be shown.

Went 'up' to this item:

Then 'up' to this one (its two more lines above the top):

Maybe it has something to do with the fact the two histories have the same start? The earlier history is simply a shorter version of the later history. I was able to reproduce this on a new 'herestring' sample, and it works either direction, up or down, as long as the beginning is the same, the shorter history item will not adjust to show more of it. It also happens in the Windows 10 PowerShell console. I suspect this is part of the optimization to not have to redraw the whole buffer?

daxian-dbw · 2019-08-30T22:49:01Z

I suspect this is part of the optimization to not have to redraw the whole buffer?

@msftclas Yes, this is because of the optimization to not redraw the whole buffer if possible. Those 2 history entries have the exactly the same first N logical lines, and thus the rendering starting from the N+1 logical line.
I personally don't concern about this scenario that much. The workaround is simply type up-arrow again and then type down-arrow. Unless the history entries are all incrementally adding new stuff that does't cause changes to the existing lines (including color), this workaround should make the desired history entry re-rendered.

daxian-dbw · 2019-09-05T23:34:41Z

@lzybkr Could you please take another look when you have time? Thanks!

daxian-dbw added 2 commits July 26, 2019 16:38

Refactor code and add more comments

fc5baea

daxian-dbw requested review from lzybkr and SteveL-MSFT July 30, 2019 17:02

daxian-dbw marked this pull request as ready for review July 30, 2019 22:27

Address the rendering issue that happens when shift-selecting text th…

4c33c66

…at has been scrolled up-off the buffer

vexx32 mentioned this pull request Aug 14, 2019

Long here-strings scroll on every keypress #962

Closed

lzybkr suggested changes Aug 21, 2019

View reviewed changes

msftrncs mentioned this pull request Aug 26, 2019

ArgumentOutOfRangeException in Integrated Console when entering long input lines that wrap PowerShell/vscode-powershell#702

Closed

daxian-dbw added 2 commits August 30, 2019 12:03

Address the easy part of Jason's comments

249bce1

Address the hard part of Jason's comment

ef67120

This was referenced Sep 13, 2019

MenuComplete garbled when cursor is at bottom of terminal #976

Closed

extra line after submitting command in VS Code terminal #1042

Closed

This was referenced Sep 13, 2019

ystem.ArgumentOutOfRangeException #1043

Closed

System.ArgumentOutOfRangeException: 该值必须大于或等于零，且必须小于控制台缓冲区在该维度的大小。 #1041

Closed

daxian-dbw added this to the 2.0.0-beta5 milestone Sep 13, 2019

msftrncs mentioned this pull request Sep 18, 2019

extra line after submitting command in VS Code terminal PowerShell/vscode-powershell#2187

Closed

daxian-dbw mentioned this pull request Feb 12, 2020

'ArgumentOutOfRangeException' issues were fixed in recent versions #1306

Closed

mark05e mentioned this pull request Apr 15, 2020

Notepad3 update error ScoopInstaller/Extras#3857

Closed

Fix the 'ArgumentOutOfRangeException' caused by top of the text being scrolled up-off the buffer #979

Fix the 'ArgumentOutOfRangeException' caused by top of the text being scrolled up-off the buffer #979

Conversation

daxian-dbw commented Jul 30, 2019 • edited Loading

Major change includes:

Edit long text in small buffer console (66 x 17, Windows)

Copy long text to small buffer console (66 x 17, Windows)

Copy long text in Ubuntu terminal

Edit long text in Ubuntu terminal

daxian-dbw commented Jul 30, 2019

lzybkr commented Jul 31, 2019

lzybkr commented Jul 31, 2019

daxian-dbw commented Jul 31, 2019

daxian-dbw commented Jul 31, 2019 • edited Loading

lzybkr commented Jul 31, 2019

daxian-dbw commented Jul 31, 2019

lzybkr commented Aug 1, 2019

daxian-dbw commented Aug 1, 2019

lzybkr commented Aug 1, 2019

daxian-dbw commented Aug 1, 2019

lzybkr commented Aug 1, 2019

daxian-dbw commented Aug 16, 2019

msftrncs commented Aug 21, 2019 • edited Loading

lzybkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Aug 21, 2019 • edited Loading

msftrncs commented Aug 23, 2019

msftrncs commented Aug 25, 2019

daxian-dbw commented Aug 30, 2019

daxian-dbw commented Sep 5, 2019

daxian-dbw commented Jul 30, 2019 •

edited

Loading

daxian-dbw commented Jul 31, 2019 •

edited

Loading

msftrncs commented Aug 21, 2019 •

edited

Loading

daxian-dbw commented Aug 21, 2019 •

edited

Loading