refactor(engine): split execute_unguarded, load_styles and resize_border (Sonar S3776) (#1726)

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Dylan M. Taylor
2026-10-24 04:50:14 -04:00
committed by GitHub
co-authored by Claude Sonnet 5.5
parent c142678bd5
commit 8271f1a26b
3 changed files with 97 additions and 62 deletions
+36 -25
View File
@@ -201,9 +201,9 @@ fn plan(doc: &Document, src: &Document, paragraph: Option<&[String]>, character:
Ok((para, chars))
}
fn load(s: &mut Session, p: &Value) -> Result<Value> {
let src = source(p, ID)?;
let conflict = match p.get("conflicts") {
/// The `conflicts` parameter: how a style that differs under the same name is handled.
fn conflict_param(p: &Value) -> Result<Conflict> {
Ok(match p.get("conflicts") {
None | Some(Value::Null) => Conflict::Redefine,
Some(v) => match v.as_str() {
Some("redefine") => Conflict::Redefine,
@@ -211,7 +211,37 @@ fn load(s: &mut Session, p: &Value) -> Result<Value> {
Some("autoRename") => Conflict::AutoRename,
_ => return Err(bad(ID, "`conflicts` must be redefine, useExisting or autoRename")),
},
})
}
/// Give the loaded style `ours` the export tag the source had for `theirs` (`kind` is `p` or `c`).
fn carry_export_tag(src: &Styles, dst: &mut Styles, kind: char, theirs: &str, ours: &str) {
let tag = format!("{kind}:{ours}");
match src.export_tags.get(&format!("{kind}:{theirs}")) {
Some(t) => dst.export_tags.insert(tag, t.clone()),
None => dst.export_tags.remove(&tag),
};
}
/// A loaded style and one the document keeps can name each other as their base.
fn check_based_on_cycles(styles: &Styles, loaded: &[(bool, String)]) -> Result<()> {
for (is_para, name) in loaded {
let looped = if *is_para {
styles.para(name).and_then(|s| s.based_on.as_deref()).is_some_and(|b| styles.para_based_on_cycles(name, b))
} else {
styles.char_style(name).and_then(|s| s.based_on.as_deref()).is_some_and(|b| styles.char_based_on_cycles(name, b))
};
if looped {
let kind = if *is_para { "paragraph" } else { "character" };
return Err(bad(ID, format!("{kind} style `{name}` would be based on itself; load the style it is based on as well")));
}
}
Ok(())
}
fn load(s: &mut Session, p: &Value) -> Result<Value> {
let src = source(p, ID)?;
let conflict = conflict_param(p)?;
let (paragraph, character) = (names(p, "paragraph")?, names(p, "character")?);
let doc = s.doc()?.doc.clone();
let (mut para, mut chars) = plan(&doc, &src, paragraph.as_deref(), character.as_deref(), conflict)?;
@@ -231,11 +261,7 @@ fn load(s: &mut Session, p: &Value) -> Result<Value> {
p.nested_line_styles.iter_mut().flatten().for_each(|l| chars.follow(&mut l.style));
swatches.extend(char_swatches(&style.chars).chain(para_swatches(&style.para)).cloned());
lists.extend(style.para.list_name.iter().cloned());
let tag = format!("p:{}", style.name);
match src.styles.export_tags.get(&format!("p:{}", theirs.name)) {
Some(t) => styles.export_tags.insert(tag, t.clone()),
None => styles.export_tags.remove(&tag),
};
carry_export_tag(&src.styles, &mut styles, 'p', &theirs.name, &style.name);
loaded.push((true, style.name.clone()));
match styles.para_mut(&style.name) {
Some(m) if action == Action::Replace => {
@@ -256,11 +282,7 @@ fn load(s: &mut Session, p: &Value) -> Result<Value> {
chars.follow(&mut style.name);
style.based_on.iter_mut().for_each(|n| chars.follow(n));
swatches.extend(char_swatches(&style.chars).cloned());
let tag = format!("c:{}", style.name);
match src.styles.export_tags.get(&format!("c:{}", theirs.name)) {
Some(t) => styles.export_tags.insert(tag, t.clone()),
None => styles.export_tags.remove(&tag),
};
carry_export_tag(&src.styles, &mut styles, 'c', &theirs.name, &style.name);
loaded.push((false, style.name.clone()));
match styles.char_style_mut(&style.name) {
Some(m) if action == Action::Replace => {
@@ -275,18 +297,7 @@ fn load(s: &mut Session, p: &Value) -> Result<Value> {
}
}
}
// A loaded style and one the document keeps can name each other as their base.
for (is_para, name) in &loaded {
let looped = if *is_para {
styles.para(name).and_then(|s| s.based_on.as_deref()).is_some_and(|b| styles.para_based_on_cycles(name, b))
} else {
styles.char_style(name).and_then(|s| s.based_on.as_deref()).is_some_and(|b| styles.char_based_on_cycles(name, b))
};
if looped {
let kind = if *is_para { "paragraph" } else { "character" };
return Err(bad(ID, format!("{kind} style `{name}` would be based on itself; load the style it is based on as well")));
}
}
check_based_on_cycles(&styles, &loaded)?;
let swatches = missing_swatches(&doc, &src, swatches);
let lists: Vec<NumberedList> = {
+31 -17
View File
@@ -1273,6 +1273,35 @@ fn resize_track(sizes: &[f64], index: usize, delta: f64, keep: bool) -> Option<V
out.iter().all(|v| v.is_finite()).then_some(out)
}
/// The frame a border drag names must hold the table's story and be editable.
fn check_border_frame(st: &crate::DocState, p: &Value, story: designcraft_doc::StoryId) -> Result<()> {
const ID: &str = "table.resizeBorder";
if let Some(fid) = super::id_param(p, "frame") {
let it = st.doc.item(fid).ok_or_else(|| bad(ID, format!("no frame {}", fid.0)))?;
if it.text_frame().map(|f| f.story) != Some(story) {
return Err(bad(ID, "the table is not in that frame's story"));
}
if it.locked || st.doc.layer(it.layer).is_some_and(|l| l.locked) {
return Err(bad(ID, "the frame or its layer is locked"));
}
}
Ok(())
}
/// The change in track size: `delta`, or `size` measured from the current size `cur`.
fn border_delta(p: &Value, cur: f64) -> Result<f64> {
const ID: &str = "table.resizeBorder";
let delta = match (p.get("delta").and_then(Value::as_f64), p.get("size").and_then(Value::as_f64)) {
(Some(d), _) => d,
(None, Some(sz)) => sz - cur,
(None, None) => return Err(bad(ID, "`delta` or `size` required")),
};
if !delta.is_finite() {
return Err(bad(ID, "`delta`/`size` must be a finite number"));
}
Ok(delta)
}
fn resize_border(s: &mut Session, p: &Value) -> Result<Value> {
const ID: &str = "table.resizeBorder";
let column = match str_param(p, "axis") {
@@ -1284,15 +1313,7 @@ fn resize_border(s: &mut Session, p: &Value) -> Result<Value> {
let keep = p.get("keepTableSize").and_then(Value::as_bool).unwrap_or(false);
let g = target(s, p, ID)?;
let st = s.doc()?;
if let Some(fid) = super::id_param(p, "frame") {
let it = st.doc.item(fid).ok_or_else(|| bad(ID, format!("no frame {}", fid.0)))?;
if it.text_frame().map(|f| f.story) != Some(g.story) {
return Err(bad(ID, "the table is not in that frame's story"));
}
if it.locked || st.doc.layer(it.layer).is_some_and(|l| l.locked) {
return Err(bad(ID, "the frame or its layer is locked"));
}
}
check_border_frame(st, p, g.story)?;
let t = st.doc.story(g.story).and_then(|x| x.tables.get(&g.table)).ok_or_else(|| bad(ID, "no table"))?;
let sizes: Vec<f64> = if column {
t.columns.iter().map(|c| c.width).collect()
@@ -1301,14 +1322,7 @@ fn resize_border(s: &mut Session, p: &Value) -> Result<Value> {
(0..t.nrows()).map(|r| composed_row_height(&cs, t, r).unwrap_or(MIN_TRACK)).collect()
};
let cur = *sizes.get(index).ok_or_else(|| bad(ID, format!("index {index} out of range")))?;
let delta = match (p.get("delta").and_then(Value::as_f64), p.get("size").and_then(Value::as_f64)) {
(Some(d), _) => d,
(None, Some(sz)) => sz - cur,
(None, None) => return Err(bad(ID, "`delta` or `size` required")),
};
if !delta.is_finite() {
return Err(bad(ID, "`delta`/`size` must be a finite number"));
}
let delta = border_delta(p, cur)?;
let new = resize_track(&sizes, index, delta, keep).ok_or_else(|| bad(ID, "invalid table geometry"))?;
let size = new.get(index).copied().unwrap_or(cur);
let scaled = keep && index + 1 == sizes.len();
+30 -20
View File
@@ -542,17 +542,7 @@ impl Session {
// passes that its command's description doesn't name fails the test.
#[cfg(test)]
check_params(id, params)?;
if let Err(e) = (spec.enabled)(self) {
// A command that takes `ids` acts on the objects the call names, so the selection
// doesn't matter: with a text caret, nothing selected, or a selection the command
// can't act on, `{"ids": [5]}` still runs.
let named = if cmd::selection_reason(&e) { cmd::named_targets(spec, params) } else { None };
let Some(ids) = named else { return Err(EngineError::Disabled(id.into(), e)) };
let st = self.doc()?;
if let Some(gone) = ids.iter().find(|i| st.doc.item(**i).is_none()) {
return Err(EngineError::Disabled(id.into(), format!("no object with id {}", gone.0)));
}
}
self.check_enabled(spec, id, params)?;
// Preview is temporary. Save, merge, and every other edit restore the template first.
// Queries stay read-only so the panel can poll fields during a preview. `data.preview`
// itself is the one command that fills the live document.
@@ -598,11 +588,38 @@ impl Session {
st.doc = Arc::new(d);
}
}
// Auto-size frames fit the text the command changed, in the same undo step.
self.autosize_changed(spec, before.as_ref());
self.keep_guide_selection_valid(before.as_ref(), id);
self.record_undo_step(spec, before, nested);
if spec.journal {
self.journal.push((id.to_string(), params.clone()));
}
self.discard_stale_find_scope();
Ok(r)
}
/// Fails with `Disabled` unless the command is enabled or names objects it can act on.
fn check_enabled(&self, spec: &CommandSpec, id: &str, params: &Value) -> Result<()> {
if let Err(e) = (spec.enabled)(self) {
// A command that takes `ids` acts on the objects the call names, so the selection
// doesn't matter: with a text caret, nothing selected, or a selection the command
// can't act on, `{"ids": [5]}` still runs.
let named = if cmd::selection_reason(&e) { cmd::named_targets(spec, params) } else { None };
let Some(ids) = named else { return Err(EngineError::Disabled(id.into(), e)) };
let st = self.doc()?;
if let Some(gone) = ids.iter().find(|i| st.doc.item(**i).is_none()) {
return Err(EngineError::Disabled(id.into(), format!("no object with id {}", gone.0)));
}
}
Ok(())
}
/// Auto-size frames fit the text the command changed, in the same undo step.
fn autosize_changed(&mut self, spec: &CommandSpec, before: Option<&(u64, Arc<Document>, Selection)>) {
if spec.undoable
&& let Some(st) = self.active_mut()
{
let prev = before.as_ref().filter(|(uid, _, _)| *uid == st.uid).map(|(_, d, _)| d.as_ref());
let prev = before.filter(|(uid, _, _)| *uid == st.uid).map(|(_, d, _)| d.as_ref());
if prev.is_none_or(|p| !std::ptr::eq(p, st.doc.as_ref())) {
let todo = autosize::candidates(&st.doc, prev);
if !todo.is_empty() {
@@ -614,13 +631,6 @@ impl Session {
}
}
}
self.keep_guide_selection_valid(before.as_ref(), id);
self.record_undo_step(spec, before, nested);
if spec.journal {
self.journal.push((id.to_string(), params.clone()));
}
self.discard_stale_find_scope();
Ok(r)
}
/// Drops selected guides the command `id` made stale, against the document as it will be