GeezerVison

This is looking good to me. I want a change to build and test it out myself though as it's a lot of untestable new behavior.

Tested:

• Ctrl-C to kill single-line (doesn't yank, should it?)
• Ctrl-C to exit multi-line
• Ctrl-C is passed to running commands
• Ctrl-K works, and is yankable with Ctrl-Y
• Ctrl-Y can yank anywhere
• Ctrl-L clears, and keeps current line (even in multi-line)
• Ctrl-R reverse searches
• Ctrl-R incrementally searches
• Ctrl-R cycles backward through matches
• Ctrl-R reports when there are no more matches to be found
• Ctrl-R text does not match PSReadLine
• Ctrl-S does not cycle forward through history search
• $Host.UI.RawUI.BackgroundColor is customizable (MegaPatch that includes all of #717, plus RawUI fixes #730)
• $Host.UI.RawUI.ForegroundColor is not customizable
• Ctrl-Home does nothing, should it?
• Ctrl-End does nothing, should it?
• Ctrl-Home in PSReadLine on Windows moves to the top of the buffer
• Ctrl-End in PSReadLine on Windows moves to the end of the buffer

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/main.cs, line 576 [r1] (raw file):
Just some style notes:

• We can (and should) prefer var over a type; let the compiler deduce it to make refactoring easy later
• We should avoid this. as it is (almost) never necessary
• string vs String should be consistent
• We need to write some style documentation for all of PowerShell...

src/Microsoft.PowerShell.Linux.Host/main.cs, line 624 [r1] (raw file):
Is ToString() necessary? Shouldn't WriteLine, taking a string, implicitly do this? I don't know.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 87 [r1] (raw file):
Yay!

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 165 [r1] (raw file):
Does anything from here possibly throw? If so, we should probably reset TreatControlCAsInput in a finally block.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 214 [r1] (raw file):
Should these refs maybe become private class members?

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 615 [r1] (raw file):
Is there not a ui.Write(ForegroundColor, BackgroundColor, string)?

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 617 [r1] (raw file):
PSReadLine calls this bck-i-search: and when typing, shows bck-i-search: foo_

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 621 [r1] (raw file):
$"{searchResult}" is much preferred to String.Format (same with reverse-i-search)

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
We should handle ConsoleKey.S to searchPos++. I'm fine with a new issue for this instead though. PSReadLine calls this fwd-i-search:.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
PSReadLine displays failed-bck-i-search:.

Comments from the review on Reviewable.io

Just saw this:

/home/andrew/src/PowerShell-Linux |-/ (reverse-i-search)'c': $Host.UI.RawUI.BackgroundColor = 'Black'                              (failed reverse-i-search)'co': $Host.UI.RawUI.BackgroundColor = 'Black'                      (failed reverse-i-search)'con': $Host.UI.RawUI.BackgroundColor = 'Black'                     (failed reverse-i-search)'cons': $Host.UI.RawUI.BackgroundColor = 'Black'                    (failed reverse-i-search)'conso': $Host.UI.RawUI.BackgroundColor = 'Black'                   (failed reverse-i-search)'consol': $Host.UI.RawUI.BackgroundColor = 'Black'                  (failed reverse-i-search)'console': $Host.UI.RawUI.BackgroundColor = 'Black'

Only it was wrapping at my Window border. Is it fixable?

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

We also need to initialize $Host.UI.RawUI.BackgroundColor and $Host.UI.RawUI.ForegroundColor on start to solve the first issue in #714.

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

To answer some of your concerns:

1. Ctrl-R text. I took the text from bash, since it has a similar one-line behavior.
2. Ctrl-S. I mentioned this at stand up, that it may not make sense to implement forward search. Everyone agreed.
3. Foreground color is not customizable? Please elaborate. It seems to work for me.
4. Ctrl-Home and Ctrl-End. I didn't see a requirement on this. It wasn't the intention to implement every key shortcut.
5. Console colors default to -1 at startup. Once you override, the -1 goes away. If you have default value from profile, you also will not see -1.

src/Microsoft.PowerShell.Linux.Host/main.cs, line 624 [r1] (raw file):
It was necessary because compiler complained that it could not convert from PSObject to String, if I remember correctly.

Comments from the review on Reviewable.io

1. We're trying to emulate PSReadLine, not Bash
2. Ctrl-S should be implemented since PSReadLine does so, but this can be done separately
3. Oh; I expected it to change the color of the text in the prompt; it just changes cmdlet output color
4. I don't mind them being unimplemented; it just looked like you had
5. They appear to be set on Windows, so we should do the same

Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 7 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 165 [r1] (raw file):
Nothing here is supposed to throw. And if something does throw, how should we handle it? Abort the input like ctrl-c? In theory, no valid command will get executed, so not changing TreatControlCAsInput to false would not be catastrophic .

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 214 [r1] (raw file):
I could've implemented these bool's as class members. Not sure if it's worth changing though.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 615 [r1] (raw file):
Yes, I could use that. Wrote these before the migration to ui.write.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 617 [r1] (raw file):
Do we have to match PSReadline?

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 621 [r1] (raw file):
"much preferred" because? Not sure it's a big difference, but I will change it.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
It's not clear where forward search starts at, and native Window PS basically does nothing. I brought this up at stand up, and it's agreed that the lack of definition of where "current line" is makes implementing this a difficult thing, and not worth the effort now.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
Again, do we have to match PSReadline? I chose this implementation because it's fairly simple, and can be done in one-line. I don't claim this is a full-featured implementation of reverse-search. In fact, PSReadLine's implementation does a lot more, especially when it comes to multi-line commands. I chose not to spend days to support multi-line recall right now.

Comments from the review on Reviewable.io

I don't care if we emulate PSReadLine, or Bash. Had I chosen PSReadLine, someone might say that we should emulate Bash. Is there a requirement that we should match PSReadLine exactly?

Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from the review on Reviewable.io

Yes, we are trying to emulate PSReadLine as close as reasonably possible.

Review status: all files reviewed at latest revision, 5 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 615 [r1] (raw file):
We probably should so it's consistent.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 617 [r1] (raw file):
Yes, that's the goal.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 621 [r1] (raw file):
Yes, much preferred. It's both more maintainable and significantly faster, as it allows the compiler to string smash.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
Forward search starts where backward search left off, literally just reverse direction from current result. If no reverse search was done, it is a no-op.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
That's fine, but I think thr text should match.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 5 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
Except that to go from reverse search to forward search, you need to type "Enter". And once you type "Enter", the "current" line is now the last line, and forward search becomes meaningless.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
Personally, I don't think we must match PSReadLine's wording. But I also don't have reasons not to. :) I will modify.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 5 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
That doesn't seem to be the case with PSReadLine on Windows.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
Thanks.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 4 unresolved discussions.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 615 [r1] (raw file):
Done.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 617 [r1] (raw file):
Done.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 621 [r1] (raw file):
Done.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 640 [r1] (raw file):
Not going to implement for now, since it was agreed at Stand Up that we don't need this right now.

src/Microsoft.PowerShell.Linux.Host/readline.cs, line 698 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

This is good, going to merge. We still have some open questions, but I'll create separate issues (some of the require more discussion than we should do here).