From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Sat, 30 Apr 2022 12:21:34 +0000 (+0100)
Subject: Reorganise Z setting into two phases
X-Git-Tag: otter-1.1.0~395
X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=ce4e8a399d604b9de5e4963446bc4c79a67babd4;p=otter.git

Reorganise Z setting into two phases

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---

diff --git a/daemon/api.rs b/daemon/api.rs
index 22c83fa0..9150bf4b 100644
--- a/daemon/api.rs
+++ b/daemon/api.rs
@@ -457,7 +457,7 @@ api_route!{
   fn op(&self, a: ApiPieceOpArgs) -> PieceUpdate {
     let ApiPieceOpArgs { gs,piece, .. } = a;
     let gpc = gs.pieces.byid_mut(piece)?;
-    op_do_set_z(gpc, gs.gen, &self.z)?;
+    api_op_set_z(gpc, gs.gen, &self.z)?.implement(gpc);
     let update = PieceUpdateOp::SetZLevel(());
     (WhatResponseToClientOp::Predictable,
      update, vec![]).into()
@@ -575,14 +575,8 @@ api_route!{
       let gpc = a.gs.pieces.byid_mut(a.piece)?;
       if gpc.held != None { throw!(Ia::PieceHeld) }
       if ! (self.z > gpc.zlevel.z) { throw!(Ia::BadPieceStateForOperation); }
-      op_do_set_z(gpc, a.gs.gen, &self.z)?;
-      a.ipc.show(y).op_multigrab(a, y, self.n, &self.z).map_err(|e| match e {
-        // TODO: The error handling is wrong, here.  If op_multigrab
-        // returns a deferred thunk, the APOE::PartiallyProcessed will
-        // not be applked.
-        APOE::Inapplicable(ia) => APOE::PartiallyProcessed(ia, vec![]),
-        other => other,
-      })?
+      let new_z = api_op_set_z(gpc, a.gs.gen, &self.z)?;
+      a.ipc.show(y).op_multigrab(a, y, self.n, new_z)?
     }
   }
 }
diff --git a/src/currency.rs b/src/currency.rs
index 34d4365e..8d6784a9 100644
--- a/src/currency.rs
+++ b/src/currency.rs
@@ -124,9 +124,8 @@ impl PieceTrait for Banknote {
 
   #[throws(ApiPieceOpError)]
   fn op_multigrab(&self, _: ApiPieceOpArgs, show: ShowUnocculted,
-                  take: MultigrabQty, new_z: &ZCoord) -> OpOutcomeThunk {
+                  take: MultigrabQty, new_z: ShouldSetZLevel) -> OpOutcomeThunk {
     let currency = self.currency.clone();
-    let new_z = new_z.clone();
     OpOutcomeThunk::Reborrow(Box::new(
       move |ig: &mut InstanceGuard, player: PlayerId, tpiece: PieceId|
   {
diff --git a/src/fastsplit.rs b/src/fastsplit.rs
index 96281349..bab779ac 100644
--- a/src/fastsplit.rs
+++ b/src/fastsplit.rs
@@ -83,7 +83,7 @@ impl InstanceGuard<'_> {
   #[throws(ApiPieceOpError)]
   pub fn fastsplit_split<I>(
     &mut self, player: PlayerId,
-    tpiece: PieceId, show: ShowUnocculted, new_z: ZCoord,
+    tpiece: PieceId, show: ShowUnocculted, new_z: ShouldSetZLevel,
     implementation: I
   ) -> UpdateFromOpComplex
   where I: FnOnce(&IOccults, &GameOccults, &GPlayer,
@@ -113,7 +113,7 @@ impl InstanceGuard<'_> {
       pos:           tgpc.pos,
       face:          tgpc.face,
       held:          None,
-      zlevel:        ZLevel { z: new_z, zg: ig.gs.gen },
+      zlevel:        tgpc.zlevel.clone() /* placeholder, overwritten below */,
       pinned:        tgpc.pinned,
       occult:        default(),
       angle:         tgpc.angle,
@@ -126,6 +126,7 @@ impl InstanceGuard<'_> {
       rotateable:    tgpc.rotateable,
       fastsplit:     tgpc.fastsplit,
     };
+    new_z.implement(&mut ngpc);
 
     let tipc_p = (||{
       let p = tipc.p.show(show);
diff --git a/src/gamestate.rs b/src/gamestate.rs
index 96e848f9..04d64aa7 100644
--- a/src/gamestate.rs
+++ b/src/gamestate.rs
@@ -274,7 +274,7 @@ pub trait PieceTrait: PieceBaseTrait + Downcast + Send + Debug + 'static {
   //
   // So the multigrab operation specifies a ZCoord.
   fn op_multigrab(&self, _a: ApiPieceOpArgs, _show: ShowUnocculted,
-                  _qty: MultigrabQty, _new_z: &ZCoord)
+                  _qty: MultigrabQty, _new_z: ShouldSetZLevel)
                   -> Result<OpOutcomeThunk,ApiPieceOpError>  {
     Err(Ia::BadPieceStateForOperation)?
   }
@@ -806,12 +806,35 @@ mod test {
 
 // ---------- 2-phase Z level setting ----------
 
+/// Drop this only on errors; don't just forget it
+#[derive(Debug, Clone)]
+pub struct ShouldSetZLevel(ZLevel);
+
+/// Prepare to set the z level of gpc to z
+///
+/// If this is a good idea, returns a ShouldSetZLevel.
+/// That should be [`implement`](ShouldSetZLevel::implement)ed
+/// along with the the rest of the operation, when committing.
+///
+/// This allows us to do all of an operation's checks and preparation,
+/// including Z level setting, first, and then only set the Z level
+/// infallibly at the end.
 #[throws(ApiPieceOpError)]
-pub fn op_do_set_z(gpc: &mut GPiece, gen: Generation, z: &ZCoord) {
-    if gpc.occult.is_active() {
-      if z >= &gpc.zlevel.z { throw!(Ia::Occultation) }
-    }
-    gpc.zlevel = ZLevel { z: z.clone(), zg: gen };
+pub fn api_op_set_z(gpc: &mut GPiece, gen: Generation, z: &ZCoord)
+                    -> ShouldSetZLevel
+{
+  if gpc.occult.is_active() {
+    if z >= &gpc.zlevel.z { throw!(Ia::Occultation) }
+  }
+  ShouldSetZLevel(ZLevel { z: z.clone(), zg: gen })
+}
+
+impl ShouldSetZLevel {
+  pub fn implement(self, gpc: &mut GPiece) {
+    gpc.zlevel = self.0;
+  }
+
+  pub fn inspect(&self) -> &ZLevel { &self.0 }
 }
 
 // ---------- log expiry ----------