Review: reject revisions after Compare, one or all

review.reject {index | all} drops revisions from the list without applying
them, so the open deck stays as it is: a rejected inserted slide is never
added, a rejected deleted slide stays, a rejected changed slide keeps its
current content. Accept All skips what was rejected. Rejections are kept on
the compare session (keyed by the open slide id and the revised slide) and
end with End Review; like Compare and End Review, rejecting is not an undo
step, since it leaves the document untouched.

Revisions pane: Reject next to each Accept, and Reject All.
This commit is contained in:
Marco Arnulfo
2026-10-15 22:54:04 +02:00
parent 9859da3925
commit d28b4e0f54
6 changed files with 135 additions and 11 deletions
+32 -4
View File
@@ -59,6 +59,15 @@ pub fn specs() -> Vec<CommandSpec> {
cmd!(noundo "review.compare", "Compare", ["Review", "Compare"], None, "{path} | {name, data: base64} — compare with a revised copy → {name, changes}", has_doc, compare),
cmd!(query "review.revisions", "Revisions", [], None, "{} → {name, changes: [{kind: inserted|deleted|changed, slide, revised, details}]}", has_doc, revisions),
cmd!("review.accept", "Accept Revision", ["Review", "Compare"], None, "{index} | {all: true}", comparing, accept),
cmd!(
noundo "review.reject",
"Reject Revision",
["Review", "Compare"],
None,
"{index} | {all: true} → {rejected} (drop revisions without applying them: the open deck stays as it is; not an undo step)",
comparing,
reject
),
cmd!(noundo "review.endCompare", "End Review", ["Review", "Compare"], None, "{}", comparing, end_compare),
cmd!(noundo "show.fromStart", "Play from Start", ["Slide Show", "Start Slide Show"], Some("Cmd+Shift+Enter"), "{}", has_slide, |s, _| start(s, 0)),
cmd!(noundo "show.fromCurrent", "Play from Current Slide", ["Slide Show", "Start Slide Show"], Some("Cmd+Enter"), "{}", has_slide, |s, _| {
@@ -648,7 +657,7 @@ fn revised_bytes(p: &Value) -> Result<(String, Vec<u8>)> {
fn revisions_json(st: &crate::DocState) -> Result<Value> {
let c = st.compare.as_ref().ok_or_else(|| bad("review.revisions", "not comparing"))?;
let name = std::path::Path::new(&c.name).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| c.name.clone());
Ok(json!({"name": name, "changes": crate::compare::diff(&st.doc, &c.revised, c.by_position), "byPosition": c.by_position}))
Ok(json!({"name": name, "changes": crate::compare::pending(&st.doc, c), "byPosition": c.by_position}))
}
fn compare(s: &mut Session, p: &Value) -> Result<Value> {
@@ -656,7 +665,7 @@ fn compare(s: &mut Session, p: &Value) -> Result<Value> {
let revised = super::file::open_presentation(&name, &bytes)?;
let st = s.doc_mut()?;
let by_position = crate::compare::unrelated(&st.doc, &revised);
st.compare = Some(crate::compare::Compare { name, revised: Arc::new(revised), by_position });
st.compare = Some(crate::compare::Compare { name, revised: Arc::new(revised), by_position, rejected: Default::default() });
s.ui_requests.push(UiRequest::Pane { id: "revisions".into() });
revisions_json(s.doc()?)
}
@@ -666,7 +675,8 @@ fn revisions(s: &mut Session, _: &Value) -> Result<Value> {
}
fn accept(s: &mut Session, p: &Value) -> Result<Value> {
let (rev, by_pos) = s.doc()?.compare.as_ref().map(|c| (c.revised.clone(), c.by_position)).ok_or_else(|| bad("review.accept", "not comparing"))?;
let cmp = s.doc()?.compare.clone().ok_or_else(|| bad("review.accept", "not comparing"))?;
let rev = cmp.revised.clone();
let all = bool_param(p, "all").unwrap_or(false);
let index = usize_param(p, "index");
if !all && index.is_none() {
@@ -677,7 +687,7 @@ fn accept(s: &mut Session, p: &Value) -> Result<Value> {
// Each accept shifts indices, so the list is recomputed; bounded by the slide counts.
let limit = doc.slides.len().saturating_add(rev.slides.len()).saturating_add(1);
for _ in 0..limit {
let changes = crate::compare::diff(doc, &rev, by_pos);
let changes = crate::compare::pending(doc, &cmp);
let c = if all {
changes.into_iter().find(|c| !(c.kind == crate::compare::ChangeKind::Deleted && doc.slides.len() <= 1))
} else {
@@ -700,6 +710,24 @@ fn accept(s: &mut Session, p: &Value) -> Result<Value> {
})
}
fn reject(s: &mut Session, p: &Value) -> Result<Value> {
let all = bool_param(p, "all").unwrap_or(false);
let index = usize_param(p, "index");
if !all && index.is_none() {
return Err(bad("review.reject", "give `index` or `all`"));
}
let st = s.doc_mut()?;
let doc = st.doc.clone();
let c = st.compare.as_mut().ok_or_else(|| bad("review.reject", "not comparing"))?;
let pending = crate::compare::pending(&doc, c);
let picked: Vec<_> = if all { pending } else { pending.into_iter().nth(index.unwrap_or(0)).into_iter().collect() };
if picked.is_empty() {
return Err(bad("review.reject", "no such revision"));
}
c.rejected.extend(picked.iter().map(|ch| crate::compare::ChangeKey::of(&doc, ch)));
Ok(json!({"rejected": picked.len()}))
}
fn end_compare(s: &mut Session, _: &Value) -> Result<Value> {
s.doc_mut()?.compare = None;
ok()
+74 -2
View File
@@ -1,5 +1,5 @@
//! Review › Compare: the differences between the open presentation and a revised copy of it, slide
//! by slide, and accepting a revised slide into the open one.
//! by slide, and accepting a revised slide into the open one or rejecting it.
//!
//! Slides match by id (a revised copy of the same deck keeps them); when the two decks share no
//! slide id at all, by position. Shapes match by id, then by name.
@@ -18,6 +18,34 @@ pub struct Compare {
/// Slides match by position (the decks shared no slide id when the compare started); fixed
/// for the session so accepting slides doesn't switch the matching half way.
pub by_position: bool,
/// Revisions rejected: no longer listed and never accepted, the open deck left as it is.
pub rejected: HashSet<ChangeKey>,
}
/// What a revision is about, stable while slides move: the open slide (for changed and deleted)
/// and the revised slide (for changed and inserted).
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
pub struct ChangeKey {
kind: ChangeKind,
open: Option<SlideId>,
revised: Option<usize>,
}
impl ChangeKey {
pub fn of(doc: &Presentation, change: &Change) -> ChangeKey {
let open = match change.kind {
ChangeKind::Inserted => None,
ChangeKind::Changed | ChangeKind::Deleted => doc.slides.get(change.slide).map(|s| s.id),
};
ChangeKey { kind: change.kind, open, revised: change.revised }
}
}
/// The revisions still to review: [`diff`] without the rejected ones.
pub fn pending(doc: &Presentation, c: &Compare) -> Vec<Change> {
let mut out = diff(doc, &c.revised, c.by_position);
out.retain(|ch| !c.rejected.contains(&ChangeKey::of(doc, ch)));
out
}
/// Whether `cur` and `rev` share no slide id (then slides match by position).
@@ -26,7 +54,7 @@ pub fn unrelated(cur: &Presentation, rev: &Presentation) -> bool {
!cur.slides.iter().any(|s| ids.contains(&s.id))
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize)]
#[serde(rename_all = "camelCase")]
pub enum ChangeKind {
/// Only in the revised deck.
@@ -434,6 +462,50 @@ mod tests {
assert!(s.execute("review.accept", &json!({"all": true})).is_err());
}
#[test]
fn reject_keeps_the_open_deck_and_accept_all_skips_rejected_revisions() {
let (mut s, rev, n) = decks();
s.execute("review.compare", &json!({"name": "rev.deckcraft", "data": rev})).unwrap();
let before = s.execute("review.revisions", &json!({})).unwrap()["changes"].as_array().unwrap().clone();
let ins = before.iter().position(|c| c["kind"] == "inserted").unwrap();
let doc = s.doc().unwrap().doc.clone();
let undo = s.doc().unwrap().history.undo.len();
assert_eq!(s.execute("review.reject", &json!({"index": ins})).unwrap()["rejected"], 1);
assert!(std::sync::Arc::ptr_eq(&doc, &s.doc().unwrap().doc), "rejecting leaves the open deck as it is");
let left = s.execute("review.revisions", &json!({})).unwrap();
assert_eq!(kinds(&left).len(), before.len() - 1);
assert!(!kinds(&left).contains(&"inserted".into()), "{left}");
// Accept All takes the rest, never the rejected slide.
s.execute("review.accept", &json!({"all": true})).unwrap();
let st = s.doc().unwrap();
assert_eq!(st.doc.slides.len(), n - 1, "the deleted slide goes, the rejected insertion never comes");
assert!(!st.doc.slides.iter().any(|x| super::slide_title(x) == "Brand new"));
assert!(s.execute("review.revisions", &json!({})).unwrap()["changes"].as_array().unwrap().is_empty());
s.execute("edit.undo", &json!({})).unwrap();
assert!(std::sync::Arc::ptr_eq(&doc, &s.doc().unwrap().doc));
assert_eq!(s.doc().unwrap().history.undo.len(), undo, "rejecting is not an undo step");
// Reject All: nothing left to review, the deck as it was; Accept All then has nothing to take.
assert_eq!(s.execute("review.reject", &json!({"all": true})).unwrap()["rejected"], before.len() - 1);
assert!(s.execute("review.revisions", &json!({})).unwrap()["changes"].as_array().unwrap().is_empty());
assert!(s.execute("review.accept", &json!({"all": true})).is_err());
assert!(std::sync::Arc::ptr_eq(&doc, &s.doc().unwrap().doc));
assert!(s.execute("review.reject", &json!({"all": true})).is_err(), "nothing left to reject");
assert!(s.execute("review.reject", &json!({})).is_err());
s.execute("review.endCompare", &json!({})).unwrap();
assert!(s.execute("review.reject", &json!({"index": 0})).is_err());
}
#[test]
fn rejecting_one_revision_keeps_the_others_in_order() {
let (mut s, rev, _) = decks();
s.execute("review.compare", &json!({"name": "rev.deckcraft", "data": rev})).unwrap();
let before = s.execute("review.revisions", &json!({})).unwrap()["changes"].as_array().unwrap().clone();
assert!(s.execute("review.reject", &json!({"index": before.len()})).is_err());
s.execute("review.reject", &json!({"index": 0})).unwrap();
let left = s.execute("review.revisions", &json!({})).unwrap()["changes"].as_array().unwrap().clone();
assert_eq!(left.as_slice(), &before[1..], "the rejected one goes, the others stay in order");
}
#[test]
fn unrelated_decks_match_by_position_and_bad_input_fails() {
let mut s = Session::with_new();