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.

Sign up for GitHub

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

Jump to bottom

Add Name as tooltip when tab completing process ID #3664

Merged
merged 4 commits into from
May 6, 2017
Merged

Add Name as tooltip when tab completing process ID #3664

merged 4 commits into from
May 6, 2017

Conversation

powercode
Copy link
Collaborator

No description provided.

adityapatwardhan
adityapatwardhan previously requested changes Apr 28, 2017
View reviewed changes
Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

Please add test similar to: https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Language/Parser/ExtensibleCompletion.Tests.ps1#L344

@powercode powercode force-pushed the process-completion branch 2 times, most recently from b10bb4a to c9edd3a Compare April 28, 2017 18:52
@powercode
Copy link
Collaborator Author

The failure seems unrelated to my checkin

@powercode powercode force-pushed the process-completion branch 3 times, most recently from 0d05b36 to 118b0ab Compare April 29, 2017 09:14
@lzybkr lzybkr assigned lzybkr and unassigned powercode Apr 30, 2017
lzybkr
lzybkr reviewed Apr 30, 2017
View reviewed changes
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Outdated
@@ -3205,8 +3205,10 @@ private static void NativeCompletionProcessCommands(string wordToComplete, strin
if (pattern.IsMatch(completionText))
{
var listItemText = completionText;
Copy link
Member

Choose a reason for hiding this comment

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

I've always thought the user experience wasn't quite right with this completion.

I wonder if it would be better to use listItemText instead of tooltip to show the process name and the process id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess you are right, since it is harder to see the tooltip

Copy link
Collaborator Author

@powercode powercode Apr 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There is some weird case here where I have a null/empty process name on macOS. Is that expected or a bug somewhere in the process code?

@powercode
Copy link
Collaborator Author

New version with listtext and tooltip "{id} - {name}"

daxian-dbw
daxian-dbw reviewed May 1, 2017
View reviewed changes
test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 Outdated
$cmd = 'Get-Process -Id '
$res = TabExpansion2 -inputScript $cmd -cursorColumn $cmd.Length
$res.CompletionMatches[0].CompletionText -match '^\d+$' | Should be true
$res.CompletionMatches[0].ToolTip -match '^\w' | Should be true
Copy link
Member

Choose a reason for hiding this comment

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

\w stands for "word character", usually [A-Za-z0-9_]

So ToolTip -match '^\w' doesn't guarantee it's in our expected form "<digits - string>".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't know that we get the name string. Some tests on mac fail with null (or maybe empty) string. But maybe we could have '^\d+ - '

Copy link
Member

Choose a reason for hiding this comment

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

'^\d+ - ' looks good :)

@rkeithhill
Copy link
Collaborator

Keep in mind that the PowerShell extension for VSCode uses the TooltipText for the detail text in IntelliSense. ListItemCompletion is used for sorting and the Label.

@daxian-dbw daxian-dbw dismissed adityapatwardhan’s stale review May 5, 2017 22:41

Tests have been added.

@daxian-dbw
Copy link
Member

@powercode can you please update the test with '^\d+ - '? Otherwise, the changes look good to me.
@rkeithhill do you have any concerns on the changes?

daxian-dbw
daxian-dbw reviewed May 5, 2017
View reviewed changes
test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 Outdated
It 'Should complete "Get-Process -Id " with Id and name in tooltip' {
$cmd = 'Get-Process -Id '
$res = TabExpansion2 -inputScript $cmd -cursorColumn $cmd.Length
$res.CompletionMatches[0].CompletionText -match '^\d+ - ' | Should be true
Copy link
Member

Choose a reason for hiding this comment

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

You changed the wrong one. It's the ToolTip test below that you should change the pattern to ^\d+ - '

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't do this in the middle of the night :) Fixing

@powercode
Copy link
Collaborator Author

pushed a rebased version

@daxian-dbw daxian-dbw merged commit 8d744f2 into PowerShell:master May 6, 2017
@powercode powercode deleted the process-completion branch January 26, 2018 19:04
@TravisEz13 TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Waiting on Author labels May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzybkr lzybkr lzybkr left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan left review comments

Assignees

@lzybkr lzybkr

Labels
Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@powercode @rkeithhill @daxian-dbw @lzybkr @adityapatwardhan @msftclas @TravisEz13