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

Migrate system commands extension #452

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

moooyo
Copy link
Collaborator

@moooyo moooyo commented Feb 24, 2025

Summary of the Pull Request

Seems most of them are work well now...

Need to fix if possible:
1. Tooltips:
Seems we don't have a way to show tooltips for item itself? So, I need to create a tag to do it.

Use detail page to show the connection details.

2. ContentDialog
I don't find a easy way to create a content dialog inside the extension...
So, I use win32 MessageBox instead.

Successfully implemented it!

Video.Project.8.mp4

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • 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

This comment has been minimized.

This comment has been minimized.

@moooyo moooyo changed the title [Draft][DO NOT MERGE] Migrate system commands extension Migrate system commands extension Feb 25, 2025
@moooyo moooyo marked this pull request as ready for review February 25, 2025 09:13
@zadjii-msft
Copy link
Owner

Tooltips:
Seems we don't have a way to show tooltips for item itself? So, I need to create a tag to do it.

I think the "idomatic" way to do this might be with the Details member on IListItem, and setting ShowDetails to true on the Page. That would give you the side dietails panel which you can put all sorts of stuff into

image

ContentDialog

If you need the dialog to prompt the user before doing something - that we haven't implemented yet (#399).

If not - I think you've got two options:

  • if you're showing a message while dismissing the palette (the copy command is a good examplee), then:
    return CommandResult.ShowToast("Emptied the recycle bin");
    will show a little confirmation message when the palette closes
  • Or, if this is just a "status" message that you want to flash at the user, but keep the palette open, It say use:
      private readonly ToastStatusMessage _toast = new(new StatusMessage() { Message = "The recycle bin is empty", State = MessageState.Info });
      // ... 
      public override CommandResult Invoke()
      {
          // do the thing...
          _toast.Show();
      }
    (though, there might be a weird wacky footgun with StatusMessage if you're a built-in command provider. it can be fixed, just haven't gotten to it yet)

@moooyo
Copy link
Collaborator Author

moooyo commented Feb 26, 2025

@zadjii-msft
Actually, we need both of them. We need a confirm dialog to make user can confirm their empty recycle bin operation.
And we need a toast to show some information if bin is empty or we canceled the empty bin task.

So, there's another question, for the second usage you suggest, can we use it after the Invoke call finished? I mean, can we use it inside a async task and just fire and forget it in the Invoke function (such as, we start a async function to empty the bin but it throw exception. Then we need to show a toast to display the error message to user)? I‘m not sure if it will cause any problem.

  private readonly ToastStatusMessage _toast = new(new StatusMessage() { Message = "The recycle bin is empty", State = MessageState.Info });

And Details looks good for me. I will use it to replace the tag usage.

image

@moooyo
Copy link
Collaborator Author

moooyo commented Feb 26, 2025

And seems CommandResult.ShowToast not work now. Any insight?

Testing code:
image

Nothing happened...

@zadjii-msft
Copy link
Owner

YOu may need to merge main first, I literally implemented ShowToast last week 😝

But ideally, Invoke should be able to take as long as the command needs (without the host UI hanging as a result). I think for the kind of scenario you're using here, I was doing a .ConfigureAwait(false) on the async task before returning the CommandResult, so that we make sure we complete the async thing before returning.
(if that doesn't work we should figure out how to fix that)

@moooyo
Copy link
Collaborator Author

moooyo commented Feb 28, 2025

YOu may need to merge main first, I literally implemented ShowToast last week 😝

But ideally, Invoke should be able to take as long as the command needs (without the host UI hanging as a result). I think for the kind of scenario you're using here, I was doing a .ConfigureAwait(false) on the async task before returning the CommandResult, so that we make sure we complete the async thing before returning. (if that doesn't work we should figure out how to fix that)

Thanks.

CommandResult.ShowToast work for me but _toast.show not work.

I agree, but for this empty task. It may take a long time. just consider if you have a very big (such as 20G) recycle bin, it may take more than 1 minute to finish the empty task. And we need (at least the v1 plugin author think) to show some message to user after the task complete.

So, I think we need to keep the win32 messagebox in here at least for those purpose:

  1. confirmation dialog
  2. async notification (outside the extension/page/invoke lifecycle)

If in the future we successfully implement it in cmdpal (I'm happy to investigate), we can remove the win32 messagebox.

@zadjii-msft
Copy link
Owner

. And we need (at least the v1 plugin author think) to show some message to user after the task complete.

So, I think we need to keep the win32 messagebox in here at least for those purpose:

  1. confirmation dialog
  2. async notification (outside the extension/page/invoke lifecycle)

If in the future we successfully implement it in cmdpal (I'm happy to investigate), we can remove the win32 messagebox.

Okay I think the way to do this with the most "native command palette feel" would be:

  • Return a CommandResult.Confirm(new ConfirmationArgs(){ PrimaryCommand = actuallyEmptyRecycleBinCommand}) to prompt for confirmation of the deletion
  • Then, in ActuallyEmptyRecycleBinCommand.Invoke(), show a status message with an indeterminate progress state. That'll let us keep the

hrumf. Seems like there's not a great way to

  • A: Prompt for confirmation,
  • B: show a toast then dismiss the palette,
  • C: then show another toast when the emptying is complete.

I can think of ways to do one of B or C, but not all three cleanly.

@moooyo
Copy link
Collaborator Author

moooyo commented Mar 13, 2025

. And we need (at least the v1 plugin author think) to show some message to user after the task complete.
So, I think we need to keep the win32 messagebox in here at least for those purpose:

  1. confirmation dialog
  2. async notification (outside the extension/page/invoke lifecycle)

If in the future we successfully implement it in cmdpal (I'm happy to investigate), we can remove the win32 messagebox.

Okay I think the way to do this with the most "native command palette feel" would be:

  • Return a CommandResult.Confirm(new ConfirmationArgs(){ PrimaryCommand = actuallyEmptyRecycleBinCommand}) to prompt for confirmation of the deletion
  • Then, in ActuallyEmptyRecycleBinCommand.Invoke(), show a status message with an indeterminate progress state. That'll let us keep the

hrumf. Seems like there's not a great way to

  • A: Prompt for confirmation,
  • B: show a toast then dismiss the palette,
  • C: then show another toast when the emptying is complete.

I can think of ways to do one of B or C, but not all three cleanly.

By our newly implemented confirmation dialog, we only need to consider how to prompt a message after the empty bin task complete (the win32 message box). I think push a notification to the windows notification central may be a good idea? I mean, this one:
image

@moooyo moooyo mentioned this pull request Mar 13, 2025
11 tasks
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.

2 participants