search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
165k stars 29.54k forks source link

Disabled formatters must unregister #70314

Closed jrieken closed 5 years ago

jrieken commented 5 years ago

This is part of #41882 and tracks callers of registerDocumentFormattingEditProvider and registerDocumentRangeFormattingEditProvider that don't dispose their registrations when being disabled.

For VSCode calling registerDocument[Range]?FormattingEditProvider adds a formatter, that for instance drives enablement of context menu actions, like "Format Selection" or "Format Document". Most formatters follow our recommendation of having a setting to enable/disable them. However, some formatters don't unregister when being disabled, returning a null/empty-edit instead. That's problematic, because it doesn't allow VSCode to update the UI correctly (e.g hiding the "Format Document" action) and it contributes to the problem of multiple formatters (#41882). A disabled, but registered formatter is still a formatter in VSCode terms, meaning VSCode will continue to ask it for formatting edits and VSCode will continue to have multiple formatter available for a language.

A correct formatter unregisters when being disabled, as shown in the sample below. The following list is composed of those formatters that conflict most often:

The snippet below check with vsocde.workspace.getConfiguration() if the formatter is enabled and re-checks whenever the setting has changes (see vscode.workspace.onDidChangeConfiguration). Listening to the config-change is important for a smooth formatter update, without a restart.

    // document formatter
    const formatter: vscode.DocumentFormattingEditProvider = {
        provideDocumentFormattingEdits(document: vscode.TextDocument): vscode.TextEdit[] {
            const firstLine = document.lineAt(0);
            if (firstLine.text !== '42') {
                return [vscode.TextEdit.insert(firstLine.range.start, '42\n')];
            }
        }
    };

    // have a function that adds/removes the formatter based
    // on a configuration setting
    let registration: vscode.Disposable | undefined;
    function registerFormatterIfEnabled() {
        const isEnabled = vscode.workspace.getConfiguration().get('fooLang.formatter.enabled', true);
        if (isEnabled && !registration) {
            // 👍good - only enabled formatter is added
            registration = vscode.languages.registerDocumentFormattingEditProvider('fooLang', formatter);
        } else if (!isEnabled && registration) {
            // 👍good - disabled formatter is removed
            registration.dispose();
            registration = undefined;
        }
    }

    // register at activate-time
    registerFormatterIfEnabled();

    // add/remove formatter when config changes
    vscode.workspace.onDidChangeConfiguration(event => {
        if (event.affectsConfiguration('fooLang.formatter.enabled')) {
            registerFormatterIfEnabled();
        }
    });
jrieken commented 5 years ago

@dbaeumer can you add a sample how to unregister a formatter in LSP land?

octref commented 5 years ago

Will do https://github.com/vuejs/vetur/issues/1121 for March.

dbaeumer commented 5 years ago

You need to register a formatter dynamically to be able to unregister it. It can not be registered via the return value from the initialize call. So instead of something like this:

    let result: InitializeResult = {
        capabilities: {
            textDocumentSync: TextDocumentSyncKind.Full,
            documentFormattingProvider: true
        }
    };

you need to do something like this:

let registration: Disposable | undefined;
connection.client.register(DocumentFormattingRequest.type, {
    documentSelector: ['javascript']
}).then((disposable) => {
    registration = disposable;
});

To unregister you call registration.dispose();

DanTup commented 5 years ago

@jrieken There was a note here about providing the setting name to VS Code to do this:

https://github.com/Microsoft/vscode/issues/41882#issuecomment-451221511

Is that still an option? It seems simpler than having to implement the code above for every extension. It also gets a bit complicated than the above if you register multiple document modes dynamically, and have document + onType formatters. And also, I think the above code isn't adding the subscription to context.subscriptions (is this required? I've been doing it for everything I register, even though it seems like VS Code should be able to track them itself).

multics commented 5 years ago

Does it make sense that leaving the disabled formatter registered? Is it possible that vscode unregisters the formatter when it is disabled automatically?

jrieken commented 5 years ago

@multics Two times "no". A disabled formatter must unregister so that it doesn't show in the UX anymore (see https://github.com/Microsoft/vscode/issues/70314#issue-420052214). Also, it's not possible to unregister them for us because we don't know the setting an extension uses. That means the responsibility is with the formatter

DanTup commented 5 years ago

Couldn't VS Code keep all registered formatters, but skip over them when formatting if their corresponding setting is disabled?

It doesn't matter to me now, I've already done my work - but it feels like it'd be a much simpler API than each extension having to do its own housekeeping.

jrieken commented 5 years ago

but skip over them when formatting if their corresponding setting is disabled?

And how would vscode know those settings and how interpret them?

DanTup commented 5 years ago

@jrieken you posted a comment here:

https://github.com/Microsoft/vscode/issues/41882#issuecomment-451221511

which sounded like it was the plan. We pass a string setting name to VS Code and it toggles the formatter based on that setting (which would be a boolean). I honestly thought that was the plan (and couldn't see any obvious reason why that was no longer the case), which is why I was surprised it wasn't so trivial when I came to do it.

jrieken commented 5 years ago

Please read the whole thread: https://github.com/Microsoft/vscode/issues/41882#issuecomment-451383501

DanTup commented 5 years ago

Aha! Apologies, I thought I'd read the whole thread, but I may have missed some comments collapsed by GH. Makes sense now, thanks!

iansan5653 commented 5 years ago

What happens if an extension with a registered formatter is literally disabled or uninstalled without restart?

octref commented 5 years ago

I added vetur.format.enable, will be released in 0.17.1.

jrieken commented 5 years ago

What happens if an extension with a registered formatter is literally disabled or uninstalled without restart?

In that case the deactivate function of that extension is being called. If the extension did everything right, e.g. manages its registrations correctly than the formatter is unregistered.

octref commented 5 years ago

Some of the reasons people didn't use dynamic registration are probably:

I created https://github.com/Microsoft/vscode-docs/issues/2522 for that.

However, if all formatters should have a boolean setting for enablement / disablement, do we still consider adding API to simplify the registration?

In LSP it could look like:

let result: InitializeResult = {
  capabilities: {
    documentFormattingProvider: {
      enablementSetting: 'prettier.enable'
    }
  }
}
DanTup commented 5 years ago

I was just trying to run some of my tests against LSP and discovered my setting to disable the formatter didn't work. Is there a way to handle this? @octref mentions LSP above but I'm not sure I understand the snippet. If I understand correctly, we need to use a VS Code setting to tell the LSP client not to call formatting on the server (the server doesn't really need to know, it just won't get called for formatting)?

octref commented 5 years ago

@DanTup Above was just a suggestion how it could look in LSP.

my setting to disable the formatter didn't work.

You should:

DanTup commented 5 years ago

@octref I badly described the issue - my test highlighted that this setting doesn't work when using LSP (so if I migrate users to LSP, the setting will seem broken), I wasn't specifically trying to fix the tests.

I'm not sure how best to make that setting actually affect the LSP server. I guess I could pass it in the initialisation options and then have the server pretend it doesn't support it but that seems weird (the client shouldn't tell the server what it supports, and it seems a fairly VS-Code-specific way to handle it).

It seems like maybe the LSP client should be able to handle this (for ex. if we can tell it to disable/re-enable the formatter?).

That said, I wonder if this is necessary now that VS Code can prompt for multiple formatters anyway. Maybe it's reasonable to just ditch the setting entirely for both LSP and non-LSP?

octref commented 5 years ago

I'm not sure how best to make that setting actually affect the LSP server.

https://github.com/microsoft/vscode/issues/70314#issuecomment-472336416 https://github.com/vuejs/vetur/commit/061e4cfe27bd896ffb017a5b6080dbe177264144

That said, I wonder if this is necessary now that VS Code can prompt for multiple formatters anyway. Maybe it's reasonable to just ditch the setting entirely for both LSP and non-LSP?

It's better to provide an option so users can use other features of your extension and not having to choose formatter each time.

DanTup commented 5 years ago

@octref aha! I'd overlooked that - I think that'll work fine - thanks! 👍