Simplify LSP settings #865
Conversation
crates/ark/src/lsp/state_handlers.rs
Outdated
| // For document configs we collect all pairs of URIs and config keys of | ||
| // interest in a flat vector | ||
| let document_keys = VscDocumentConfig::FIELD_NAMES_AS_ARRAY; | ||
| let mut document_items: Vec<ConfigurationItem> = | ||
| itertools::iproduct!(uris.iter(), document_keys.iter()) | ||
| .map(|(uri, key)| ConfigurationItem { | ||
| scope_uri: Some(uri.clone()), | ||
| section: Some(VscDocumentConfig::section_from_key(key).into()), | ||
| }) | ||
| .collect(); | ||
| items.append(&mut document_items); |
There was a problem hiding this comment.
Hmm isn't this important? I'm pretty sure we collected document config on a per open document basis and now we just collect it once with a scope_uri: None which doesn't seem right
crates/ark/src/lsp/state_handlers.rs
Outdated
| if changed { | ||
| lsp::spawn_diagnostics_refresh_all(state.clone()); | ||
| } |
There was a problem hiding this comment.
We dropped a refresh here
| let config: DocumentConfig = config.into(); | ||
|
|
||
| // Finally, update the document's config | ||
| state.get_document_mut(&uri)?.config = config; |
There was a problem hiding this comment.
Yea see we dropped the per document config update
lionel-
force-pushed
the
feature/top-level-assign-symbols
branch
from
bc8ec87 to
db257d2
Compare
lionel-
force-pushed
the
task/refactor-config
branch
4 times, most recently
from
1c4fa66 to
a4d669b
Compare
|
Sorry I totally missed we lost the doc updates when the AI moved code around! I've restored the logic. Note that Positron/Code do not support per-document settings so we can't actually test this with our IDE. This is mostly an overcomplication for now but this will be useful with other IDEs like vim or Emacs, perhaps Zed. I've checked that global settings are updated as expected, diagnostics in particular (which do need the refresh): Screen.Recording.2025-07-22.at.12.24.28.mov |
| for uri in uris.into_iter() { | ||
| let keys = document_keys.into_iter(); | ||
| let items: Vec<Value> = configs.by_ref().take(n_document_items).collect(); | ||
|
|
||
| let mut map = serde_json::Map::new(); | ||
| std::iter::zip(keys, items).for_each(|(key, item)| { | ||
| map.insert(key.into(), item); | ||
| }); | ||
|
|
||
| // Deserialise the VS Code configuration | ||
| let config: VscDocumentConfig = serde_json::from_value(serde_json::Value::Object(map))?; | ||
|
|
||
| // Now convert the VS Code specific type into our own type | ||
| let config: DocumentConfig = config.into(); | ||
| // Need to juggle a bit because `split_off()` returns the tail of the | ||
| // split and updates the vector with the head | ||
| let tail = remaining.split_off(DOCUMENT_SETTINGS.len()); | ||
| let head = std::mem::replace(&mut remaining, tail); | ||
|
|
||
| for (mapping, value) in DOCUMENT_SETTINGS.iter().zip(head) { | ||
| if let Ok(doc) = state.get_document_mut(&uri) { | ||
| (mapping.set)(&mut doc.config, value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I was imagining this double loop, which I find easier to understand?
let mut i = 0;
// Iterate `n_uris * n_document_settings` total times, setting each document setting
// on a per-uri basis
for uri in uris.into_iter() {
let uri = state.get_document_mut(&uri);
let Ok(doc) = uri else {
i += DOCUMENT_SETTINGS.len();
continue;
};
for setting in DOCUMENT_SETTINGS.iter() {
let value = std::mem::take(&mut document_configs[i]);
(setting.set)(&mut doc.config, value);
i += 1;
}
}Requires let mut document_configs above as well
There was a problem hiding this comment.
Hmm I think I prefer the iterator tools than a raw loop with an index
lionel-
force-pushed
the
task/refactor-config
branch
from
a4d669b to
eccee92
Compare
I think you'll like this one @DavisVaughan.
Branched from #859. I was concerned at how hard it was to add a new setting and section. I think the refactor in this PR will make things much simpler and easier:
Add a generic
Settingtype that allows us to flatten a representation of all our settings in a flat arraySETTINGSthat is easy to loop over. This drastically simplify the updating logic where we send all keys of interest to the client in a flat array, as we no longer need any bookkeeping.The setting objects in this array contain the conversion-from-json logic as simple methods assigned to a closure field. The default handling is still delegated to
Defaultmethods in our setting types.Remove the split between VS Code and LSP settings. If we want to add vscode agnostic settings in the future, we could add editor aliases in the
SETTINGSarray..