From 621a0fca9717cfee66accc705fe78eed0b8da1ec Mon Sep 17 00:00:00 2001
From: Lukhnos Liu <lukhnos@lukhnos.org>
Date: Sat, 20 Jan 2024 10:52:59 -0800
Subject: [PATCH] Use values, not references, when passing callbacks

Before this commit, the callbacks that are passed to KeyHandler have
been by-reference. The problem is that, if a callback is owned by a
candidate object, and the key handler invokes the StateCallback with a
committing state, the commit will cause the candidate panel to reset,
destroying the candidate object. At that point the callback reference is
no longer valid, and further use of the callback (still within the key
handler) is therefore no longer sound.

This becomes problematic when a StateCallback is used multiple times and
when the commiting state is involved. To make the code safer, we
therefore update the public functions to take callbacks as values, not
via references.

This doesn't change the private functions of KeyHandler as those are
already referring to the callback object via the value received from the
public functions.
---
 src/KeyHandler.cpp | 15 +++++++--------
 src/KeyHandler.h   | 14 ++++++--------
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/src/KeyHandler.cpp b/src/KeyHandler.cpp
index a95da54..752eb47 100644
--- a/src/KeyHandler.cpp
+++ b/src/KeyHandler.cpp
@@ -102,8 +102,8 @@ KeyHandler::KeyHandler(
 }
 
 bool KeyHandler::handle(Key key, McBopomofo::InputState* state,
-                        const StateCallback& stateCallback,
-                        const ErrorCallback& errorCallback) {
+                        StateCallback stateCallback,
+                        ErrorCallback errorCallback) {
   // From Key's definition, if shiftPressed is true, it can't be a simple key
   // that can be represented by ASCII.
   char simpleAscii = (key.ctrlPressed || key.shiftPressed) ? '\0' : key.ascii;
@@ -431,7 +431,7 @@ bool KeyHandler::handle(Key key, McBopomofo::InputState* state,
 
 void KeyHandler::candidateSelected(
     const InputStates::ChoosingCandidate::Candidate& candidate,
-    const StateCallback& stateCallback) {
+    StateCallback stateCallback) {
   if (inputMode_ == InputMode::PlainBopomofo) {
     reset();
     std::unique_ptr<InputStates::Committing> committingState =
@@ -446,12 +446,12 @@ void KeyHandler::candidateSelected(
 
 void KeyHandler::dictionaryServiceSelected(std::string phrase, size_t index,
                                            InputState* currentState,
-                                           const StateCallback& stateCallback) {
+                                           StateCallback stateCallback) {
   dictionaryServices_->lookup(std::move(phrase), index, currentState,
                               stateCallback);
 }
 
-void KeyHandler::candidatePanelCancelled(const StateCallback& stateCallback) {
+void KeyHandler::candidatePanelCancelled(StateCallback stateCallback) {
   if (inputMode_ == InputMode::PlainBopomofo) {
     reset();
     std::unique_ptr<InputStates::EmptyIgnoringPrevious>
@@ -465,9 +465,8 @@ void KeyHandler::candidatePanelCancelled(const StateCallback& stateCallback) {
 }
 
 bool KeyHandler::handleCandidateKeyForTraditionalBopomofoIfRequired(
-    Key key,
-    const SelectCurrentCandidateCallback& SelectCurrentCandidateCallback,
-    const StateCallback& stateCallback, const ErrorCallback& errorCallback) {
+    Key key, SelectCurrentCandidateCallback SelectCurrentCandidateCallback,
+    StateCallback stateCallback, ErrorCallback errorCallback) {
   if (inputMode_ != McBopomofo::InputMode::PlainBopomofo) {
     return false;
   }
diff --git a/src/KeyHandler.h b/src/KeyHandler.h
index 682fb27..4b43613 100644
--- a/src/KeyHandler.h
+++ b/src/KeyHandler.h
@@ -68,26 +68,24 @@ class KeyHandler {
   // the key should be absorbed, signaling that the key is accepted and handled,
   // or false if the event should be let pass through.
   bool handle(Key key, McBopomofo::InputState* state,
-              const StateCallback& stateCallback,
-              const ErrorCallback& errorCallback);
+              StateCallback stateCallback, ErrorCallback errorCallback);
 
   // Candidate selected. Can assume the context is in a candidate state.
   void candidateSelected(
       const InputStates::ChoosingCandidate::Candidate& candidate,
-      const StateCallback& stateCallback);
+      StateCallback stateCallback);
 
   void dictionaryServiceSelected(std::string phrase, size_t index,
                                  InputState* currentState,
-                                 const StateCallback& stateCallback);
+                                 StateCallback stateCallback);
 
   // Candidate panel canceled. Can assume the context is in a candidate state.
-  void candidatePanelCancelled(const StateCallback& stateCallback);
+  void candidatePanelCancelled(StateCallback stateCallback);
 
   // Workaround for the Traditional Bopomofo mode.
   bool handleCandidateKeyForTraditionalBopomofoIfRequired(
-      Key key,
-      const SelectCurrentCandidateCallback& SelectCurrentCandidateCallback,
-      const StateCallback& stateCallback, const ErrorCallback& errorCallback);
+      Key key, SelectCurrentCandidateCallback SelectCurrentCandidateCallback,
+      StateCallback stateCallback, ErrorCallback errorCallback);
 
   void reset();