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

Support view type for creating new untitled file. #100034

Merged
merged 7 commits into from
Jun 15, 2020

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Jun 12, 2020

The PR makes opening an untitled custom/notebook editor possible. It includes two sets of changes

  • Move GlobalNewUntitledFileAction to command NEW_UNTITLED_FILE_COMMAND_ID
    • registerWorkbenchAction is replaced by MenuRegistry.addCommand, MenuRegistry.appendMenuItem and appendToCommandPalette. @bpasero can you confirm if it's all I need to do?
  • Add a second parameter viewType for the command. This one is straightforward and doesn't affect existing Ctrl/Cmd+N experience.

@rebornix rebornix self-assigned this Jun 12, 2020
@rebornix rebornix requested a review from bpasero June 12, 2020 20:54
@rebornix rebornix added this to the June 2020 milestone Jun 12, 2020
registry.registerWorkbenchAction(SyncActionDescriptor.from(CompareWithClipboardAction, { primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyCode.KEY_C) }), 'File: Compare Active File with Clipboard', category.value);
registry.registerWorkbenchAction(SyncActionDescriptor.from(ToggleAutoSaveAction), 'File: Toggle Auto Save', category.value);
registry.registerWorkbenchAction(SyncActionDescriptor.from(ShowOpenedFileInNewWindow, { primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyCode.KEY_O) }), 'File: Open Active File in New Window', category.value);

MenuRegistry.addCommand({
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing to me, shouldn't this move to the // Commands section and be CommandsRegistry.registerCommand?

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, isn't this command already registered via KeybindingsRegistry.registerCommandAndKeybindingRule

rebornix and others added 3 commits June 15, 2020 08:33
@@ -61,6 +60,11 @@ if (isMacintosh) {
// Commands
CommandsRegistry.registerCommand('_files.windowOpen', openWindowCommand);
CommandsRegistry.registerCommand('_files.newWindow', newWindowCommand);
MenuRegistry.addCommand({
Copy link
Member Author

Choose a reason for hiding this comment

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

@bpasero @sbatten

I registered this command to MenuRegistry as it's part of registerWorkbenchAction, which consists of

  • CommandsRegistry.registerCommand
  • KeybindingsRegistry.registerKeybindingRule
  • MenuRegistry.addCommand
  • MenuRegistry.appendMenuItem

I'm not 100% sure of the intention of holding a cache of commands in the MenuRegistry and MenuRegistry.getCommand is used in quite a few places.

@rebornix rebornix requested a review from bpasero June 15, 2020 15:43
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I would remove the call to MenuRegistry.addCommand as I do not understand it. I also think that Jo has a way to contribute actions all in one go.

export function registerAction2(ctor: { new(): Action2 }): IDisposable {

@rebornix
Copy link
Member Author

@bpasero I removed the menu command registration for now after discussing with @sbatten. Moving to registerAction2 can be a debt item later on since it involves more changes in the workbench/files.

@rebornix rebornix requested a review from bpasero June 15, 2020 17:34
@rebornix rebornix merged commit b95c725 into master Jun 15, 2020
@rebornix rebornix deleted the rebornix/untitled-file-viewtype branch June 15, 2020 18:48
@github-actions github-actions bot locked and limited conversation to collaborators Jul 30, 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.

2 participants