Add a watcher for clangd.path and clangd.arguments #117
Conversation
|
Hi @njames93, I wanted to add my opinion on this feature: I am not sure prompting to restart the server on settings change is necessary. As @HighCommander4 said in #116 the user, after modifying those clangd settings "almost certainly wants to restart clangd immediately". I would leave out the almost: in fact I cannot see any reason why a user would want to edit those settings and apply them later, and I actually think that a prompt asking them to confirm that, would be redundant if not annoying. Furthermore, I think that we do not even have the problem of restarting it too many times consecutively, because the event is triggered only on settings.json save, so an automatic restart wouldn't be a problem. On the other hand the lack of a prompt would not give any feedback to the user that the server has restarted, but again, I really think this should be completely transparent to the user. Of course this is only my opinion, I just wanted to share, given that I've worked on the same feature (sorry again for the double PR...) |
src/config-watchers.ts
Outdated
| export function activate(context: ClangdContext) { | ||
| if (config.get<string>('onConfigChanged') != 'ignore') { | ||
| const watcher = new ConfigFileWatcher(context); | ||
| } | ||
| vscode.workspace.onDidChangeConfiguration(event => { | ||
| let Settings: string[] = ['clangd.path', 'clangd.arguments']; | ||
| Settings.forEach(element => { |
There was a problem hiding this comment.
I'd convert this to a for in loop and add a break after the first succesful match, otherwise if multiple settings change at the same time the prompt might be shown repeatedly
|
I'm inclined to disagree that restarting the server should be transparent. There is a cost associated with restarting the server and on very large projects, with potentially many files open this can result in noticeable latency issues whiles the server is restarting and reloading preambles. For this reason we should inform the user that we need to restart the server. Obviously in the common case people won't be working on projects so large, but they can just hit yes, always and be done with it. |
|
Always hitting "yes" can be annoying too, especially if we factor in how many extensions tend to show prompts and notifications. Can we have this as a configuration option? Sort of "Automatically restart clangd" with possible values ranging from "whenever it is required/makes sense" to "never, manual restarts only". |
This supports a configuration option that will always restart the server if you change those settings. The prompt gives you options to restart the server on this instance only, always restart it or never restart it. If you set never or always restart you'll never get the prompt again. |
Oh, my apologies. I was late to the party and started reacting to the comments without checking the code. I've had a look and your logic seems perfect to me. |
|
I suggest to consider adding a few more settings to the watcher, such as If you want to consider all the settings that would require a restart, |
Also update the config description to be more generic.
When
clangd.pathorclangd.argumentsis updated, Prompt the user to restart the server.Addresses #116