PowerToys icon indicating copy to clipboard operation
PowerToys copied to clipboard

[cmdpal] Support invoke command in Bookmarks extension

Open moooyo opened this issue 8 months ago • 10 comments

Summary of the Pull Request

image image

  • 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
  • [ ] 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

moooyo avatar Apr 24 '25 04:04 moooyo

/azp run

moooyo avatar Apr 24 '25 04:04 moooyo

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 24 '25 04:04 azure-pipelines[bot]

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, or cmd.exe /k ipconfig, or pwsh foo.ps1, or py foo.py or path\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 avatar Apr 25 '25 11:04 zadjii-msft

@zadjii-msft Fair point. And this solves my short thought about not being able to select "Windows Terminal".

htcfreek avatar Apr 25 '25 13:04 htcfreek

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)

zadjii-msft avatar Apr 25 '25 13:04 zadjii-msft

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

To 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.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).

  • 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.

github-actions[bot] avatar Apr 27 '25 07:04 github-actions[bot]

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

To 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.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).

  • 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.

github-actions[bot] avatar Apr 27 '25 07:04 github-actions[bot]

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

To 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.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).

  • 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.

github-actions[bot] avatar Apr 27 '25 07:04 github-actions[bot]

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

To 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.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).

  • 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.

github-actions[bot] avatar May 06 '25 09:05 github-actions[bot]

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

moooyo avatar May 06 '25 09:05 moooyo

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

To 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.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).

  • 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.

github-actions[bot] avatar May 07 '25 02:05 github-actions[bot]

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.OpenInShell them 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:

  1. 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.
  2. 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)
  3. Remove unused string.
  4. Updated the PR's descrption.

moooyo avatar May 07 '25 07:05 moooyo

  1. 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.

moooyo avatar May 07 '25 08:05 moooyo

/azp run

moooyo avatar May 07 '25 08:05 moooyo

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 07 '25 08:05 azure-pipelines[bot]

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:

  1. 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.
  2. It will be more streamline if we guess the type first, and then let people to "change" the bookmark type later on.

yeelam-gordon avatar May 07 '25 09:05 yeelam-gordon

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:

  1. 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.
  2. 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)

yeelam-gordon avatar May 08 '25 00:05 yeelam-gordon

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:

  1. 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.
  2. 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.

moooyo avatar May 08 '25 00:05 moooyo

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.

moooyo avatar May 08 '25 02:05 moooyo

@zadjii-msft

any suggestion? I think we can move on.

moooyo avatar Jun 05 '25 08:06 moooyo

Close it because we have a better one. #40430

moooyo avatar Jul 15 '25 03:07 moooyo