Fix System.ArgumentOutOfRangeException while get-tab

[jianyunt]

In the #663, @gshacklett has commented get- and then tab PSReadLine will throw. This case is very often on cloudshell because Azure cmdlets are quite long and people use tab a lot.

The fix is to let contents display until complete for the menu not selected case. I tested on Windows, Linux and cloudshell, the fix seems to be working. Please code review. Thanks.

However I broke the PossibleCompletions() tests. The fix changed the parameter in calling InvokePrompt https://github.com/lzybkr/PSReadLine/blob/master/PSReadLine/Completion.cs#L736 so code path is different. With the fix, we are executing the else block in https://github.com/lzybkr/PSReadLine/blob/master/PSReadLine/ReadLine.cs#L942.

[lzybkr]

It looks like MenuComplete is still a problem and should be fixed at the same time.

It seems reasonable to fall back to this fix if the menu + the command line won't fit on the screen.

Also - when selecting items from the menu, the tooltip is displayed below the menu. That should be disabled if it won't fit on the screen.

[jianyunt]

Thank you @lzybkr for your comments. Could you please give me more detailed info when you say "it looks like MenuComplete is still a problem "?

[lzybkr]

If you invoke MenuComplete, e.g. bind Tab like this:

Set-PSReadLineKeyBinding -Key Tab -Function MenuComplete

Then you will hit the call that tries to set the cursor position incorrectly. Your change avoids that call, but it is still reachable and many people do use MenuComplete.

[jianyunt]

For the cursor position issue while hit the Tab, we also need to apply the fix from https://github.com/PowerShell/PowerShell/pull/7299/files (the next release of pscore will contain it).

Once applied the fix, invoking the MenuComplete should work fine. I tested it on Linux and CloudShell. I don't see the cursor issue.
Also if the tooltip does not fit the screen while a user selects a menu, PSReadline scroll up the screen first. It seems to work fine.

[lzybkr]

I don't think that's correct. I have your to PSReadLine and shouldn't need the fix to PowerShell (no Write-Progress calls).

Try opening an instance of PowerShell, set the window height to 20, type Get-CCtrl+Space then use the arrow keys until Get-ChildItem is selected.

After doing this, I see the same exception as the one you are trying to fix.

[lzybkr]

This could write over some of the buffer if you press Tab anywhere that is not the last line, e.g.:

PS> & { Get-C<Tab><Tab>
>> }

After pressing Tab Tab, the screen might look like:

PS> & { Get-C<Tab><Tab>
Get-C1   Get-C2    Get-C3
PS> & { Get-C
>> }

When it should look like:

PS> & { Get-C<Tab><Tab>
>> }
Get-C1   Get-C2    Get-C3
PS> & { Get-C
>> }

[jianyunt]

Jason, I submitted the fix and tested it. It seems working fine now. Please take a look it again. Thanks!

[lzybkr]

Is this call necessary? I suspected it wasn't and tested briefly with it removed, I didn't see any problems.

[jianyunt]

Yes. we needed it. On Windows, I do not see problem either. But on Linux the System.ArgumentOutOfRangeException occurs on the EnsureMenuAndInputIsVisible() , Singleton._initialY -= toScroll becomes negative when rows is large.

[lzybkr]

@jianyunt - after resolving this comment (either removing the call or confirming it is needed), I think this PR is ready to merge.

[jianyunt]

@lzybkr Yes we need to keep this Line https://github.com/lzybkr/PSReadLine/blob/2f57fe5091e0e0cd343b86851dc31c9bdcb2afd5/PSReadLine/Completion.cs#L501 to avoid the ArgumentOutOfRangeException exception.

[lzybkr]

Are you relying on the + 1 to write out a single newline?

Based on the comment in EnsureMenuAndInputIsVisible, I could see that + 1 being removed at some point, and then the method would do nothing, right?

If my understanding is correct, it would be good to have some way to ensure this problem doesn't come back if someone removes that + 1 - a test, or maybe pass rows:1.

[jianyunt]

I found calling EnsureMenuAndInputIsVisible can cause the ArgumentOutOfRangeException when the window is reduced to one line height or the following can cause the exception too when the window height is 5 in this case (full screen editing). Thus I changed back to use console.Write(), after console.SetCursorPosition(0, this.Top); . It seems work fine for all cases now. Thanks.

& {Get-C
abc1
abc2
abc3
}

[lzybkr]

Is this block necessary? InvokePrompt is called immediately after the call site that passes menuSelect: false and InvokePrompt is correctly updating these fields.

I tested briefly with these commented out and didn't see any problems.

[jianyunt]

You are right. We do not need it. I added it because I used Console.Write("\n") before. Removed it now. Thanks.

[jianyunt]

@lzybkr Yes we need to keep this Line https://github.com/lzybkr/PSReadLine/blob/2f57fe5091e0e0cd343b86851dc31c9bdcb2afd5/PSReadLine/Completion.cs#L501 to avoid the ArgumentOutOfRangeException exception.

[jianyunt]

Yes. we needed it. On Windows, I do not see problem either. But on Linux the System.ArgumentOutOfRangeException occurs on the EnsureMenuAndInputIsVisible() , Singleton._initialY -= toScroll becomes negative when rows is large.

[jianyunt]

You are right. We do not need it. I added it because I used Console.Write("\n") before. Removed it now. Thanks.