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

Warning when invoking explorer.newFile (fix #100604) #109905

Merged
merged 1 commit into from Nov 3, 2020
Merged

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Nov 3, 2020

This PR fixes #100604

@bpasero bpasero added this to the November 2020 milestone Nov 3, 2020
@bpasero bpasero requested a review from isidorn November 3, 2020 10:25
@bpasero bpasero self-assigned this Nov 3, 2020
@isidorn
Copy link
Contributor

isidorn commented Nov 3, 2020

@bpasero I would prefer if we use a precondition to disable the command completely in that case.
What do you think?

@bpasero
Copy link
Member Author

bpasero commented Nov 3, 2020

@isidorn ah ok, my understanding was that the user explicitly binds this command to a certain key? In that case, when condition would not help? Check the screenshot from original issue.

@isidorn
Copy link
Contributor

isidorn commented Nov 3, 2020

@bpasero I think it would help even in that case. when case would help, a preconditoin would not.
precondition I think applies for the keybinding
if the when is false even if there is a keybinding it would not trigger it

@bpasero
Copy link
Member Author

bpasero commented Nov 3, 2020

@isidorn I think it is the other way around. The when condition is lost once the user defines the keybinding, by default it will not have any when condition.

Do you already have a context key for the explorer being available or not?

@isidorn
Copy link
Contributor

isidorn commented Nov 3, 2020

Try this WorkbenchStateContext.notEqualsTo('workspace')
Code pointer how I use it for example

when: ContextKeyExpr.and(WorkbenchStateContext.notEqualsTo('workspace'), RemoteNameContext.notEqualsTo(''), IsWebContext.toNegated()),

@bpasero
Copy link
Member Author

bpasero commented Nov 3, 2020

@isidorn actually you already do this here:

appendToCommandPalette(NEW_FILE_COMMAND_ID, { value: NEW_FILE_LABEL, original: 'New File' }, category, WorkspaceFolderCountContext.notEqualsTo('0'));
appendToCommandPalette(NEW_FOLDER_COMMAND_ID, { value: NEW_FOLDER_LABEL, original: 'New Folder' }, category, WorkspaceFolderCountContext.notEqualsTo('0'));

via WorkspaceFolderCountContext.notEqualsTo('0')

Problem is that you cannot do this on the level of commands:

CommandsRegistry.registerCommand({
id: NEW_FILE_COMMAND_ID,
handler: async (accessor) => {
await openExplorerAndCreate(accessor, false);
}
});
CommandsRegistry.registerCommand({
id: NEW_FOLDER_COMMAND_ID,
handler: async (accessor) => {
await openExplorerAndCreate(accessor, true);
}
});

Once you register a command, it needs to be executable always.

An alternative is to simply return early from the command if the explorer is not visible instead of showing this warning...

@isidorn
Copy link
Contributor

isidorn commented Nov 3, 2020

Then it seems like there is nothing smarter to do here.
Your approach improves the error message, so let's go with that -> approved.

@bpasero bpasero merged commit 57340b7 into master Nov 3, 2020
@bpasero bpasero deleted the ben/100604 branch November 3, 2020 17:50
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 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.

Warning when invoking explorer.newFile
2 participants