[cmdpal] Support invoke command in Bookmarks extension
Summary of the Pull Request
- Refactored some logic to make sure we can easily add more bookmark type.
- Support invoke command
- Add BookmarkType enum to categorize bookmarks
- Use JsonSerializer to serialize all i18n string to avoid invalid character.
- Forward compatibility. No issue if we upgrade from old bookmark ext.
PR Checklist
- [x] Closes: #38700
- [x] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
- [x] Tests: Added/updated and all pass
- [x] Localization: All end user facing strings can be localized
- [ ] Dev docs: Added/updated
- [ ] New binaries: Added on the required places
- [ ] JSON for signing for new binaries
- [ ] WXS for installer for new binaries and localization folder
- [ ] YML for CI pipeline for new test projects
- [ ] YML for signed pipeline
- [ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
/azp run
Azure Pipelines successfully started running 1 pipeline(s).
I don't think I love the type dropdown here. I don't really want people to have to think about what the type of thing is. Plus, if we add built-in support for python 2 and 3, then why not other things? why not bash or node or ...? What if they don't have python installed? Or pwsh?
I think it's cleaner for us to require the actual executable in the bookmark. And then we can heuristically figure out:
- Is this a URL -> launch it with the URL opener (we probably could move the Open in browser stuff from the web search plugin to the toolkit)
- Is it a path to a folder? -> add the open folder commands (we probably could even lift some stuff out of the indexer into the toolkit for that)
- else, it's something to
ShellExecute. (test.bat, orcmd.exe /k ipconfig, orpwsh foo.ps1, orpy foo.pyorpath\to\some_exe.exe with args, or heck even"path\to\My Word Doc.docx"etc) and we should just start that.
I know that's not necessarily as clean as just explicitly bookmarking "foo.py as a Python bookmark", but I think it's almost better to just have to bookmark "python foo.py".
(plus, I bet we could try to use the ThumbnailHelper to get the thumbnail of the exe for the bookmark)
@zadjii-msft Fair point. And this solves my short thought about not being able to select "Windows Terminal".
off-topic: it might be a hot take, but I don't think we should have the "open in Windows Terminal"-like options anymore. The Default Terminal setting is available back even on Windows 10 these days. We should just always respect that
(I believe I have similar notes in #38277)
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (5)
abosolutely bookmak exectuable exsiting splited
These words are not needed and should be removed
SHELLEXTENSION SHELLNEWVALUE SHGFIICON SHGFILARGEICONTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the yuleng/cmdpal/bookmarks branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/67debf50669c7fc76fc8f5d7f996384535a72b77/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/14689520447/attempts/1'
Errors (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :x: Errors | Count |
|---|---|
| :x: ignored-expect-variant | 2 |
| :warning: no-newline-at-eof | 1 |
See :x: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile 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). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.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.
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (4)
abosolutely bookmak exectuable exsiting
These words are not needed and should be removed
SHELLEXTENSION SHELLNEWVALUE SHGFIICON SHGFILARGEICONTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the yuleng/cmdpal/bookmarks branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/67debf50669c7fc76fc8f5d7f996384535a72b77/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/14689567484/attempts/1'
Errors (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :x: Errors | Count |
|---|---|
| :x: ignored-expect-variant | 2 |
| :warning: no-newline-at-eof | 1 |
See :x: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile 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). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.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.
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (3)
bookmak exectuable exsiting
These words are not needed and should be removed
SHELLEXTENSION SHELLNEWVALUE SHGFIICON SHGFILARGEICONTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the yuleng/cmdpal/bookmarks branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/67debf50669c7fc76fc8f5d7f996384535a72b77/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/14689709411/attempts/1'
Errors (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :x: Errors | Count |
|---|---|
| :x: ignored-expect-variant | 2 |
| :warning: no-newline-at-eof | 1 |
See :x: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile 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). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.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.
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (1)
descrption
These words are not needed and should be removed
localappdata pswd SHELLEXTENSION SHELLNEWVALUE SHGFIICON SHGFILARGEICONTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the yuleng/cmdpal/bookmarks branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/67debf50669c7fc76fc8f5d7f996384535a72b77/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/14856676418/attempts/1'
Errors (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :x: Errors | Count |
|---|---|
| :x: ignored-expect-variant | 2 |
| :warning: no-newline-at-eof | 1 |
See :x: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile 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). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.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.
How about the new one? Just call the command directly. So we don't need to consider which command type it is.
@zadjii-msft @htcfreek
@check-spelling-bot Report
:red_circle: Please review
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
Unrecognized words (1)
Placeholde
These words are not needed and should be removed
iextn localappdata pswd SHELLEXTENSION SHELLNEWVALUE SHGFIICON SHGFILARGEICONTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands
... in a clone of the [email protected]:microsoft/PowerToys.git repository
on the yuleng/cmdpal/bookmarks branch (:information_source: how do I use this?):
curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/67debf50669c7fc76fc8f5d7f996384535a72b77/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/14873968999/attempts/1'
Errors (2)
See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.
| :x: Errors | Count |
|---|---|
| :x: ignored-expect-variant | 2 |
| :warning: no-newline-at-eof | 1 |
See :x: Event descriptions for more information.
If the flagged items are :exploding_head: false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile 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). -
well-formed pattern.
If you can write a pattern that would match it, try adding it to the
patterns.txtfile.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.
Okay I have feedback that I don't think fits in one place in particular
I think we can probably make this a lot simpler, by getting rid of the File and Folder command types too. We can probably just
OpenInShellHelper.OpenInShellthem too.Trying to determine if it's a "command" or a "path" based on the presence of a literal space is probably not the right way to do that.
Consider:
cmd.exe(this is a "command" that doesn't have a space)c:\users\Mike\OneDrive - Microsoft\foo.png(this is a file that has a space in the path)I think we need to better be able to parse the string into (path, args...). I would think that System.CommandLine should be able to do that? But this is notoriously challenging
We may want to do the lazy ThumbnailHelper icon loading for files/folders/{the exe of a command}, rather than just the static File/Folder/Command icons
- But that could be just a follow-up
I only don't immediately recommend merging the web commands with the rest, because that one has Special Favicon Handling
- by all accounts, OpenInShellHelper.OpenInShell should Just Work for URLs too
- heck, I bet we could still probably figure out the favicon handling without too much trouble
Emm, you are right.
Made the following changes:
- Merged the file, folder and Web into OpenInShell. But keep the bookmark type in to avoid dup jug. Yeah, you are right. We can merge all of them into openInShell. Actually, favicon is not a blocker. I've moved this logic into IconHelper.
- Fetch file and folder icon (ha actually I want to create a new PR for it before but we need a big change for this PR. So I decide to do it in this PR)
- Remove unused string.
- Updated the PR's descrption.
- Merged the file, folder
@zadjii-msft
The only bug is we can not open a "oneDrive" folder as I did in the PR's descrption. But the normal folder is ok.
/azp run
Azure Pipelines successfully started running 1 pipeline(s).
I think it is difficult to "assume" what's URL, File/Folder, or Command. If we add more thing later on, it just not scalability here on what does a Command parsing means.
Two thoughts:
- Can we have "interaction" between extension? i.e. At the "Run Command", I can add say "Bookmark" it? Since people can try out the command at the "Run command", and then bookmark it.
- It will be more streamline if we guess the type first, and then let people to "change" the bookmark type later on.
I think it is difficult to "assume" what's URL, File/Folder, or Command. If we add more thing later on, it just not scalability here on what does a Command parsing means.
Two thoughts:
- Can we have "interaction" between extension? i.e. At the "Run Command", I can add say "Bookmark" it? Since people can try out the command at the "Run command", and then bookmark it.
- It will be more streamline if we guess the type first, and then let people to "change" the bookmark type later on.
Looks like from what Mike just added the issue thread #39261 , bookmark from other common extension like "All Apps", "Search Files (and Folder)", "Run Command", does make sense. There is already alias and shortcut management for bookmark, just not easy to be discovered at the bookmark UX itself (i.e. need go to setting page now)
I think it is difficult to "assume" what's URL, File/Folder, or Command. If we add more thing later on, it just not scalability here on what does a Command parsing means. Two thoughts:
- Can we have "interaction" between extension? i.e. At the "Run Command", I can add say "Bookmark" it? Since people can try out the command at the "Run command", and then bookmark it.
- It will be more streamline if we guess the type first, and then let people to "change" the bookmark type later on.
Looks like from what Mike just added the issue thread #39261 , bookmark from other common extension like "All Apps", "Search Files (and Folder)", "Run Command", does make sense. There is already alias and shortcut management for bookmark, just not easy to be discovered at the bookmark UX itself (i.e. need go to setting page now)
Yeah, agree.
Not suggest to get in for 0.91 except there is special reason
If possible, I want to get it in. But if not, it's ok to hold.
In fact, the original version also have many assumption. This PR at least I think it just add a new ability to invoke command.
So, If we want to consider how to get the correct bookmark type (through a new choices input or other way), we can do in the future.
@zadjii-msft
any suggestion? I think we can move on.
Close it because we have a better one. #40430