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

[PT Run] VirtualDesktopHelper & WindowWalker improvements #16325

Merged
merged 42 commits into from
Mar 7, 2022

Conversation

htcfreek
Copy link
Collaborator

@htcfreek htcfreek commented Feb 13, 2022

Summary of the Pull Request

image

This PR add the following improvements/fixes:

  • VirtualDesktopHelper in Wox.Plugin.Common
  • Wox.Infrastructure.Helper.OpenInShell() can execute command with hidden window
  • WindowWalker
    • Improved IsCloaked check to hide hidden uwp app windows correctly.
    • "Search on current desktop only" feature
    • Code improvements
    • Result features (ResultHelper class)
      • Add PID to Subtitle
      • Add desktop name to subtitle (Only when more than one desktop exists.)
      • ToolTip: name, process, desktop
      • Explorer setting info (on keyword search)
    • Context menu features (ContextMenuHelper class)
      • Kill process feature
      • Close window feature
    • Settings (Settings class)
      • ResultsFromVisibleDesktopOnly (default: off)
      • ShowHiddenUwpWindows
      • SubtitleShowPid (default: off)
      • SubtitleShowDesktopName (default: on)
      • ConfirmKillProcess (default: on)
      • KillProcessTree (default: off)
      • HideKillProcessOnElevatedProcesses (default: off)
      • HideExplorerSettingInfo (default: off)
      • StayOpenAfterKillAndClose (default: off)

Limitations for "Kill Process" feature

  • Killing explorer windows will only be allowed if each window is running in it's own process.
  • You can only kill elevated processes if you have admin permissions (uac).
  • If you kill the process of an uwp app window you kill all instances of the app. All windows are assigned to the same process.
  • Uwp app don't know their process until they are searched in non-minimized state.

Other open ToDo items

Screenshots

Note

in the screenshots, where does desk1, deskall, desk2 come from?

It's the window title of cmd. I have renamed the window titles to know/see which window is on which desktop when testing.

Screenshots - Settings

image

Screenshots - Search on visible desktop

pt-ww-cdesk-only

Screenshots - Tool tip and sub title

image

Screenshots - Explorer process information

image
pt-ww-exp2

Screenshots - context menu

image

image image

How does someone test / validate

Tested code changes in the local build.

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@htcfreek htcfreek self-assigned this Feb 13, 2022
@htcfreek htcfreek added Product-PowerToys Run Improved app launch PT Run (Win+R) Window Run-Plugin Things that relate with PowerToys Run's plugin interface labels Feb 13, 2022
@htcfreek
Copy link
Collaborator Author

@niels9001
I will need three icons:

  1. Kill process (must be font asset)
  2. Close window (must be font asset)
  3. Hidden uwp window - I thought on something like the in-private icon of edge.

Do you have an idea for icon 1 and 2? Can you please help me with icon 3?

@htcfreek
Copy link
Collaborator Author

@jaimecbernardo
What triggered spellcheck here? I didn't pull from master. 🤨🤔

@jaimecbernardo
Copy link
Collaborator

@jaimecbernardo What triggered spellcheck here? I didn't pull from master. 🤨🤔

I triggered it manually after checking which PRs seemed to have been affected. 😅

@github-actions
Copy link

github-actions bot commented Feb 20, 2022

@check-spelling-bot Report

🔴 Please review

See the files view or the action log for details.

Unrecognized words (1)

uac

Previously acknowledged words that are now absent abap abcd abcdefgh appcontainer atop autogenerates AZCLI azurecr binskim bios btm buildcommand buildtools cameligo CCB cdpxwin cdxml CEBB CFFDAFADA ChaseKnowlden cjs CleanCodeDeveloper cljs CPPARM CPPx crutkas csx CTLCOLORSTATIC defaultcommand Deuchert dupenv DUSTIN efgh errc errorlevel estdir etcore Firefox fsscript Gamebar graphql Grayscale gsuberland hmon iccex ICONINFORMATION IMonitor INITCOMMONCONTROLSEX INSTALLLOGATTRIBUTES INSTALLLOGMODE INSTALLUILEVEL Javascript julia Knowlden kotlin ktm kts LASTEXITCODE LEQ ligo linkid litcoffee MAINICON MAKELPARAM Minimizeallwindows mkdir moderncop msiexec MSIINSTALLER MSIL namings NATIVEFNTCTL neq netlify nocache php phps plx policheck popd powerquery psc psm Pui pushd pyc pyd pyi pyo pyz Qin rda rdata rdeveen rds reportbug rexit robocopy scm SETRANGE SETSTEP SHAREIMAGELISTS smallbasic spamming spdth sregex STEPIT stx svh SWITCHEND symlink symlinks systemverilog taskkill tbc tcl tlbimp trackpad typeparamref UAC UITo umd uninit UWPUI vbhtml verilog vse vsix WDK wdksetup wdkvsix We'd webclient webpack wifstream WINMSAPP WTL
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:htcfreek/PowerToys.git repository
on the PT_WwVdh branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
-H "Content-Type: application/json" \
"https://api.github.com/repos/microsoft/PowerToys/issues/comments/1046252381" > "$comment_json"
comment_body=$(mktemp)
jq -r ".body // empty" "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")

patch_add=$(perl -e '$/=undef; $_=<>; if (m{Unrecognized words[^<]*</summary>\n*```\n*([^<]*)```\n*</details>$}m) { print "$1" } elsif (m{Unrecognized words[^<]*\n\n((?:\w.*\n)+)\n}m) { print "$1" };' < "$comment_body")

update_files
rm $comment_body
git add -u
If the flagged items do not appear to be text

If items relate to a ...

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

  • binary file.

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@htcfreek
Copy link
Collaborator Author

Build fixed. Sorry for that.

@htcfreek
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 16325 in repo microsoft/PowerToys

@crutkas
Copy link
Member

crutkas commented Feb 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Works great for me.
Left some language nit comments.
I think the context menu items need a menu item for the default action as well (Show Window). I believe the other PTRun plugins also keep a button for the default action of the plugin in the UI.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 3, 2022

Works great for me.
Left some language nit comments.
I think the context menu items need a menu item for the default action as well (Show Window). I believe the other PTRun plugins also keep a button for the default action of the plugin in the UI.

Thank you. Which plugins have a default action button atm?

Copy link
Collaborator

@SeraphimaZykova SeraphimaZykova left a comment

Choose a reason for hiding this comment

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

Tested, works great!

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 3, 2022

Works great for me.
Left some language nit comments.
I think the context menu items need a menu item for the default action as well (Show Window). I believe the other PTRun plugins also keep a button for the default action of the plugin in the UI.

Thank you. Which plugins have a default action button atm?

Because most of the plugins don't have this and it doesn't make any sense to me I will only fix spelling the next days.

I don't see any advantage having this context menu button.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 3, 2022

I found a bug in the settings class that I have to fix.

Settings return default values in unit teste.

@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 6, 2022

Feedback is resolved and settings class is fixed. We now have Tests for the settings class.

@jaimecbernardo
Copy link
Collaborator

Works great for me.
Left some language nit comments.
I think the context menu items need a menu item for the default action as well (Show Window). I believe the other PTRun plugins also keep a button for the default action of the plugin in the UI.

Thank you. Which plugins have a default action button atm?

Because most of the plugins don't have this and it doesn't make any sense to me I will only fix spelling the next days.

I don't see any advantage having this context menu button.

You're right, for some reason I got that impression.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer. Product-PowerToys Run Improved app launch PT Run (Win+R) Window Run-Plugin Things that relate with PowerToys Run's plugin interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants