Skip to content

Add Name as tooltip when tab completing process ID #3664

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
Merged
daxian-dbw merged 4 commits into from
May 6, 2017
Merged

Add Name as tooltip when tab completing process ID #3664

daxian-dbw merged 4 commits into from
May 6, 2017

Conversation

@powercode
Copy link
Collaborator

@powercode powercode commented Apr 28, 2017

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

powercode commented Apr 28, 2017

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
Copy link
Contributor

@lzybkr lzybkr Apr 30, 2017

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

@powercode powercode Apr 30, 2017

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 powercode force-pushed the process-completion branch from 118b0ab to 8c02c06 Compare May 1, 2017 17:43
@powercode
Copy link
Collaborator Author

powercode commented May 1, 2017

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

@daxian-dbw daxian-dbw May 1, 2017

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

@powercode powercode May 3, 2017

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

@daxian-dbw daxian-dbw May 5, 2017

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

rkeithhill commented May 1, 2017

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

daxian-dbw commented May 5, 2017

@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?

@powercode powercode force-pushed the process-completion branch from 0220380 to 997e567 Compare May 5, 2017 23:56
daxian-dbw
daxian-dbw reviewed May 5, 2017
View reviewed changes
test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 Outdated
Copy link
Member

@daxian-dbw daxian-dbw May 5, 2017

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

@powercode powercode May 6, 2017

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 powercode force-pushed the process-completion branch from 997e567 to 3f64b47 Compare May 6, 2017 00:29
@powercode
Copy link
Collaborator Author

powercode commented May 6, 2017

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

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

@adityapatwardhan adityapatwardhan adityapatwardhan left review comments

+1 more reviewer

@lzybkr lzybkr lzybkr left review comments

Reviewers whose approvals may not affect merge requirements

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