Skip to content
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

Add the prediction ListView and also hook up with the CommandPrediction APIs introduced in PS 7.1 #1909

Merged
merged 18 commits into from
Nov 3, 2020

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Oct 28, 2020

PR Summary

Feature Overview

Major feature level changes:

  1. Add the ListView to the predictive intellisense feature in PSReadLine.
  2. Hook up PSReadLine with the CommandPrediction APIs introduced in PS 7.1, so when using PSReadLine with PS 7.1, a user can import a predictor module and PSReadLine can render the suggestions returned from the predictor module.

There will be 4 options for the PredictionSource: None, History, Plugin, HistoryAndPlugin.

  • None -- disable the predictive intellisense feature, which is the default.
  • History -- enable the predictive intellisense feature, and use the PSReadLine history as the only source.
  • Plugin -- enable the predictive intellisense feature, and use the plugins (CommandPrediction) as the only source.
  • HistoryAndPlugin -- enable the predictive intellisense feature, and use both history and plugin as the sources.

There will be 2 views for the PredictionViewStyle: InlineView and ListView.

  • InlineView -- the style as existing today, similar as in fish shell and zsh.
  • ListView -- suggestions are rendered in a drop down list, and users can select using UpArrow and DownArrow.

tabcom2

Two more color settings were added to support customization of the ListView:

  • ListPredictionColor -- set color for the leading > character and the trailing source name, e.g. [History]. By default, it uses DarkYellow as the foreground color.
  • ListPredictionSelectedColor -- set color for indicating a list item is selected. By default, it uses DarkBlack as the background color.

image

Handling of Commonly Used Keys

To improve the user experience, some commonly used keys/functionalities are specially handled:

  1. Escape (RevertLine) -- in the list view, it will revert to the original line, no matter whether or not a list item is selected, and clear the list view. So UpArrow and DownArrow can then be used to navigate history commands as normal.

tabcom2

  1. Ctrl+z (Undo) -- in the list view, it will revert to the original line, no matter whether or not a list item is selected, but keep the list view.

tabcom2

  1. Alt+n (DigitArgument) -- in the list view, the digit argument won't refresh the suggestion results, and it can be used together with UpArrow and DownArrow to speed up the selection.

tabcom2

  1. Moving cursor, or selection operations such as Shift+LeftArrow, Shift+Home, etc. won't refresh the suggestion results. This is very necessary for a good user experience.

tabcom2

  1. Switching between InlineView and ListView with a hotkey F2 (SwitchPredictionView).

tabcom2

Feedback Trigger

The CommandPrediction has 2 APIs for the host client to provide feedback to the predictor plugin/module:
OnSuggestionAccepted, OnCommandLineAccepted.

OnCommandLineAccepted is triggered when the current command line is accepted (AcceptLine). Call to AcceptLine due to Ctrl+c is filtered out.

OnSuggestionAccepted is triggered differently based on the prediction view in use:

  • InlineView -- callback is triggered when either the suggestion is accepted via RightArrow, or the next word of the suggestion is accepted via Ctrl+f (ForwardWord).
  • ListView -- callback is triggered when
    • a list item is selected and then any edit is done on the selected item. For example, typing new characters, deleting characters, moving cursor or selecting part of the item and then replace with new typing or delete the selected range.
    • a list item is selected and then the user types Enter to accept the item.

Work with downlevel PowerShell

On supported downlevel PowerShell versions, such as Windows PowerShell 5.1 and PowerShell 7.0:

  • Only History can be used as the prediction source. The Plugin and HistoryAndPlugin options for PredictionSource only work with PS 7.1+.
  • ListView works on all supported downlevel PowerShell versions when using the History prediction source.

image
image

We are still shipping only a single Microsoft.PowerShell.PSReadLine2.dll that is built targeting net461. To bridge it with different PowerShell runtimes (PS7.1+ vs. downlevel), I introduced the Microsoft.PowerShell.PSReadLine.Polyfiller.dll. The module structure is shown as follows:

C:\REPOS\PSREADLINE\BIN\DEBUG\PSREADLINE
│   Changes.txt
│   License.txt
│   Microsoft.PowerShell.PSReadLine2.dll
│   PSReadLine.format.ps1xml
│   PSReadLine.psd1
│   PSReadLine.psm1
│   SamplePSReadLineProfile.ps1
│   System.Runtime.InteropServices.RuntimeInformation.dll
│
├───net461
│       Microsoft.PowerShell.PSReadLine.Polyfiller.dll
└───net5.0
        Microsoft.PowerShell.PSReadLine.Polyfiller.dll
  • The net461 version of the Polyfiller.dll contains the stub definitions of CommandPrediction and related types.
  • The net5.0 version of the Polyfiller.dll contains type forwarders that forwards the CommandPrediction and related types to the 7.1+ version of System.Management.Automation.dll.

By loading the right Polyfiller.dll, the Microsoft.PowerShell.PSReadLine2.dll built against net461 can work on all supported versions of PowerShell.

To make sure the right Polyfiller.dll is loaded, PSReadLine registers a handler to AppDomain.CurrentDomain.AssemblyResolve upon module loading and unregisters the handler during module unloading. The handler decides which Polyfiller.dll to use based on the current .NET version (Environment.Version).

Build and Tests

Build scripts were updated to build PSReadLine with multi-target net461 and net5.0. Also, the artifacts are packaged in the structure as shown above.

12 xUnit tests were added and each of them contains quite amount of sub tests. The new test code is more than 1300 line.
To test the plugin scenario, I made the use of the new CommandPrediction APIs mockable, by adding corresponding member methods to IPSConsoleReadLineMockableMethods.

Also fix #1591

PR Checklist

Microsoft Reviewers: Open in CodeFlow

Copy link
Collaborator

@theJasonHelmick theJasonHelmick left a comment

Choose a reason for hiding this comment

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

Updated PM RFC #262 to reflect property and value changes, including keybindings, described here.

@ghost ghost removed the Needs-Author Feedback label Oct 29, 2020
@daxian-dbw
Copy link
Member Author

@SteveL-MSFT @PaulHigin and @iSazonov Thanks for the review! I addressed all comment except for the header to be used for new code in PSReadLine. Please take another look when you get a chance.

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

Just one minor comment.

Copy link

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

Interesting, how many does MSFT team want to support for Windows PowerShell? I mean polyfill.dll could be enhanced in separate repo and reused as nuget package.

@daxian-dbw
Copy link
Member Author

@iSazonov The way Polyfill.dll works in this PR was inspired by https://github.com/SeeminglyScience/AstPolyfillExample. This helps to minimize the code that needs to be cross compiled and make it easier to test. @SeeminglyScience's idea is to make it like the netstandard.dll -- having Windows PowerShell ship with the polyfill.dll with code stubs for new types, and having PowerShell 7.1+ ship with the polyfill.dll that forward the new types to S.M.A.

@iSazonov
Copy link

@iSazonov @SeeminglyScience's idea is to make it like the netstandard.dll -- having Windows PowerShell ship with the polyfill.dll with code stubs for new types, and having PowerShell 7.1+ ship with the polyfill.dll that forward the new types to S.M.A.

I'm just wondering if PowerShell Polyfill will be a separate project so that other developer can benefit from it.

@SeeminglyScience
Copy link
Contributor

SeeminglyScience commented Oct 30, 2020

I'm just wondering if PowerShell Polyfill will be a separate project so that other developer can benefit from it.

That's sort of difficult to do well without also shipping the 7.1 version of the polyfill with 7.1 itself. Otherwise you'd either need a proxy assembly that registers the resolve event handler, or require the package consumer to set one up themselves.

I think there is potentially a lot of value in shipping the TypeForwardedTo version of these assemblies with the version they're introduced, but that could get unmaintainable quickly. Every new API (or rather, every version with a new API) would need it's own assembly, in-box type forward version, and nuget polyfill version.

It's worth discussing but probably out of scope for this PR.

@daxian-dbw
Copy link
Member Author

@SeeminglyScience @iSazonov We can use PSReadLine as an experiment to see how the polyfiller idea works. Foreseeably, the polyfiller assemblies shipped in PSReadLine will be updated as new types/members exposed in PowerShell engine in 7.2.

… results;

2. Similarly, avoid refreshing suggestion results for `DigitArgument` binding, which only changes the status line, not the user input line;
3. Fix a bug -- `Ctrl+c` incorrectly triggers OnCommandLineAccepted;
4. Have OnSuggestionAccepted hooked up with relatively high accuracy.
@daxian-dbw
Copy link
Member Author

@SteveL-MSFT Can you please review again? Thanks!

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

The copyright headers can be updated as a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Predictive text behaves something odd
6 participants