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 action keywords for each plugin #6129

Merged
merged 10 commits into from
Oct 19, 2020
Merged

Conversation

gordonwatts
Copy link
Contributor

@gordonwatts gordonwatts commented Aug 23, 2020

Summary of the Pull Request

Added keywords for each plug-in the PowerToys Run module.

The keywords proposed are:

  • Calculator: =
  • Indexer: ?
  • Program: .
  • URI: //
  • Window Walker: <

All the plug-ins that were a member of the global plug-in list (used "*" as an action keyword in the json configuration) retain that.

PR Checklist

  • Applies to add ActionKeyword for Running processes #5823
  • CLA signed
  • 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 a discussion took place: add ActionKeyword for Running processes #5823. I was not directly involved in the discussion, other than reading it. This might impact a PR that is trying to remove the space between the action keyword and the search string (see

Info on Pull Request

See discussion in issue #5823. Changes most plugin.json files in the PowerToys Runner plug in to use actionKeywords rather than actionKeyword, and adds a non-global keyword as noted above.

Other Changes:

Two things were needed to make action keywords work properly. Hopefully, they do not impact other operations of PowerToys!

  • The WindowsWalker plugin was using RawQuery instead of Query to drive its searches. RawQuery, of course, contains the action keyword (like ":"). Fixed by changing RawQuery to Query.
  • QueryBuilder adds all global plug-ins to the dictionary of plug-in list for each query. When a plugin is a member of both global and nonglobal lists, it can be added twice to the plugin-pair dictionary. This behavior is protected with a simple if statement to prevent double insertion.

Oustanding Issues:

Need advice: the current settings infrastructure stores the keywords in the user's settings file. This will override what changes are done here. Normally this is fine - however, there is no UI for altering the keywords. Someone who has already run PowerToys and wants to take advantage of this will have to edit the settings json file by hand.

Validation Steps Performed

Ran the tests and also tested by hand

  • Some tests failed before (and after) I made modifications: the fancyzones tests and the rename tests.

- Added keywords for each plug-in
-  Change window walker search to use Query, not RawQuery (to get rid of action keyword)
- Make sure plug-ins that have action keywords as well as are in global list don't get added twice

All tests passed that passed before I started

Addressing issue microsoft#5823
@gordonwatts
Copy link
Contributor Author

Failed tests seem to be the rename - which looks like it is failing across many many tests not just mine. I am not sure how to mark as "ready for review" - but this does need input/help at the very least from maintainers.

@htcfreek
Copy link
Collaborator

htcfreek commented Aug 23, 2020

@gordonwatts

PR Checklist

Note: Normally the corresponding issue and not an commit id is named here.

I am not sure how to mark as "ready for review"

A PR is ready to review if it is not marked as draft.
You can define reviewers. And if you are not allowed to do so, you can write a comment and name the reviewers.

@gordonwatts
Copy link
Contributor Author

@gordonwatts

PR Checklist

Note: Normally the corresponding issue and not an commit id is named here.

I am not sure how to mark as "ready for review"

A PR is ready to review if it is not marked as draft.
You can define reviewers. And if you are not allowed to do so, you can write a comment and name the reviewers.

Thanks!

@crutkas
Copy link
Member

crutkas commented Aug 24, 2020

rerunning CI, failure was transient

@crutkas
Copy link
Member

crutkas commented Aug 24, 2020

CI passed. We're getting toward end of our 0.21 release window. Lets target this for 0.22 release window as i want to have more time to try this

@gordonwatts
Copy link
Contributor Author

Sounds great! Let me know what you need from me.

@crutkas
Copy link
Member

crutkas commented Sep 15, 2020

Playing around with this, liking it but the indexer didn't want to work. On 0.22 it does work and on master.

General rules, if a file can use it, i don't think it should a hot key, example & since & will pull up snip & sketch other apps.

https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names

Like:

  • Calculator: =
    • Exact lead you'd expect here

Unsure of hotkey:

  • Indexer: /

    • ? as you're querying the system.
  • URI: &

    • : as http:// ... It mimics a keyshortcut that you'd already be used to.
  • Running Process (Window Walker): :

    • < since the shell plugin uses >. Feels a bit ok.
  • Program: $

    • Thoughts is i want this to be quick, this is one of our main use cases
    • # feels more programmy
    • @ could maybe feel right here too
    • '/' is super quick at least on a US keyboard.

Don't think we need:

  • Folder: \
    • why: c:\ already pulls, unless i'm missing something, i couldn't get this to pop up with

@crutkas
Copy link
Member

crutkas commented Sep 15, 2020

@gordonwatts looking at your PR, really don't see why indexer wouldn't work

@gordonwatts
Copy link
Contributor Author

I noticed this last night when I tried the indexer for the first time. I thought I'd tested it. I'll get all the commits from master and then have a go at it. Actually, I see there are now conflicts, so I need to do this anyway, I suppose. I'll take your advice on keyboard shortcuts as well when I do that.

@gordonwatts
Copy link
Contributor Author

gordonwatts commented Sep 16, 2020

This is an update:

  • Pulled all changes from master, and resolved conflicts.
  • The folder and indexer plug-in suddenly started working!

Need to investigate (mostly note to myself):

  • Why $ github gets the "run command: github" as top billing rather than the "Github Desktop" program

Your proposed keyboard shortcuts are fine. I'll update those when I've figured out what is going on above.

Apologies - I have an early morning code review shift for my work so more tomorrow!

@crutkas
Copy link
Member

crutkas commented Sep 17, 2020

@gordonwatts, bet you got some weird point in time where maybe something broke it and we fixed it in a later commit

@gordonwatts
Copy link
Contributor Author

Why $ github always has the run command rising to the top

This took me a while (slightly embarrassed). Also, it doesn't always have it rise to the top, it turns out. Sometimes it is second in the list.

  • The program plugin actually does two things:
    • Looks for matches in UWP and win32 install programs
    • Looks for exact matches to run a program (like the .exe name).

In the case that there is an installed program with the same name as a .exe, they both get the same score. And then they do not sort in a stable way.

Example: If I type "$ github" I get back three results from the Program plugin: "run command: github", "Application: github", "Application: Github Desktop". The first two both get a score of 172, and the third gets 164 (I'm pretty sure those numbers are accurate).

I think the unstable sort here is a bug, but it isn't a bug for this PR. We can open another issue and fix it there if you think that is something that should be fixed. Now that I understand this behavior - I'm going to move onto getting the keys updated to your suggestions.

@gordonwatts
Copy link
Contributor Author

gordonwatts commented Sep 18, 2020

I have adjusted the keywords in the last push, and I checked that they were all working. I also updated the top of this MR to reflect the new keywords (I hope that was appropriate in this project!) I think those are the updates you asked me for, @crutkas.

Three points:

  1. Currently, if it detects a keyword, it still loads up the other global plugins. That means you can have the indexer searching for /github at the same time it is searching for the program github (indeed, on my system you get back mostly the github programs, but also some github file from the indexer). If you put the space in there / github then indexer items are not returned.

  2. Now that I've implemented it, i'm not 100% on the '/' - I have feet in both the linux and windows world, and there "/" means search in many editors that use keyboard commands. I suspect this will be fine as soon as my fingers re-learn these keywords.

  3. In order for the system to see the new settings in the plugin.json files, I had to delete my current settings file in C:\Users\xxx\AppData\Local\Microsoft\PowerToys\PowerToys Run\Settings\Settings.json. However, I notice that when the new version re-creates this file, the action keywords are no longer in the file (nice!). I'm not sure when that got updated, and I don't know how this will affect users. But if they have anything in their settings files w.r.t. action keywords, then they will not see these changes when they upgrade to the new version.

Didn't quite get the if statement right the first time I put it in.
@damnskippy
Copy link

damnskippy commented Sep 18, 2020

Hello, I had this suggestion elsewhere in #6691 to have different dedicated key activation shortcuts for launching powertoys-run in different modes for searching.

  • windows-only
  • windows-of-currently-active-app (to switch among windows/instances of same app)
  • files-only
  • apps-only (shell plugin)
  • open-url
  • all-of-the-above (current behavior)

Maybe this is what is already proposed above, it's not clear to me, so adding this suggestion here FWIW.
Being able to search among windows of current app would be very useful to have, please consider.

@gordonwatts
Copy link
Contributor Author

I like some of these! The "currently active app" might be especially cool, if it could do tabs in chrome/edge (I see that the tabs will appear in the atl-tab display at some point for edge).

I'd suggest those become separate requests, however, rather than adding to this PR (depending on the feelings of the project coordinators, of course).

The one thing that might impact this is if it is better to have a different windows-level keyboard shortcut instead of the keys you enter here. In that case, this should be closed, and we should start from scratch.

@damnskippy
Copy link

It would be significantly more efficient and quicker to be able to directly go into a specific search mode, than a 2 step process to first activate the app and then to go into a desired mode and then do the search. It may seem there's just one extra key stroke, but it's more than that practically speaking.

@damnskippy
Copy link

In pure window switching mode, it would be even better to list the user-chosen n number of most recently used windows in MRU order, as opposed to the current approach where the search window pops up blank or with the last search string. All of this makes for a fast, efficient text based alt-tab switcher on steroids. Just another thought.

@crutkas
Copy link
Member

crutkas commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas
Copy link
Member

crutkas commented Oct 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas
Copy link
Member

crutkas commented Oct 14, 2020

Playing with this, feels a lot more solid.

I have two requests that i feel make this natural.

  • for URL-> do // I tried this and if felt more natural and it works.
  • for programs, i didn't like / since it was too folder / URL like. (yes i know i suggested it)
    • Tried . and :
    • Honestly, i was liking the period for quick app search.

Thoughts? A lot is, what feels intutative and natural. I sync'ed with Craig from WSL and he felt they all seemed natural.

@ryanbodrug-microsoft / @alekhyareddy28 / @enricogior would love your opinion.

@htcfreek
Copy link
Collaborator

@crutkas
What do you think about using the ampersand & character for program/app search? - Like the call operator in Powershell.

@crutkas
Copy link
Member

crutkas commented Oct 14, 2020

i find & to be a stretch keystroke. happy to get more opinions.

@gordonwatts
Copy link
Contributor Author

I like the // idea. I'll wait to update for the conclusion of the search for app. I've been using it for a while, and I have to say, my fingers "learned" it. From my POV, I think once we choose something, our fingers will memorize it.

@alekhyareddy28
Copy link
Contributor

@crutkas, I agree with the Program plugin. I think / does not feel very intuitive to use for the program plugin. I think the others are fine and we could get used to them once we start using those action keywords.

Regarding the PR, since we're not executing the global plugins once we see an action keyword, I wanted to point out that we should be mindful of adding action keywords which might be a part of a valid query in the future. We might not get a few results because of this.

@gordonwatts I'll test out this PR and review it sometime today. Thank you for this!
Also, regarding testing the QueryBuilder you could pass the global plugins as an argument, like how the non-global plugins are being passed and that could be mocked/set to what we want while testing.

@gordonwatts
Copy link
Contributor Author

Ok. Just to make sure we are on the same page here. I should change:

  • The URL plug-in to "//"
  • The program plug-in to "."

For the testing, @alekhyareddy28, the thing that needs to be tested is how it handles the global plug-ins after dealing with the non-global plug-ins. That logic doesn't really come into play unless the global plug-in list is built, and that isn't something that is passed in as an argument - the routine pulls it from a global variable. I looked a little bit into the moq framework to see if it could do global variable injection, but it didn't look like it. However, my knowledge of this framework is pretty minimal, so it is quite possible I missed something.

@crutkas
Copy link
Member

crutkas commented Oct 15, 2020

@gordonwatts yes, please change that.

Copy link
Contributor

@alekhyareddy28 alekhyareddy28 left a comment

Choose a reason for hiding this comment

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

I did try out the changes in this PR and they look good to me.

A few comments -

  1. @crutkas, . seems to be a valid regex character that is valid for the indexer and ww plugin. On adding . as an action keyword we would not see those results anymore. I'm not sure if anyone is extensively using them but just wanted to point that out.
    image

So previously .txt would show up all the txt files but they won't show up once we set the action keyword of the program plugin to .

  1. @gordonwatts It would be great to have a few tests to ensure that we don't break any functionality in the future as well.
    We could analyze the plugin-query pairs that are returned by the Build function to see how the global plugins are handled. You could convert the Build function so as to take the GlobalPlugins as an argument. This should fix the issue and it should be easy to add tests for the same. This would make it quite similar to how the non-global plugins are being handled right now.

Modify it to something like -

public static Dictionary<PluginPair, Query> Build(ref string text, Dictionary<string, PluginPair> nonGlobalPlugins, List<PluginPair> GlobalPlugins)

This way you can pass the GlobalPlugins and then analyze the plugin-query pairs to see how the global plugins were handled.

Once the tests are in and the action keywords are updated to // and . as suggested by crutkas the PR should be good to get in. Thank you!

@alekhyareddy28
Copy link
Contributor

Clarification regarding the above comment. I was wrong about the period being treated as a regex by the indexer plugin. Indexer does not treat the Period as a regex. Even ww seems to have it's own scoring mechanism which just compares characters.
But presently as stated in the above comment, the user can search for all txt files using .txt. However, if we set the action keyword of the program plugin to ., when the user enters .txt, it is now treated as a search for txt within only the program plugin and may not return any results depending on the programs. The indexer results will not show up anymore. The rest of the plugin action keywords should not be an issue as they are restricted characters within the indexer plugin.

// Reserved keywords in oleDB
private readonly string reservedStringPattern = @"^[\/\\\$\%]+$|^.*[<>].*$";

@gordonwatts
Copy link
Contributor Author

Thanks, @alekhyareddy28, for the careful look!

  1. I made the changes to the shortcuts (it is easy enough to change them back), and updated the header of this PR.
  2. Indeed, ".txt" does not get you a text file. "?.txt" gets you the right thing.
  3. Given we can have 2 character keywords, we could use that feature to work around this. I'm open to suggestions. One nice thing about working on this project is I do not have to make a final decision here. It is good not to be in charge once in a while. ;-)
  4. The tests are a good idea - and what you describe @alekhyareddy28 sounds perfect. I'd not realized that modifying the API was an option. I'll do that. However, I can't get to that before the end of the weekend or early next week. I'm planning on taking a bit of a much-needed break this weekend. Apologies for not being able to get to it sooner.

@crutkas crutkas merged commit 4660dd4 into microsoft:master Oct 19, 2020
@alekhyareddy28
Copy link
Contributor

Issue #7378 captures the remaining work related to unit testing this functionality.

@ghost ghost mentioned this pull request Oct 26, 2020
@@ -1,6 +1,6 @@
{
"ID":"B4D3B69656E14D44865C8D818EAE47C4",
"ActionKeyword":"*",
"ActionKeyword": "*",
"Name":"Folder",
"Description":"Searcn and open folders",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Searcn -> Search

@damnskippy
Copy link

@crutkas @gordonwatts Any considerations/thoughts on being able invoke specific modes with a direct keyboard shortcut as suggested/discussed above? Just curious. Thanks.

@crutkas
Copy link
Member

crutkas commented Oct 31, 2020

@damnskippy, something discussed in #5273

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.

6 participants