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

[PowerToys Run] Add Suport for Commandline arguments in Program Plugin #5791

Merged
merged 7 commits into from
Sep 17, 2020

Conversation

royvou
Copy link
Contributor

@royvou royvou commented Aug 7, 2020

Summary of the Pull Request

Add support for CommandLine arguments to the Program Plugin

PR Checklist

  • Applies to Allow Command Line args in PowerToys Run #5724
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

Adds Support for CommandLine arguments to the Program Plugin.

Validation Steps Performed

  • Part 1
    • Ran an UWP Program with commandline arguments
    • Validate it ran as expected
  • Part 2
    • Ran an Win32 Program with commandline arguments
    • Validate it ran as expected
  • Part 3
    • Added Unit Tests :)

@royvou
Copy link
Contributor Author

royvou commented Aug 8, 2020

The PR Is "Draft" because it needs some work to determine where the "Searchtext" should be resolved to the application / and where it would start passing the rest as arguments (E.g. Microsoft Edge is currently parsed as "Microsoft for application, Edge as arguments)).

Currently two options
"--" seperates search text with argumentlist
"-", "/" before a word (and not the first text, at least second or third)

@htcfreek
Copy link
Collaborator

htcfreek commented Aug 8, 2020

What about doing it like windows search? Accept parameters only when the user enters the exe file name.

input result
firefox.exe -profile firefox.exe -profile
firefox -profile /no result/
firefox Mozilla Firefox

@royvou royvou force-pushed the feature/programcommandlinearguments branch 3 times, most recently from 393eae7 to 8fc2fd3 Compare August 12, 2020 18:25
@royvou
Copy link
Contributor Author

royvou commented Aug 12, 2020

@htcfreek i don't think this would be great, since it will not be possible to launch the programs via their description (And do all UWP programs have an .exe? Open question))

Currently i implemented in the following way.

  • If the search query contains a "--", use everything behind it as the queryarguments
  • If the search query (starting at term 2) contains a - or / it will parse that + rest as query arguments
  • Otherwise parse everything to the query and keep arguments empty.

Doing my own testing now to see how i like the feature (you can too) and will add tests for the parsers later :)

@royvou royvou force-pushed the feature/programcommandlinearguments branch 5 times, most recently from e927fcf to d6fe003 Compare August 13, 2020 19:23
@royvou royvou marked this pull request as ready for review August 13, 2020 20:33
@htcfreek
Copy link
Collaborator

htcfreek commented Aug 14, 2020

The PR Is "Draft" because it needs some work to determine where the "Searchtext" should be resolved to the application / and where it would start passing the rest as arguments (E.g. Microsoft Edge is currently parsed as "Microsoft for application, Edge as arguments)).

Currently two options
"--" seperates search text with argumentlist
"-", "/" before a word (and not the first text, at least second or third)

What does happen if input is edge.exe --inprivate or edge -- --inprivate?

@royvou royvou force-pushed the feature/programcommandlinearguments branch 2 times, most recently from c84fdcb to 02b3c8b Compare August 14, 2020 16:37
@royvou
Copy link
Contributor Author

royvou commented Aug 14, 2020

Added the tests for it as well :)

Search Command arguments Reason
edge.exe --inprivate edge.exe --inprivate No ' -- ' found and no single dash for arguments
edge -- --inprivate edge --inprivate Found ' -- '

@htcfreek
Copy link
Collaborator

Added the tests for it as well :)

Search Command arguments Reason
edge.exe --inprivate edge.exe --inprivate No ' -- ' found and no single dash for arguments
edge -- --inprivate edge --inprivate Found ' -- '

I think the first case should be also possible because for example chrome uses double minus for parameters.

@royvou royvou force-pushed the feature/programcommandlinearguments branch 3 times, most recently from 1a5da61 to 86f64dd Compare August 17, 2020 18:13
@royvou
Copy link
Contributor Author

royvou commented Aug 17, 2020

Added support for the doubledash as well (And updated the regex to make it easier to add even more but don't think that will be needed). I thought the doubledash was more of the Linux style of commandline seperators but seems it's also used on Windows, so a good suggestion 👍

@royvou royvou force-pushed the feature/programcommandlinearguments branch from 86f64dd to 0af21f7 Compare August 20, 2020 17:39
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a UTF8 files, but the other files in this project are UTF8 + BOM, seems like a sensible change to add?

@crutkas
Copy link
Member

crutkas commented Aug 21, 2020

@royvou i know you were doing some stuff here a while ago, can we test this out?

@royvou
Copy link
Contributor Author

royvou commented Aug 21, 2020

@crutkas Yes! Latest commits were only merges to keep up with the latest master branch 😄

Tests should now also be ran by CI so that's awesome!

@htcfreek
Copy link
Collaborator

@royvou
Will we get this in for 0.21? Would be great if yes.

@royvou royvou force-pushed the feature/programcommandlinearguments branch from 0af21f7 to d003693 Compare August 24, 2020 17:38
@royvou
Copy link
Contributor Author

royvou commented Aug 25, 2020

@htcfreek I think this all depends on how much / if any feedback we'll get on this feature 👍 but i assume will be close if everything goes great.
@crutkas were you able to try it out yet?

Yesterday i saw another merge conflict so i rebased it on top of that again.

@htcfreek
Copy link
Collaborator

@crutkas
Will we get this in for 0.21.0 or do we need more time for testing?

@royvou royvou force-pushed the feature/programcommandlinearguments branch from d003693 to 2050815 Compare September 10, 2020 20:25
@royvou
Copy link
Contributor Author

royvou commented Sep 10, 2020

Rebased it on the latest master branch to resolve merge conflict on the csproj.

@htcfreek
Copy link
Collaborator

Can we merge this for 0.23?

@crutkas Have you tried this feature?

Copy link
Contributor

@ryanbodrug-microsoft ryanbodrug-microsoft left a comment

Choose a reason for hiding this comment

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

I really like this feature! Nicely done.

@crutkas crutkas merged commit b3833fc into microsoft:master Sep 17, 2020
@crutkas
Copy link
Member

crutkas commented Sep 18, 2020

@royvou i was testing this due to an old issue i found, #4497.

For CMD, this works but for terminal, this pattern does not.

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 18, 2020

@royvou i was testing this due to an old issue i found, #4497.

For CMD, this works but for terminal, this pattern does not.

@crutkas
What command have you entered?
I think it musst be written like wt.exe -- ping github.com.

@royvou
Copy link
Contributor Author

royvou commented Sep 19, 2020

@crutkas i would need to inspect but since neither use a - - or - / with argument I would expect it to be part of the 'program' instead of the arguments.

@royvou
Copy link
Contributor Author

royvou commented Sep 19, 2020

Current IProgramArgumentParser implementation assumes every keyword you type in (keywords are seperated by a space e.g. Microsoft Edge) would be part of the "Program" so wt ping github.com is determined to be the complete search string, which in turn will cause no arguments to be passed in :).

does everything via shell so that's no problem (They Assume the first 'keyword' (You need to escape/quote it and it must be an exact match).

I think there's a few suggestions on how to handle it

  • Suggest -- to users.
  • Create a special IProgramArgumentParser which causes everything after .exe (so wt.exe ping 1.1.1.1) be picked up correctly.
  • Create a special IProgramArgumentParser which special cases known terminals (WT/PWSH/Bash/cmd)
  • Change logic to try to search programs in 'steps' (Would be hard to get right though)

Will update the bug you mentioned later, i think #2 / #3 are good to add.

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.

4 participants