From 14f45ac1bcd48d40116b7e9f9102e5c9a948b749 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 5 Aug 2021 13:04:46 -0700 Subject: [PATCH 1/4] Log error when failing to load a theme in ThemeSelector --- zed/src/theme_selector.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/zed/src/theme_selector.rs b/zed/src/theme_selector.rs index 7b4456fd39..25816a41d2 100644 --- a/zed/src/theme_selector.rs +++ b/zed/src/theme_selector.rs @@ -107,10 +107,13 @@ impl ThemeSelector { fn confirm(&mut self, _: &(), cx: &mut ViewContext) { if let Some(mat) = self.matches.get(self.selected_index) { - if let Ok(theme) = self.registry.get(&mat.string) { - self.settings_tx.lock().borrow_mut().theme = theme; - cx.notify_all(); - cx.emit(Event::Dismissed); + match self.registry.get(&mat.string) { + Ok(theme) => { + self.settings_tx.lock().borrow_mut().theme = theme; + cx.notify_all(); + cx.emit(Event::Dismissed); + } + Err(error) => log::error!("error loading theme {}: {}", mat.string, error), } } } From 92df60f684e3e476213b3d9d60fe6efd65bf53b4 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 16 Aug 2021 13:22:10 -0700 Subject: [PATCH 2/4] In themes, replace variables with more general reference construct --- zed/assets/themes/_base.toml | 32 +++--- zed/assets/themes/dark.toml | 18 +-- zed/assets/themes/light.toml | 18 +-- zed/src/theme.rs | 207 +++++++++++++++-------------------- 4 files changed, 123 insertions(+), 152 deletions(-) diff --git a/zed/assets/themes/_base.toml b/zed/assets/themes/_base.toml index 20cc34ee0e..50a1b15e0c 100644 --- a/zed/assets/themes/_base.toml +++ b/zed/assets/themes/_base.toml @@ -1,9 +1,9 @@ [ui] -background = "$elevation_1" +background = "$surfaces.1" [ui.tab] -background = "$elevation_2" -text = "$text_dull" +background = "$surfaces.2" +text = "$text_colors.dull" border = { color = "#000000", width = 1.0 } padding = { left = 10, right = 10 } icon_close = "#383839" @@ -11,13 +11,13 @@ icon_dirty = "#556de8" icon_conflict = "#e45349" [ui.active_tab] -extends = "ui.tab" -background = "$elevation_3" -text = "$text_bright" +extends = "$ui.tab" +background = "$surfaces.3" +text = "$text_colors.bright" [ui.selector] -background = "$elevation_4" -text = "$text_bright" +background = "$surfaces.4" +text = "$text_colors.bright" padding = { top = 6.0, bottom = 6.0, left = 6.0, right = 6.0 } margin.top = 12.0 corner_radius = 6.0 @@ -31,17 +31,17 @@ border = { color = "#000000", width = 1.0 } padding = { top = 6.0, bottom = 6.0, left = 6.0, right = 6.0 } [ui.selector.active_item] -extends = "ui.selector.item" +extends = "$ui.selector.item" background = "#094771" [editor] -background = "$elevation_3" -gutter_background = "$elevation_3" -active_line_background = "$elevation_4" -line_number = "$text_dull" -line_number_active = "$text_bright" -text = "$text_normal" +background = "$surfaces.3" +gutter_background = "$surfaces.3" +active_line_background = "$surfaces.4" +line_number = "$text_colors.dull" +line_number_active = "$text_colors.bright" +text = "$text_colors.normal" replicas = [ - { selection = "#264f78", cursor = "$text_bright" }, + { selection = "#264f78", cursor = "$text_colors.bright" }, { selection = "#504f31", cursor = "#fcf154" }, ] diff --git a/zed/assets/themes/dark.toml b/zed/assets/themes/dark.toml index dae0bdf1ec..58872cc759 100644 --- a/zed/assets/themes/dark.toml +++ b/zed/assets/themes/dark.toml @@ -1,13 +1,15 @@ extends = "_base" -[variables] -elevation_1 = "#050101" -elevation_2 = "#131415" -elevation_3 = "#1c1d1e" -elevation_4 = "#3a3b3c" -text_dull = "#5a5a5b" -text_bright = "#ffffff" -text_normal = "#d4d4d4" +[surfaces] +1 = "#050101" +2 = "#131415" +3 = "#1c1d1e" +4 = "#3a3b3c" + +[text_colors] +dull = "#5a5a5b" +bright = "#ffffff" +normal = "#d4d4d4" [syntax] keyword = { color = "#0086c0", weight = "bold" } diff --git a/zed/assets/themes/light.toml b/zed/assets/themes/light.toml index ba7ec9915a..1cdb2c51dd 100644 --- a/zed/assets/themes/light.toml +++ b/zed/assets/themes/light.toml @@ -1,13 +1,15 @@ extends = "_base" -[variables] -elevation_1 = "#ffffff" -elevation_2 = "#f3f3f3" -elevation_3 = "#ececec" -elevation_4 = "#3a3b3c" -text_dull = "#acacac" -text_bright = "#111111" -text_normal = "#333333" +[surfaces] +1 = "#ffffff" +2 = "#f3f3f3" +3 = "#ececec" +4 = "#3a3b3c" + +[text_colors] +dull = "#acacac" +bright = "#111111" +normal = "#333333" [syntax] keyword = "#0000fa" diff --git a/zed/src/theme.rs b/zed/src/theme.rs index b1c09bf7c8..f1740f0d15 100644 --- a/zed/src/theme.rs +++ b/zed/src/theme.rs @@ -136,7 +136,7 @@ impl ThemeRegistry { return Ok(theme.clone()); } - let theme_data = self.load(name)?; + let theme_data = self.load(name, true)?; let mut theme = serde_json::from_value::(theme_data.as_ref().clone())?; theme.name = name.into(); let theme = Arc::new(theme); @@ -144,7 +144,7 @@ impl ThemeRegistry { Ok(theme) } - fn load(&self, name: &str) -> Result> { + fn load(&self, name: &str, evaluate_references: bool) -> Result> { if let Some(data) = self.theme_data.lock().get(name) { return Ok(data.clone()); } @@ -165,7 +165,7 @@ impl ThemeRegistry { .map(str::to_string) { let base_theme_data = self - .load(&base_name) + .load(&base_name, false) .with_context(|| format!("failed to load base theme {}", base_name))? .as_ref() .clone(); @@ -175,44 +175,44 @@ impl ThemeRegistry { } } - // Evaluate `extends` fields in styles - // First, find the key paths of all objects with `extends` directives - let mut directives = Vec::new(); - let mut key_path = Vec::new(); - for (key, value) in theme_data.iter() { - if value.is_array() || value.is_object() { + // Find all of the key path references in the object, and then sort them according + // to their dependencies. + if evaluate_references { + let mut references = Vec::new(); + let mut key_path = Vec::new(); + for (key, value) in theme_data.iter() { key_path.push(Key::Object(key.clone())); - find_extensions(value, &mut key_path, &mut directives); + find_references(value, &mut key_path, &mut references); key_path.pop(); } - } - // If you extend something with an extend directive, process the source's extend directive first - directives.sort_unstable(); + sort_references(&mut references); - // Now update objects to include the fields of objects they extend - for ExtendDirective { - source_path, - target_path, - } in directives - { - let source = value_at(&mut theme_data, &source_path)?.clone(); - let target = value_at(&mut theme_data, &target_path)?; - if let (Value::Object(mut source_object), Value::Object(target_object)) = - (source, target.take()) + // Now update objects to include the fields of objects they extend + for KeyPathReference { + source_path, + target_path, + } in references { - deep_merge_json(&mut source_object, target_object); - *target = Value::Object(source_object); - } - } - - // Evaluate any variables - if let Some((key, variables)) = theme_data.remove_entry("variables") { - if let Some(variables) = variables.as_object() { - for value in theme_data.values_mut() { - evaluate_variables(value, &variables, &mut Vec::new())?; + let source = value_at(&mut theme_data, &source_path).cloned(); + let target = value_at(&mut theme_data, &target_path).unwrap(); + if let Some(source) = source { + if let Value::Object(target_object) = target.take() { + if let Value::Object(mut source_object) = source { + deep_merge_json(&mut source_object, target_object); + *target = Value::Object(source_object); + } else { + Err(anyhow!( + "extended key path {:?} is not an object", + source_path + ))?; + } + } else { + *target = source; + } + } else { + Err(anyhow!("invalid key path {:?}", source_path))?; } } - theme_data.insert(key, variables); } let result = Arc::new(Value::Object(theme_data)); @@ -314,47 +314,41 @@ enum Key { } #[derive(Debug, PartialEq, Eq)] -struct ExtendDirective { +struct KeyPathReference { source_path: Vec, target_path: Vec, } -impl Ord for ExtendDirective { - fn cmp(&self, other: &Self) -> Ordering { +impl PartialOrd for KeyPathReference { + fn partial_cmp(&self, other: &Self) -> Option { if self.target_path.starts_with(&other.source_path) || other.source_path.starts_with(&self.target_path) { - Ordering::Less + Some(Ordering::Less) } else if other.target_path.starts_with(&self.source_path) || self.source_path.starts_with(&other.target_path) { - Ordering::Greater + Some(Ordering::Greater) } else { - Ordering::Equal + None } } } -impl PartialOrd for ExtendDirective { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -fn find_extensions(value: &Value, key_path: &mut Vec, directives: &mut Vec) { +fn find_references(value: &Value, key_path: &mut Vec, references: &mut Vec) { match value { Value::Array(vec) => { for (ix, value) in vec.iter().enumerate() { key_path.push(Key::Array(ix)); - find_extensions(value, key_path, directives); + find_references(value, key_path, references); key_path.pop(); } } Value::Object(map) => { for (key, value) in map.iter() { if key == "extends" { - if let Some(source_path) = value.as_str() { - directives.push(ExtendDirective { + if let Some(source_path) = value.as_str().and_then(|s| s.strip_prefix("$")) { + references.push(KeyPathReference { source_path: source_path .split(".") .map(|key| Key::Object(key.to_string())) @@ -362,18 +356,39 @@ fn find_extensions(value: &Value, key_path: &mut Vec, directives: &mut Vec< target_path: key_path.clone(), }); } - } else if value.is_array() || value.is_object() { + } else { key_path.push(Key::Object(key.to_string())); - find_extensions(value, key_path, directives); + find_references(value, key_path, references); key_path.pop(); } } } + Value::String(string) => { + if let Some(source_path) = string.strip_prefix("$") { + references.push(KeyPathReference { + source_path: source_path + .split(".") + .map(|key| Key::Object(key.to_string())) + .collect(), + target_path: key_path.clone(), + }); + } + } _ => {} } } -fn value_at<'a>(object: &'a mut Map, key_path: &Vec) -> Result<&'a mut Value> { +fn sort_references(references: &mut Vec) { + for i in 0..references.len() { + for j in (i + 1)..references.len() { + if let Some(Ordering::Greater) = &references[i].partial_cmp(&references[j]) { + references.swap(i, j) + } + } + } +} + +fn value_at<'a>(object: &'a mut Map, key_path: &Vec) -> Option<&'a mut Value> { let mut key_path = key_path.iter(); if let Some(Key::Object(first_key)) = key_path.next() { let mut cur_value = object.get_mut(first_key); @@ -384,63 +399,15 @@ fn value_at<'a>(object: &'a mut Map, key_path: &Vec) -> Resu Key::Object(key) => cur_value = value.get_mut(key), } } else { - return Err(anyhow!("invalid key path")); + return None; } } - cur_value.ok_or_else(|| anyhow!("invalid key path")) + cur_value } else { - Err(anyhow!("invalid key path")) + None } } -fn evaluate_variables( - value: &mut Value, - variables: &Map, - stack: &mut Vec, -) -> Result<()> { - match value { - Value::String(s) => { - if let Some(name) = s.strip_prefix("$") { - if stack.iter().any(|e| e == name) { - Err(anyhow!("variable {} is defined recursively", name))?; - } - if validate_variable_name(name) { - stack.push(name.to_string()); - if let Some(definition) = variables.get(name).cloned() { - *value = definition; - evaluate_variables(value, variables, stack)?; - } - stack.pop(); - } - } - } - Value::Array(a) => { - for value in a.iter_mut() { - evaluate_variables(value, variables, stack)?; - } - } - Value::Object(object) => { - for value in object.values_mut() { - evaluate_variables(value, variables, stack)?; - } - } - _ => {} - } - Ok(()) -} - -fn validate_variable_name(name: &str) -> bool { - let mut chars = name.chars(); - if let Some(first) = chars.next() { - if first.is_alphabetic() || first == '_' { - if chars.all(|c| c.is_alphanumeric() || c == '_') { - return true; - } - } - } - false -} - pub fn deserialize_syntax_theme<'de, D>( deserializer: D, ) -> Result, D::Error> @@ -488,13 +455,13 @@ mod tests { "themes/_base.toml", r##" [ui.active_tab] - extends = "ui.tab" + extends = "$ui.tab" border.color = "#666666" - text = "$bright_text" + text = "$text_colors.bright" [ui.tab] - extends = "ui.element" - text = "$dull_text" + extends = "$ui.element" + text = "$text_colors.dull" [ui.element] background = "#111111" @@ -502,7 +469,7 @@ mod tests { [editor] background = "#222222" - default_text = "$regular_text" + default_text = "$text_colors.regular" "##, ), ( @@ -510,10 +477,10 @@ mod tests { r##" extends = "_base" - [variables] - bright_text = "#ffffff" - regular_text = "#eeeeee" - dull_text = "#dddddd" + [text_colors] + bright = "#ffffff" + regular = "#eeeeee" + dull = "#dddddd" [editor] background = "#232323" @@ -522,7 +489,7 @@ mod tests { ]); let registry = ThemeRegistry::new(assets); - let theme_data = registry.load("light").unwrap(); + let theme_data = registry.load("light", true).unwrap(); assert_eq!( theme_data.as_ref(), &serde_json::json!({ @@ -533,7 +500,7 @@ mod tests { "width": 2.0, "color": "#666666" }, - "extends": "ui.tab", + "extends": "$ui.tab", "text": "#ffffff" }, "tab": { @@ -542,7 +509,7 @@ mod tests { "width": 2.0, "color": "#00000000" }, - "extends": "ui.element", + "extends": "$ui.element", "text": "#dddddd" }, "element": { @@ -558,10 +525,10 @@ mod tests { "default_text": "#eeeeee" }, "extends": "_base", - "variables": { - "bright_text": "#ffffff", - "regular_text": "#eeeeee", - "dull_text": "#dddddd" + "text_colors": { + "bright": "#ffffff", + "regular": "#eeeeee", + "dull": "#dddddd" } }) ); From 1a4bd3ab2ebc5b5e67bb5cfacc3dc9eb9cfad791 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 17 Aug 2021 15:19:45 -0700 Subject: [PATCH 3/4] Implement a topological sort for references in themes --- zed/src/theme.rs | 529 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 452 insertions(+), 77 deletions(-) diff --git a/zed/src/theme.rs b/zed/src/theme.rs index f1740f0d15..fbabb08579 100644 --- a/zed/src/theme.rs +++ b/zed/src/theme.rs @@ -9,7 +9,7 @@ use json::{Map, Value}; use parking_lot::Mutex; use serde::{Deserialize, Deserializer}; use serde_json as json; -use std::{cmp::Ordering, collections::HashMap, sync::Arc}; +use std::{collections::HashMap, fmt, mem, sync::Arc}; const DEFAULT_HIGHLIGHT_ID: HighlightId = HighlightId(u32::MAX); pub const DEFAULT_THEME_NAME: &'static str = "dark"; @@ -91,6 +91,30 @@ pub struct SelectorItem { pub label: LabelStyle, } +#[derive(Default)] +struct KeyPathReferenceSet { + references: Vec, + reference_ids_by_source: Vec, + reference_ids_by_target: Vec, + dependencies: Vec<(usize, usize)>, + dependency_counts: Vec, +} + +#[derive(Clone, Default, PartialEq, Eq, PartialOrd, Ord)] +struct KeyPathReference { + target: KeyPath, + source: KeyPath, +} + +#[derive(Debug, Default, Clone, PartialEq, Eq, PartialOrd, Ord)] +struct KeyPath(Vec); + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +enum Key { + Array(usize), + Object(String), +} + impl Default for Editor { fn default() -> Self { Self { @@ -178,39 +202,33 @@ impl ThemeRegistry { // Find all of the key path references in the object, and then sort them according // to their dependencies. if evaluate_references { - let mut references = Vec::new(); - let mut key_path = Vec::new(); + let mut key_path = KeyPath::default(); + let mut references = KeyPathReferenceSet::default(); for (key, value) in theme_data.iter() { - key_path.push(Key::Object(key.clone())); + key_path.0.push(Key::Object(key.clone())); find_references(value, &mut key_path, &mut references); - key_path.pop(); + key_path.0.pop(); } - sort_references(&mut references); + let sorted_references = references + .top_sort() + .map_err(|key_paths| anyhow!("cycle for key paths: {:?}", key_paths))?; // Now update objects to include the fields of objects they extend - for KeyPathReference { - source_path, - target_path, - } in references - { - let source = value_at(&mut theme_data, &source_path).cloned(); - let target = value_at(&mut theme_data, &target_path).unwrap(); - if let Some(source) = source { + for KeyPathReference { source, target } in sorted_references { + if let Some(source) = value_at(&mut theme_data, &source).cloned() { + let target = value_at(&mut theme_data, &target).unwrap(); if let Value::Object(target_object) = target.take() { if let Value::Object(mut source_object) = source { deep_merge_json(&mut source_object, target_object); *target = Value::Object(source_object); } else { - Err(anyhow!( - "extended key path {:?} is not an object", - source_path - ))?; + Err(anyhow!("extended key path {} is not an object", source))?; } } else { *target = source; } } else { - Err(anyhow!("invalid key path {:?}", source_path))?; + Err(anyhow!("invalid key path '{}'", source))?; } } } @@ -281,6 +299,295 @@ impl HighlightMap { } } +impl KeyPathReferenceSet { + fn insert(&mut self, reference: KeyPathReference) { + let id = self.references.len(); + let source_ix = self + .reference_ids_by_source + .binary_search_by_key(&&reference.source, |id| &self.references[*id].source) + .unwrap_or_else(|i| i); + let target_ix = self + .reference_ids_by_target + .binary_search_by_key(&&reference.target, |id| &self.references[*id].target) + .unwrap_or_else(|i| i); + + self.populate_dependencies(id, &reference); + self.reference_ids_by_source.insert(source_ix, id); + self.reference_ids_by_target.insert(target_ix, id); + self.references.push(reference); + } + + fn top_sort(mut self) -> Result, Vec> { + let mut results = Vec::with_capacity(self.references.len()); + let mut root_ids = Vec::with_capacity(self.references.len()); + + // Find the initial set of references that have no dependencies. + for (id, dep_count) in self.dependency_counts.iter().enumerate() { + if *dep_count == 0 { + root_ids.push(id); + } + } + + root_ids.sort_by_key(|id| &self.references[*id]); + + while results.len() < root_ids.len() { + let root_id = root_ids[results.len()]; + let root = mem::take(&mut self.references[root_id]); + results.push(root); + + // Remove this reference as a dependency from any of its dependent references. + if let Ok(dep_ix) = self + .dependencies + .binary_search_by_key(&root_id, |edge| edge.0) + { + let mut first_dep_ix = dep_ix; + let mut last_dep_ix = dep_ix + 1; + while first_dep_ix > 0 && self.dependencies[first_dep_ix - 1].0 == root_id { + first_dep_ix -= 1; + } + while last_dep_ix < self.dependencies.len() + && self.dependencies[last_dep_ix].0 == root_id + { + last_dep_ix += 1; + } + + // If any reference no longer has any dependencies, then then mark it as a root. + // Preserve the references' original order where possible. + for (_, successor_id) in self.dependencies.drain(first_dep_ix..last_dep_ix) { + self.dependency_counts[successor_id] -= 1; + if self.dependency_counts[successor_id] == 0 { + if let Err(ix) = root_ids[results.len()..].binary_search(&successor_id) { + root_ids.insert(results.len() + ix, successor_id); + } + } + } + + root_ids[results.len()..].sort_by_key(|id| &self.references[*id]); + } + } + + // If any references never became roots, then there are reference cycles + // in the set. Return an error containing all of the key paths that are + // directly involved in cycles. + if results.len() < self.references.len() { + let mut cycle_ref_ids = (0..self.references.len()) + .filter(|id| !root_ids.contains(id)) + .collect::>(); + + // Iteratively remove any references that have no dependencies, + // so that the error will only indicate which key paths are directly + // involved in the cycles. + let mut done = false; + while !done { + done = true; + cycle_ref_ids.retain(|id| { + if self.dependencies.iter().any(|dep| dep.0 == *id) { + true + } else { + done = false; + self.dependencies.retain(|dep| dep.1 != *id); + false + } + }); + } + + let mut cycle_key_paths = Vec::new(); + for id in cycle_ref_ids { + let reference = &self.references[id]; + cycle_key_paths.push(reference.target.clone()); + cycle_key_paths.push(reference.source.clone()); + } + cycle_key_paths.sort_unstable(); + return Err(cycle_key_paths); + } + + Ok(results) + } + + fn populate_dependencies(&mut self, new_id: usize, new_reference: &KeyPathReference) { + self.dependency_counts.push(0); + + // If an existing reference's source path starts with the new reference's + // target path, then insert this new reference before that existing reference. + for id in Self::reference_ids_for_key_path( + &new_reference.target.0, + &self.references, + &self.reference_ids_by_source, + KeyPathReference::source, + KeyPath::starts_with, + ) { + Self::add_dependency( + (new_id, id), + &mut self.dependencies, + &mut self.dependency_counts, + ); + } + + // If an existing reference's target path starts with the new reference's + // source path, then insert this new reference after that existing reference. + for id in Self::reference_ids_for_key_path( + &new_reference.source.0, + &self.references, + &self.reference_ids_by_target, + KeyPathReference::target, + KeyPath::starts_with, + ) { + Self::add_dependency( + (id, new_id), + &mut self.dependencies, + &mut self.dependency_counts, + ); + } + + // If an existing reference's source path is a prefix of the new reference's + // target path, then insert this new reference before that existing reference. + for prefix in new_reference.target.prefixes() { + for id in Self::reference_ids_for_key_path( + prefix, + &self.references, + &self.reference_ids_by_source, + KeyPathReference::source, + PartialEq::eq, + ) { + Self::add_dependency( + (new_id, id), + &mut self.dependencies, + &mut self.dependency_counts, + ); + } + } + + // If an existing reference's target path is a prefix of the new reference's + // source path, then insert this new reference after that existing reference. + for prefix in new_reference.source.prefixes() { + for id in Self::reference_ids_for_key_path( + prefix, + &self.references, + &self.reference_ids_by_target, + KeyPathReference::target, + PartialEq::eq, + ) { + Self::add_dependency( + (id, new_id), + &mut self.dependencies, + &mut self.dependency_counts, + ); + } + } + } + + // Find all existing references that satisfy a given predicate with respect + // to a given key path. Use a sorted array of reference ids in order to avoid + // performing unnecessary comparisons. + fn reference_ids_for_key_path<'a>( + key_path: &[Key], + references: &[KeyPathReference], + sorted_reference_ids: &'a [usize], + reference_attribute: impl Fn(&KeyPathReference) -> &KeyPath, + predicate: impl Fn(&KeyPath, &[Key]) -> bool, + ) -> impl Iterator + 'a { + let ix = sorted_reference_ids + .binary_search_by_key(&key_path, |id| &reference_attribute(&references[*id]).0) + .unwrap_or_else(|i| i); + + let mut start_ix = ix; + while start_ix > 0 { + let reference_id = sorted_reference_ids[start_ix - 1]; + let reference = &references[reference_id]; + if !predicate(&reference_attribute(reference), key_path) { + break; + } + start_ix -= 1; + } + + let mut end_ix = ix; + while end_ix < sorted_reference_ids.len() { + let reference_id = sorted_reference_ids[end_ix]; + let reference = &references[reference_id]; + if !predicate(&reference_attribute(reference), key_path) { + break; + } + end_ix += 1; + } + + sorted_reference_ids[start_ix..end_ix].iter().copied() + } + + fn add_dependency( + (predecessor, successor): (usize, usize), + dependencies: &mut Vec<(usize, usize)>, + dependency_counts: &mut Vec, + ) { + let dependency = (predecessor, successor); + if let Err(i) = dependencies.binary_search(&dependency) { + dependencies.insert(i, dependency); + } + dependency_counts[successor] += 1; + } +} + +impl KeyPathReference { + fn source(&self) -> &KeyPath { + &self.source + } + + fn target(&self) -> &KeyPath { + &self.target + } +} + +impl KeyPath { + fn new(string: &str) -> Self { + Self( + string + .split(".") + .map(|key| Key::Object(key.to_string())) + .collect(), + ) + } + + fn starts_with(&self, other: &[Key]) -> bool { + self.0.starts_with(&other) + } + + fn prefixes(&self) -> impl Iterator { + (1..self.0.len()).map(move |end_ix| &self.0[0..end_ix]) + } +} + +impl PartialEq<[Key]> for KeyPath { + fn eq(&self, other: &[Key]) -> bool { + self.0.eq(other) + } +} + +impl fmt::Debug for KeyPathReference { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "KeyPathReference {{ {} <- {} }}", + self.target, self.source + ) + } +} + +impl fmt::Display for KeyPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for (i, key) in self.0.iter().enumerate() { + match key { + Key::Array(index) => write!(f, "[{}]", index)?, + Key::Object(key) => { + if i > 0 { + ".".fmt(f)?; + } + key.fmt(f)?; + } + } + } + Ok(()) + } +} + impl Default for HighlightMap { fn default() -> Self { Self(Arc::new([])) @@ -307,70 +614,36 @@ fn deep_merge_json(base: &mut Map, extension: Map) } } -#[derive(Debug, Clone, PartialEq, Eq)] -enum Key { - Array(usize), - Object(String), -} - -#[derive(Debug, PartialEq, Eq)] -struct KeyPathReference { - source_path: Vec, - target_path: Vec, -} - -impl PartialOrd for KeyPathReference { - fn partial_cmp(&self, other: &Self) -> Option { - if self.target_path.starts_with(&other.source_path) - || other.source_path.starts_with(&self.target_path) - { - Some(Ordering::Less) - } else if other.target_path.starts_with(&self.source_path) - || self.source_path.starts_with(&other.target_path) - { - Some(Ordering::Greater) - } else { - None - } - } -} - -fn find_references(value: &Value, key_path: &mut Vec, references: &mut Vec) { +fn find_references(value: &Value, key_path: &mut KeyPath, references: &mut KeyPathReferenceSet) { match value { Value::Array(vec) => { for (ix, value) in vec.iter().enumerate() { - key_path.push(Key::Array(ix)); + key_path.0.push(Key::Array(ix)); find_references(value, key_path, references); - key_path.pop(); + key_path.0.pop(); } } Value::Object(map) => { for (key, value) in map.iter() { if key == "extends" { if let Some(source_path) = value.as_str().and_then(|s| s.strip_prefix("$")) { - references.push(KeyPathReference { - source_path: source_path - .split(".") - .map(|key| Key::Object(key.to_string())) - .collect(), - target_path: key_path.clone(), + references.insert(KeyPathReference { + source: KeyPath::new(source_path), + target: key_path.clone(), }); } } else { - key_path.push(Key::Object(key.to_string())); + key_path.0.push(Key::Object(key.to_string())); find_references(value, key_path, references); - key_path.pop(); + key_path.0.pop(); } } } Value::String(string) => { if let Some(source_path) = string.strip_prefix("$") { - references.push(KeyPathReference { - source_path: source_path - .split(".") - .map(|key| Key::Object(key.to_string())) - .collect(), - target_path: key_path.clone(), + references.insert(KeyPathReference { + source: KeyPath::new(source_path), + target: key_path.clone(), }); } } @@ -378,18 +651,8 @@ fn find_references(value: &Value, key_path: &mut Vec, references: &mut Vec< } } -fn sort_references(references: &mut Vec) { - for i in 0..references.len() { - for j in (i + 1)..references.len() { - if let Some(Ordering::Greater) = &references[i].partial_cmp(&references[j]) { - references.swap(i, j) - } - } - } -} - -fn value_at<'a>(object: &'a mut Map, key_path: &Vec) -> Option<&'a mut Value> { - let mut key_path = key_path.iter(); +fn value_at<'a>(object: &'a mut Map, key_path: &KeyPath) -> Option<&'a mut Value> { + let mut key_path = key_path.0.iter(); if let Some(Key::Object(first_key)) = key_path.next() { let mut cur_value = object.get_mut(first_key); for key in key_path { @@ -430,9 +693,10 @@ where #[cfg(test)] mod tests { - use crate::assets::Assets; + use rand::{prelude::StdRng, Rng}; use super::*; + use crate::assets::Assets; #[test] fn test_bundled_themes() { @@ -565,6 +829,117 @@ mod tests { assert_eq!(theme.highlight_name(map.get(2)), Some("variable.builtin")); } + #[test] + fn test_key_path_reference_set_simple() { + let input_references = build_refs(&[ + ("r", "a"), + ("a.b.c", "d"), + ("d.e", "f"), + ("t.u", "v"), + ("v.w", "x"), + ("v.y", "x"), + ("d.h", "i"), + ("v.z", "x"), + ("f.g", "d.h"), + ]); + let expected_references = build_refs(&[ + ("d.h", "i"), + ("f.g", "d.h"), + ("d.e", "f"), + ("a.b.c", "d"), + ("r", "a"), + ("v.w", "x"), + ("v.y", "x"), + ("v.z", "x"), + ("t.u", "v"), + ]) + .collect::>(); + + let mut reference_set = KeyPathReferenceSet::default(); + for reference in input_references { + reference_set.insert(reference); + } + assert_eq!(reference_set.top_sort().unwrap(), expected_references); + } + + #[test] + fn test_key_path_reference_set_with_cycles() { + let input_references = build_refs(&[ + ("x", "a.b"), + ("y", "x.c"), + ("a.b.c", "d.e"), + ("d.e.f", "g.h"), + ("g.h.i", "a"), + ]); + + let mut reference_set = KeyPathReferenceSet::default(); + for reference in input_references { + reference_set.insert(reference); + } + + assert_eq!( + reference_set.top_sort().unwrap_err(), + &[ + KeyPath::new("a"), + KeyPath::new("a.b.c"), + KeyPath::new("d.e"), + KeyPath::new("d.e.f"), + KeyPath::new("g.h"), + KeyPath::new("g.h.i"), + ] + ); + } + + #[gpui::test(iterations = 20)] + async fn test_key_path_reference_set_random(mut rng: StdRng) { + let examples: &[&[_]] = &[ + &[ + ("n.d.h", "i"), + ("f.g", "n.d.h"), + ("n.d.e", "f"), + ("a.b.c", "n.d"), + ("r", "a"), + ("v.w", "x"), + ("v.y", "x"), + ("v.z", "x"), + ("t.u", "v"), + ], + &[ + ("w.x.y.z", "t.u.z"), + ("x", "w.x"), + ("a.b.c1", "x.b1.c"), + ("a.b.c2", "x.b2.c"), + ], + &[ + ("x.y", "m.n.n.o.q"), + ("x.y.z", "m.n.n.o.p"), + ("u.v.w", "x.y.z"), + ("a.b.c.d", "u.v"), + ("a.b.c.d.e", "u.v"), + ("a.b.c.d.f", "u.v"), + ("a.b.c.d.g", "u.v"), + ], + ]; + + for example in examples { + let expected_references = build_refs(example).collect::>(); + let mut input_references = expected_references.clone(); + input_references.sort_by_key(|_| rng.gen_range(0..1000)); + let mut reference_set = KeyPathReferenceSet::default(); + for reference in input_references { + reference_set.insert(reference); + } + assert_eq!(reference_set.top_sort().unwrap(), expected_references); + } + } + + fn build_refs<'a>(rows: &'a [(&str, &str)]) -> impl Iterator + 'a { + rows.iter().map(|(target, source)| KeyPathReference { + target: KeyPath::new(target), + source: KeyPath::new(source), + }) + } + struct TestAssets(&'static [(&'static str, &'static str)]); impl AssetSource for TestAssets { From 4191e3adeef046e939a346503306750c653b04c9 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 18 Aug 2021 13:54:20 -0700 Subject: [PATCH 4/4] Simplify logic for guaranteeing stable sort order of references --- zed/src/theme.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/zed/src/theme.rs b/zed/src/theme.rs index fbabb08579..39412804d8 100644 --- a/zed/src/theme.rs +++ b/zed/src/theme.rs @@ -328,9 +328,11 @@ impl KeyPathReferenceSet { } } - root_ids.sort_by_key(|id| &self.references[*id]); - while results.len() < root_ids.len() { + // Just to guarantee a stable result when the inputs are randomized, + // sort references lexicographically in absence of any dependency relationship. + root_ids[results.len()..].sort_by_key(|id| &self.references[*id]); + let root_id = root_ids[results.len()]; let root = mem::take(&mut self.references[root_id]); results.push(root); @@ -356,13 +358,9 @@ impl KeyPathReferenceSet { for (_, successor_id) in self.dependencies.drain(first_dep_ix..last_dep_ix) { self.dependency_counts[successor_id] -= 1; if self.dependency_counts[successor_id] == 0 { - if let Err(ix) = root_ids[results.len()..].binary_search(&successor_id) { - root_ids.insert(results.len() + ix, successor_id); - } + root_ids.push(successor_id); } } - - root_ids[results.len()..].sort_by_key(|id| &self.references[*id]); } } @@ -899,6 +897,9 @@ mod tests { ("n.d.e", "f"), ("a.b.c", "n.d"), ("r", "a"), + ("q.q.q", "r.s"), + ("r.t", "q"), + ("x.x", "r.r"), ("v.w", "x"), ("v.y", "x"), ("v.z", "x"),