Review fix: CHPROP says what it left out; the working set is read once

Interactive CHPROP threw away properties.set's message, so an edit to an
object it had just moved onto a locked layer was left out without a word.
It now shows the message.

properties.set looked each object up in REFEDIT's working set, a list,
twice; it now builds the set once and checks the layer directly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Slopapalooza
2026-11-01 14:10:07 -05:00
co-authored by Claude Opus 5.5
parent e82dede24a
commit 8b91646795
3 changed files with 27 additions and 6 deletions
+6 -4
View File
@@ -209,19 +209,21 @@ impl LeftOut {
/// handle that names no object is an error, before anything changes.
fn split(s: &Session, hs: Vec<Handle>) -> Result<(Vec<Handle>, LeftOut)> {
let d = s.doc()?;
// The working set, once: `RefEdit::is_working` searches a list.
let working: Option<std::collections::HashSet<Handle>> = d.refedit.as_ref().map(|r| r.items.iter().map(|i| i.copy).collect());
let mut seen = std::collections::HashSet::new();
let mut out = LeftOut::default();
let mut free = Vec::new();
for h in hs {
if d.entity(h).is_none() {
let Some(e) = d.entity(h) else {
return Err(cadcraft_doc::DocError::NoEntity(h).into());
}
};
if !seen.insert(h) {
continue;
}
if super::blocks::refedit::locked(d, h) {
if working.as_ref().is_some_and(|w| !w.contains(&h)) {
out.outside += 1;
} else if super::curves::is_locked(s, h) {
} else if d.layer(&e.common.layer).is_some_and(|l| l.locked) {
out.locked += 1;
} else {
free.push(h);
+7 -2
View File
@@ -89,13 +89,18 @@ impl ChpropM {
if vals.all(|v| v == first) { first } else { "varies".into() }
}
/// Apply `props` to the selection through `properties.set`.
/// Apply `props` to the selection through `properties.set`, saying what it left out: objects
/// this command moved onto a locked layer, say.
fn apply(&self, s: &mut Session, props: Value) -> Result<()> {
let mut p = json!({ "handles": self.handles.iter().map(|h| h.hex()).collect::<Vec<_>>() });
if let (Some(p), Value::Object(props)) = (p.as_object_mut(), props) {
p.extend(props);
}
super::run_set(s, &p).map(|_| ())
let r = super::run_set(s, &p)?;
if let Some(m) = r.get("message").and_then(Value::as_str) {
s.echo(m.to_string());
}
Ok(())
}
/// Take an answer to `ask`; `false` when it was refused (the prompt comes again).
@@ -58,6 +58,20 @@ fn objects_on_locked_layers_are_left_out() {
assert_eq!(r["message"], "1 object was on a locked layer.");
}
/// CHPROP says so when it leaves an edit out: an object it moved onto a locked layer keeps its
/// color.
#[test]
fn chprop_says_when_it_leaves_an_object_it_moved_onto_a_locked_layer_alone() {
let (mut s, _) = locked_drawing();
let d = line(&mut s, 60.0);
s.cmdline("(setq d (entlast))").unwrap();
s.log.clear();
s.cmdline("(command \"_.CHPROP\" d \"\" \"_LA\" \"L1\" \"_C\" 3 \"\")").unwrap();
let e = s.doc().unwrap().entity(d).unwrap().common.clone();
assert_eq!((e.layer.as_str(), e.color), ("L1", Color::ByLayer));
assert!(s.log.iter().any(|l| l == "1 object was on a locked layer. Nothing was changed."), "{:?}", s.log);
}
/// A handle that names no object is refused before anything changes.
#[test]
fn a_missing_object_is_refused_before_any_change() {