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

#82105 Add Terminal Rename Command #84429

Merged
merged 7 commits into from
Nov 25, 2019
Merged

#82105 Add Terminal Rename Command #84429

merged 7 commits into from
Nov 25, 2019

Conversation

petevdp
Copy link
Contributor

@petevdp petevdp commented Nov 10, 2019

This PR fixes #82105 by registering a new command workbench.action.terminal.renameNoninteractive that allows the user to set the terminal title via an argument instead of interactively.
example usage:

{
        "key": "ctrl+t",
        "command": "workbench.action.terminal.renameWithArgs",
        "args": {
            "newName": "my new terminal name"
        }
}

The current changes acheive this, but a few things still have to be sorted out:

  • I'm open to suggestions for the name.
  • It seems possible to rename the terminal based on its index or even ID, if terminal IDs are exposed to the user. Would this be desirable behaviour? This would probably take the form of an optional argument index, and not changing the default behaviour.
  • How should I notify the user if the desired terminal doesn't exist? I'm guessing you can just throw an error, but if there's a preferred channel or preferred error message formatting, that would be good to adhere to. edit: 231d559 deals with this, but the error formatting still might not be ideal.
  • I see that there's an adjacent file called terminalCommands.ts. There are other commands similar to this one in terminalActions.ts but terminalCommands.ts obviously seems more appropriately named.

Any additional suggestions for preferred/extended functionality are welcome!

@msftclas
Copy link

msftclas commented Nov 10, 2019

CLA assistant check
All CLA requirements met.

@Tyriar Tyriar self-assigned this Nov 11, 2019
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Doh, forgot to post this review earlier

export class RenameActiveTerminalCommand extends Command {
public static readonly ID = TERMINAL_COMMAND_ID.RENAME_NONINTERACTIVE;
public static readonly LABEL = nls.localize(
'workbench.action.terminal.renameNoninteractive',
Copy link
Member

Choose a reason for hiding this comment

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

I think renameWithArgs is the better name, interactive/non-interactive has a very specific meaning when talking about shells so this avoids clashing with that terminology.

@Tyriar Tyriar added this to the November 2019 milestone Nov 25, 2019
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @petevdp, sorry about the delay. The command name is now renameWithArg

@Tyriar Tyriar merged commit 4491fd5 into microsoft:master Nov 25, 2019
@petevdp
Copy link
Contributor Author

petevdp commented Nov 25, 2019

Thanks @Tyriar!
And no problem, I'm in no rush!

@petevdp petevdp deleted the petevdp/add-terminal-rename-command branch November 25, 2019 16:05
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workbench.action.terminal.rename should accept name as an argument
3 participants