Use correct walk snapshots for UOM observation

UOM needs both the previous walk result and the current walk result
after node override (that is, after user manually chooses a candidate)
to make an observation, but since the nodes in a WalkResult are shared
pointers to the Node instances in the grid, WalkResult does not capture
an immutable snapshot. A new member function is introduced to enable
getting a snapshot that has a real copy of the nodes, so as to capture
the state of the grid before it is further mutated.

Author: lukhnos

src/Engine/UserOverrideModelTest.cpp

@@ -24,6 +24,7 @@
 #include <string>
 
 #include "UserOverrideModel.h"
+#include "gramambular2/reading_grid.h"
 #include "gtest/gtest.h"
 
 namespace McBopomofo {
@@ -119,4 +120,159 @@ TEST(UserOverrideModelTest, LRUBehavior) {
   ASSERT_TRUE(v.empty());
 }
 
+constexpr char kSampleData[] = R"(
+ㄐㄧ 機 -3.02367199
+ㄐㄧ 積 -3.72854036
+ㄐㄧ-ㄧㄡˊ 機油 -6.03662914
+ㄧㄡˊ 由 -3.00970678
+ㄧㄡˊ 油 -3.75900671
+)";
+
+class SimpleLM : public Formosa::Gramambular2::LanguageModel {
+ public:
+  explicit SimpleLM(const char* input, bool readingIsFirstColumn = true) {
+    std::stringstream sstream(input);
+    while (sstream.good()) {
+      std::string line;
+      getline(sstream, line);
+      if (line.empty() || line[0] == '#') {
+        continue;
+      }
+      std::stringstream linestream(line);
+      std::string col0;
+      std::string col1;
+      std::string col2;
+      linestream >> col0;
+      linestream >> col1;
+      linestream >> col2;
+      db_[readingIsFirstColumn ? col0 : col1].emplace_back(
+          readingIsFirstColumn ? col1 : col0, std::stod(col2));
+    }
+  }
+
+  std::vector<Unigram> getUnigrams(const std::string& key) override {
+    const auto f = db_.find(key);
+    return f == db_.end() ? std::vector<Unigram>() : (*f).second;
+  }
+
+  bool hasUnigrams(const std::string& key) override {
+    return db_.find(key) != db_.end();
+  }
+
+ protected:
+  std::map<std::string, std::vector<Unigram>> db_;
+};
+
+TEST(UserOverrideModelTest, WalkResultSnapshotTest) {
+  // See https://github.com/openvanilla/McBopomofo/issues/885.
+  // This is a kind of integration test for the following steps:
+  // 1. Type ㄐㄧ ㄧㄡˊ -> 機由; this assumes P(機）P(由) > P(機油)
+  // 2. Override with 機油 and tell UOM to observe
+  // 3. Reset, type ㄐㄧ ㄧㄡˊ -> 機油 -- UOM should suggest the expected
+  // override
+  // 4. Reset, type ㄐㄧ -> 機
+  // 5. Override with 積 and tell UOM to observe
+  // 6. Reset, type ㄐㄧ ㄧㄡˊ -> 積由 -- this is expected due to UOM suggestion
+  // 7. Override with 機油 and tell UOM to observe
+  // 8. Reset, type ㄐㄧ -> 積 due to UOM suggestion
+  // 9. Continue to type 由 -> walk result should be 積由, and the UOM should
+  //    suggest 機油 as the override candidate.
+
+  std::string sampleData(kSampleData);
+  Formosa::Gramambular2::ReadingGrid grid(
+      std::make_shared<SimpleLM>(sampleData.c_str()));
+  UserOverrideModel uom(kCapacity, kHalflife);
+  Formosa::Gramambular2::ReadingGrid::WalkResult walkBefore;
+  Formosa::Gramambular2::ReadingGrid::WalkResult walkLatest;
+  UserOverrideModel::Suggestion suggestion;
+  std::vector<Formosa::Gramambular2::ReadingGrid::NodePtr>::const_iterator
+      nodeIter;
+  double timestamp = kFakeNow;
+
+  grid.insertReading("ㄐㄧ");
+  grid.insertReading("ㄧㄡˊ");
+  walkBefore = grid.walk();
+  ASSERT_EQ(walkBefore.valuesAsStrings(),
+            (std::vector<std::string>{"機", "由"}));
+
+  grid.overrideCandidate(1, "機油");
+  walkLatest = grid.walk();
+  ASSERT_EQ(walkLatest.valuesAsStrings(), (std::vector<std::string>{"機油"}));
+
+  nodeIter = walkLatest.findNodeAt(1, /*outCursorPastNode=*/nullptr);
+  ASSERT_NE(nodeIter, walkLatest.nodes.cend());
+  uom.observe(walkBefore, walkLatest, 1, timestamp);
+  timestamp += 1.0;
+
+  grid.clear();
+  grid.insertReading("ㄐㄧ");
+  grid.insertReading("ㄧㄡˊ");
+  walkLatest = grid.walk();
+  suggestion = uom.suggest(walkLatest, 1, timestamp);
+  timestamp += 1.0;
+  ASSERT_EQ(suggestion.candidate, "機油");
+
+  grid.clear();
+  grid.insertReading("ㄐㄧ");
+  walkBefore = grid.walk();
+  ASSERT_EQ(walkBefore.valuesAsStrings(), (std::vector<std::string>{"機"}));
+
+  grid.overrideCandidate(0, "積");
+  walkLatest = grid.walk();
+  ASSERT_EQ(walkLatest.valuesAsStrings(), (std::vector<std::string>{"積"}));
+
+  nodeIter = walkLatest.findNodeAt(0, /*outCursorPastNode=*/nullptr);
+  ASSERT_NE(nodeIter, walkLatest.nodes.cend());
+  uom.observe(walkBefore, walkLatest, 0, timestamp);
+  timestamp += 1.0;
+
+  grid.clear();
+  grid.insertReading("ㄐㄧ");
+  walkLatest = grid.walk();
+  suggestion = uom.suggest(walkLatest, 0, timestamp);
+  timestamp += 1.0;
+  ASSERT_EQ(suggestion.candidate, "積");
+
+  grid.overrideCandidate(0, "積");
+  grid.insertReading("ㄧㄡˊ");
+
+  // THIS MUST BE A COPY OF NODES, otherwise the observation below will not
+  // capture the correct state of the grid.
+  // See https://github.com/openvanilla/McBopomofo/issues/885.
+  //
+  // This is wrong:
+  //   walkBefore = grid.walk();
+  //
+  // The following is correct:
+  walkBefore = grid.walk().copyWithFixedNodes();
+
+  ASSERT_EQ(walkBefore.valuesAsStrings(),
+            (std::vector<std::string>{"積", "由"}));
+
+  grid.overrideCandidate(1, "機油");
+  walkLatest = grid.walk();
+  ASSERT_EQ(walkLatest.valuesAsStrings(), (std::vector<std::string>{"機油"}));
+
+  nodeIter = walkLatest.findNodeAt(1, /*outCursorPastNode=*/nullptr);
+  ASSERT_NE(nodeIter, walkLatest.nodes.cend());
+  uom.observe(walkBefore, walkLatest, 1, timestamp);
+  timestamp += 1.0;
+
+  grid.clear();
+  grid.insertReading("ㄐㄧ");
+  walkLatest = grid.walk();
+  suggestion = uom.suggest(walkLatest, 0, timestamp);
+  timestamp += 1.0;
+  ASSERT_EQ(suggestion.candidate, "積");
+  grid.overrideCandidate(0, "積");
+  grid.insertReading("ㄧㄡˊ");
+  walkLatest = grid.walk();
+  ASSERT_EQ(walkLatest.valuesAsStrings(),
+            (std::vector<std::string>{"積", "由"}));
+
+  suggestion = uom.suggest(walkLatest, 1, timestamp);
+  timestamp += 1.0;
+  ASSERT_EQ(suggestion.candidate, "機油");
+}
+
 }  // namespace McBopomofo

src/Engine/gramambular2/reading_grid.h

@@ -111,6 +111,15 @@ class ReadingGrid {
           unigramIter_(unigrams_.begin()),
           overrideType_(OverrideType::kNone) {}
 
+    // An explicit copy ctor is needed since unigramIter_ must come from
+    // this.unigram_, not copied.
+    Node(const Node& o)
+        : reading_(o.reading_),
+          spanningLength_(o.spanningLength_),
+          unigrams_(o.unigrams_),
+          unigramIter_(unigrams_.begin()),
+          overrideType_(o.overrideType_) {}
+
     [[nodiscard]] const std::string& reading() const { return reading_; }
 
     [[nodiscard]] size_t spanningLength() const { return spanningLength_; }
@@ -178,6 +187,24 @@ class ReadingGrid {
 
     std::vector<std::string> valuesAsStrings() const;
     std::vector<std::string> readingsAsStrings() const;
+
+    // Makes a copy with the nodes also being copies instead of refernces to
+    // those in the current grid.
+    //
+    // For performance reasons, nodes is a vector of shared ptrs to those
+    // nodes in the referenced grid. If the intent is to have the walk capture
+    // the current state of the grid before the grid mutates, the default
+    // behavior will not be enough. Instead, use this to make sure that the
+    // nodes are correctly copied.
+    WalkResult copyWithFixedNodes() const {
+      std::vector<NodePtr> copiedNodes;
+      for (const auto& n : nodes) {
+        copiedNodes.push_back(std::make_shared<Node>(*n));
+      }
+
+      return WalkResult{copiedNodes, totalReadings, vertices, edges,
+                        elapsedMicroseconds};
+    }
   };
 
   WalkResult walk();

src/Engine/gramambular2/reading_grid_test.cpp

@@ -817,4 +817,18 @@ TEST(ReadingGridTest, FindInSpan2) {
   ASSERT_EQ(result->get()->value(), "高熱");
 }
 
+TEST(ReadingGridTest, CopyWithFixedNodesMustNotContainDanglingUnigramIter) {
+  Formosa::Gramambular2::ReadingGrid::WalkResult walkBefore;
+  {
+    Formosa::Gramambular2::ReadingGrid grid(std::make_shared<MockLM>());
+    grid.insertReading("a");
+    walkBefore = grid.walk().copyWithFixedNodes();
+  }
+
+  // Accessing value() dereferences the walkBefore.unigramIter_, which must not
+  // come from the nodes in the grid that is already gone.
+  std::string val = walkBefore.nodes[0]->value();
+  EXPECT_EQ(val, "a");
+}
+
 }  // namespace Formosa::Gramambular2

src/KeyHandler.cpp

@@ -1777,14 +1777,21 @@ void KeyHandler::pinNode(
     size_t originalCursor,
     const InputStates::ChoosingCandidate::Candidate& candidate,
     bool useMoveCursorAfterSelectionSetting) {
+  // Since WalkResult makes references to the current nodes, we must make a
+  // copy of the walk that *has a copy* of the current nodes to capture the
+  // current state. ReadingGrid::overrideCandidate() changes the state, and
+  // so having a simple copy of latestWalk_ (auto prevWalk = latestWalk_;)
+  // is NOT enough.
+  Formosa::Gramambular2::ReadingGrid::WalkResult prevWalk =
+      latestWalk_.copyWithFixedNodes();
+
   size_t actualCursor = actualCandidateCursorIndex();
   Formosa::Gramambular2::ReadingGrid::Candidate gridCandidate(
       candidate.reading, candidate.value, "");
   if (!grid_.overrideCandidate(actualCursor, gridCandidate)) {
     return;
   }
 
-  Formosa::Gramambular2::ReadingGrid::WalkResult prevWalk = latestWalk_;
   walk();
 
   // Update the user override model if warranted.