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 to run multiple actions. #11045

Merged
6 commits merged into from
Aug 31, 2021

Conversation

Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Aug 26, 2021

Summary of the Pull Request

Add a new action that can contain multiple other actions.

References

PR Checklist

  • Closes Add a ShortcutAction for running multiple ShortcutActions #3992
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema 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

Detailed Description of the Pull Request / Additional comments

Creates a shortcut action that allows a list of actions to be specified as arguments. Steals a bunch of the serialization code from my other pr. Overall, because I had the serialization code written already, this was remarkably easy.

I can't think of any combined action to be added to the defaults, so I think this is just a thing for the documentation unless someone else has a good example. I know there are lot of times when the recommended workaround is "make an action with commandline wt.exe ..." and this could be a good replacement for that, but that is all personalized.

I didn't add this to the command line parsing, since the command line is already a way to run multiple actions.

Validation Steps Performed

Created a new command, confirmed that "Move right->down" showed up in the command palette, and that running it did the correct behavior (moving right one pane, then down one pane).

      {
        "command": {
          "action": "multipleActions",
          "name": "Move right->down",
          "actions": [
            {"action":  "moveFocus", "direction": "right" },
            {"action":  "moveFocus", "direction": "down" },
          ]
        }
      }

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 26, 2021
@zadjii-msft
Copy link
Member

Before even reading the code, thank you so much for doing this one, I've meant to for so long. It's one I've really wanted 😄

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

The code here is simple enough to follow, and it looks good to me. I'm gonna temporarily block over the name discussion. I'll make sure to bring this up with the team (though that might not be till monday...)

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved
return _Name;
}

return RS_(L"MultipleActionsMissingName");
Copy link
Member

Choose a reason for hiding this comment

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

This raises a good point - this is another example of an action that doesn't really make sense unless the user gives it a manual name. I think the precedent we have here is the sendInput action, we just leave the name as the empty string if it wasn't given one. What that should do is force the user to give it a name if they want to see it in the command palette, or they can leave it nameless and only use it via a keybinding. (Granted, this has also resulted in #10981).

I'd think we could just follow that precedent here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm, I should remove this entirely and always just return _Name and force the user to deal with it?

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I might remove the _Name member entirely. Command already has a name, so the command palette can just use that one (if the user provides one). Granted, the json then becomes:

      {
        "command": {
          "action": "multipleActions",
          "actions": [
            {"action":  "moveFocus", "direction": "right" },
            {"action":  "moveFocus", "direction": "down" },
          ]
        },
        "name": "Move right->down",
      }

instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the Name property.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 26, 2021
@zadjii-msft zadjii-msft added Needs-Discussion Something that requires a team discussion before we can proceed and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 26, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.12 milestone Aug 26, 2021
@Rosefield
Copy link
Contributor Author

Note that despite this saying there are no merge conflicts, the build will break on merge because of multiple definitions of the IVector converter. If this is approved before #10972 I'll make that fix here.

@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 30, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks :)

As always, super excited for this one to land hahaha. I've wanted this for a while now.

@carlos-zamora
Copy link
Member

Oh, don't forget to update the docs please!

Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx

@Rosefield
Copy link
Contributor Author

For future me problems, is there a way to mute msftbot?

image

@carlos-zamora
Copy link
Member

For future me problems, is there a way to mute msftbot?

omg, yeah, your best bet would be to create an outlook rule to automatically delete them. I keep meaning to do that for myself haha

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

STOKED to land this.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 8d81497 into microsoft:main Aug 31, 2021
@Rosefield Rosefield deleted the feature/gh3992-run-multiple-actions branch August 31, 2021 20:44
Rosefield added a commit to Rosefield/terminal-1 that referenced this pull request Sep 1, 2021
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost pushed a commit that referenced this pull request Dec 13, 2022
### ⇒ [doc link](https://github.com/microsoft/terminal/blob/dev/migrie%2Fs%2F642-logging/doc/specs/drafts/%23642%20-%20Buffer%20Exporting%20and%20Logging/%23642%20-%20Buffer%20Exporting%20and%20Logging.md) ⇐

## Summary of the Pull Request

This is an intentionally brief spec to address the full scope of #642. The
intention of this spec is to quickly build consensus around all the features we
want for logging, and prepare an implementation plan.

### Abstract

> A common user need is the ability to export the history of a terminal session to
> a file, for later inspection or validation. This is something that could be
> triggered manually. Many terminal emulators provide the ability to automatically
> log the output of a session to a file, so the history is always captured. This
> spec will address improvements to the Windows Terminal to enable these kinds of
> exporting and logging scenarios.

## PR Checklist
* [x] Specs: #642
* [x] References: #5000, #9287, #11045, #11062
* [x] I work here

## Detailed Description of the Pull Request / Additional comments
_\*<sup>\*</sup><sub>\*</sub> read the spec  <sub>\*</sub><sup>\*</sup>\*_

## Open Discussion
* [ ] What formatting string syntax and variables do we want to use?
* [ ] How exactly do we want to handle "log printable output"? Do we include backspaces? Do we only log on newlines?
* [ ] > maybe consider even simpler options like just `${date}` and `${time}`, and allow for future variants with something like `${date:yyyy-mm-dd}` or `${time:hhmm}`
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a ShortcutAction for running multiple ShortcutActions
3 participants