Skip to content

Print prompt and command when WriteInputToHost is true #1691

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

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

andyleejordan
Copy link
Member

This fixes PowerShell/vscode-powershell#3786 by simply printing the prompt before the input of the command run by F8. It's almost cosmetic-only, except it does invoke the user's prompt function again which could have side-effects. What's more, we'll still have an empty prompt before the invocation; however, I believe that to be an accurate representation of what's happening because that prompt is canceled per my explanation here: PowerShell/vscode-powershell#3786 (comment)

@andyleejordan
Copy link
Member Author

@PrzemyslawKlys, @JustinGrote, and @SeeminglyScience what do you think of this? I'm not sure I like it to be quite honest.

@PrzemyslawKlys
Copy link

Current behavior of prod version is:

  • if the prompt is empty - just use it
  • if the prompt has anything in it - insert everything above it, and move the text user typed after it

image

  • i have first typed chars, then pressed F5 and that's the result

This seems most obvious.

@SeeminglyScience
Copy link
Collaborator

LGTM to be honest. The side effects of running prompt again are fine imo. We are essentially pretending the user cancelled out of their current REPL, ran the command they F8'd and then restored the REPL.

@JustinGrote
Copy link
Collaborator

JustinGrote commented Feb 1, 2022

The only other way it could work is to capture the user's prompt every time and then replicate the characters as-is to avoid side-effects. I think if you have a prompt that isn't idempotent youre going to have other problems and this would just reveal them, so I don't think it's that big of a deal. LGTM, this is how I would have solved it too :)

EDIT: Alternatively, maybe issue an ANSI command to move the cursor back to where the existing prompt is before running the F8? Or does it always display on a newline?

@andyleejordan
Copy link
Member Author

  • and move the text user typed after it

Hm, I'll see if I can make this work but I'm not sure with the current implementation.

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Feb 1, 2022

We are cancelling PSReadLine for this right? When we do, it saves the current REPL input for us to restore later. I believe that's done automatically by PSRL.

All we should need to do is:

  1. Trigger the ReadLine cancellation token source
  2. Wait for PSRL to cancel and do not restart it
  3. Write a new prompt
  4. Write invocation text
  5. Invoke selected lines
  6. Restart ReadLine

That should result in the desired behavior.

Edit: Now that I think about it though, the actual writing of prompt would need to happen within the execution service. Ideally there would be a check of this pseudo code

if (ShouldInterruptForeground && WriteInputToHost)
{ 
    CancelPSReadLineAndWait();
    WritePrompt();
    WriteInputText();
    NowInvoke();
}

@andyleejordan
Copy link
Member Author

@SeeminglyScience There is something wrong with it. I'm debugging. I managed to accidentally reproduce PowerShell/vscode-powershell#3751 while just trying to manually save and then restore the user's input, not knowing what you just told me about PSReadLine magically doing it for us.

@andyleejordan
Copy link
Member Author

  • Wait for PSRL to cancel and do not restart it

@SeeminglyScience As far as I can see we do not wait for PSRL to cancel. Only entry-point is:

private string InvokeReadLine(CancellationToken cancellationToken)
{
return _readLineProvider.ReadLine.ReadLine(cancellationToken);
}

And it's just being synchronously called:

try
{
string prompt = GetPrompt(cancellationToken);
UI.Write(prompt);
string userInput = InvokeReadLine(cancellationToken);
// If the user input was empty it's because:
// - the user provided no input
// - the readline task was canceled
// - CtrlC was sent to readline (which does not propagate a cancellation)
//
// In any event there's nothing to run in PowerShell, so we just loop back to the prompt again.
// However, we must distinguish the last two scenarios, since PSRL will not print a new line in those cases.
if (string.IsNullOrEmpty(userInput))
{
if (cancellationToken.IsCancellationRequested || LastKeyWasCtrlC())
{
UI.WriteLine();
}
return;
}
InvokeInput(userInput, cancellationToken);
}

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Feb 1, 2022

@SeeminglyScience As far as I can see we do not wait for PSRL to cancel. Only entry-point is:

That very well could be fine as well. The only reason we had to do that before was because ReadLine was tied to an execution status event. So as long as the pipeline execution thread is moving to our new evaluate task after ReadLine cancellation completes we're still good to go. We should just need the evaluate task to write prompt and invocation text.

@andyleejordan
Copy link
Member Author

We should just need the evaluate task to write prompt and invocation text.

This part is fine and easy. It's saving the existing input (if there is any) and then restoring it that's proving tricky.

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Feb 1, 2022

This part is fine and easy. It's saving the existing input (if there is any) and then restoring it that's proving tricky.

Ah right that code is already where I was suggesting. For some reason I thought it was in the EvaluateHandler itself instead.

I'll have to dive into the code tonight to see where it's being cancelled, how it's being restored, and see if I can figure out what's going wrong there. Or not, wooo! 🎉

@andyleejordan
Copy link
Member Author

Umm nevermind...it's working perfectly once I combined it with my fix in #1690

@andyleejordan andyleejordan force-pushed the andschwa/print-another-prompt branch 2 times, most recently from ff88d24 to ba0f25b Compare February 1, 2022 18:16
@andyleejordan
Copy link
Member Author

WTF now it's not working. What did I do!?

@SeeminglyScience
Copy link
Collaborator

Consistently? Or we got one of them super fun race conditions goin' on?

@andyleejordan
Copy link
Member Author

Super fun race condition, it's half working. Mostly works, can sometimes cause it to lose the input.

@andyleejordan andyleejordan force-pushed the andschwa/print-another-prompt branch from 8d8d3ed to 44880b6 Compare February 1, 2022 18:55
@andyleejordan andyleejordan changed the title WIP: Print prompt for F8 Print prompt and command when WriteInputToHost is true Feb 1, 2022
@andyleejordan andyleejordan changed the title Print prompt and command when WriteInputToHost is true Print prompt and command when WriteInputToHost is true Feb 1, 2022
@andyleejordan andyleejordan marked this pull request as ready for review February 1, 2022 18:55
@andyleejordan
Copy link
Member Author

It definitely works much better than it was, and the occasional loss of input is not due to this change but is an extant bug, so marking as ready for review.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

@andyleejordan andyleejordan enabled auto-merge (squash) February 1, 2022 20:02
@andyleejordan andyleejordan merged commit b433b0f into master Feb 1, 2022
@andyleejordan andyleejordan deleted the andschwa/print-another-prompt branch February 1, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Display the command that is running in the PowerShell console on the same line
4 participants