feat(core): address review comments

* Renamed various functions and types
* Clarified return values
* Updated doc comments

core/include/keyman/keyman_core_api.h

@@ -253,8 +253,8 @@ km_core_context_items_from_utf8(char const *text,
 Convert a context item array into a UTF-16 encoded string placing it into
 the supplied buffer of specified size, and return the number of code units
 actually used in the conversion. If null is passed as the buffer the
-number codeunits required is returned. This will strip markers from the
-context during the conversion.
+number of codeunits required is returned. Any markers in the context will
+not be included in the output buffer.
 ##### Return status:
 - `KM_CORE_STATUS_OK`: On success.
 - `KM_CORE_STATUS_INVALID_ARGUMENT`: If non-optional parameters are null.
@@ -285,8 +285,8 @@ km_core_context_items_to_utf16(km_core_context_item const *item,
 Convert a context item array into a UTF-8 encoded string placing it into
 the supplied buffer of specified size, and return the number of code units
 actually used in the conversion. If null is passed as the buffer the
-number codeunits required is returned. This will strip markers from the
-context during the conversion.
+number of codeunits required is returned. Any markers in the context will
+not be included in the output buffer.
 ##### Return status:
 - `KM_CORE_STATUS_OK`: On success.
 - `KM_CORE_STATUS_INVALID_ARGUMENT`: If non-optional parameters are null.
@@ -317,8 +317,8 @@ km_core_context_items_to_utf8(km_core_context_item const *item,
 Convert a context item array into a UTF-32 encoded string placing it into
 the supplied buffer of specified size, and return the number of codepoints
 actually used in the conversion. If null is passed as the buffer the
-number codepoints required is returned. This will strip markers from the
-context during the conversion.
+number of codepoints required is returned. Any markers in the context will
+not be included in the output buffer.
 ##### Return status:
 - `KM_CORE_STATUS_OK`: On success.
 - `KM_CORE_STATUS_INVALID_ARGUMENT`: If non-optional parameters are null.
@@ -571,9 +571,12 @@ removed in the future.
 ```c
 */
 
+typedef enum { KM_CORE_FALSE = 0, KM_CORE_TRUE = 1 } km_core_bool;
+typedef enum { KM_CORE_CAPS_UNCHANGED = -1, KM_CORE_CAPS_OFF = 0, KM_CORE_CAPS_ON = 1 } km_core_caps_state;
+
 typedef struct {
-  // number of codepoints (not codeunits!) to delete from app context, 0+
-  int delete_back;
+  // number of codepoints (not codeunits!) to delete from app context.
+  unsigned int delete_back_codepoints;
 
   // null-term string of characters to insert into document
   km_core_usv* output;
@@ -582,13 +585,13 @@ typedef struct {
   km_core_option_item* persist_options;
 
   // issue a beep, 0 = no, 1 = yes
-  int do_alert;
+  km_core_bool do_alert;
 
   // emit the input keystroke to the application, unmodified? 0 = no, 1 = yes
-  int emit_keystroke;
+  km_core_bool emit_keystroke;
 
   // -1=unchanged, 0=off, 1=on
-  int new_caps_lock_state;
+  km_core_caps_state new_caps_lock_state;
 } km_core_actions;
 
 /*
@@ -596,7 +599,7 @@ typedef struct {
 ### `km_core_state_get_actions`
 ##### Description:
 Returns a pointer to an actions object which details all the actions
-that the Platform layer must take after a keystroke. The `delete_back`
+that the Platform layer must take after a keystroke. The `delete_back_codepoints`
 action must be performed before the `output` action, but the other
 actions may be performed in any order.
 ##### Return:
@@ -632,52 +635,73 @@ km_core_actions_dispose(
 
 /*
 ```
-### `km_core_state_context_validate`
+### `km_core_state_context_set_if_needed`
 ##### Description:
-Determines if the input context has changed, and if so, resets
-the internal cached context (including markers), to the new
-context string.
+Sets the internal cached context for the state object, to the passed-in
+application context string, if it differs from the codepoints in the
+cached context. For the purposes of comparison, (1) cached markers are
+ignored, (2) if the cached context is shorter than the application
+context, it is considered identical, but (3) if the cached context is
+longer, then it is considered different.
 
-This and `km_core_state_context_invalidate` will replace existing Core context
-APIs.
+If a difference is found, then the cached context will be set to the
+application context, and thus any cached markers will be cleared.
+
+`km_core_state_context_set_if_needed` and `km_core_state_context_clear`
+will replace most uses of the existing Core context APIs.
 
 ##### Parameters:
 - __state__: An opaque pointer to a state object.
-- __application_context__: A pointer to an null-terminated `km_core_cp` string
-    representing the current context from the application.
+- __application_context__: A pointer to an null-terminated `km_core_cp`
+    string representing the current context from the application.
 ##### Return status:
-- `KM_CORE_STATUS_OK`: On success.
-- `KM_CORE_STATUS_INVALID_ARGUMENT`: If non-optional parameters are null.
+- `KM_CORE_CONTEXT_STATUS_UNCHANGED`: Cached context change was not needed
+- `KM_CORE_CONTEXT_STATUS_UPDATED`: Cached context was set to application
+  context
+- `KM_CORE_CONTEXT_STATUS_CLEARED`: Application context was invalid, perhaps
+  had unpaired surrogates, and so cached context was cleared instead
+- `KM_CORE_CONTEXT_STATUS_ERROR`: Internal error
+- `KM_CORE_CONTEXT_STATUS_INVALID_ARGUMENT`: One or more parameters was null
 
 ```c
 */
+
+typedef enum {
+  KM_CORE_CONTEXT_STATUS_UNCHANGED = 0,  // Cached context change was not needed
+  KM_CORE_CONTEXT_STATUS_UPDATED = 1,    // Cached context was set to application context
+  KM_CORE_CONTEXT_STATUS_CLEARED = 2,    // Application context was invalid, context was cleared
+  KM_CORE_CONTEXT_STATUS_ERROR = 3,      // Internal error
+  KM_CORE_CONTEXT_STATUS_INVALID_ARGUMENT = 4, // Invalid arguments
+} km_core_context_status;
+
 KMN_API
-km_core_status
-km_core_state_context_validate(
+km_core_context_status
+km_core_state_context_set_if_needed(
   km_core_state *state,
   km_core_cp const *application_context
 );
 
 /*
 ```
-### `km_core_state_context_invalidate`
+### `km_core_state_context_clear`
 ##### Description:
-Flushes the internal cached context for the state.
+Clears the internal cached context for the state. This is the same as
+`km_core_context_clear(km_core_state_context(&state))`.
 
-This and `km_core_state_context_validate` will replace existing Core context
-APIs.
+`km_core_state_context_set_if_needed` and `km_core_state_context_clear`
+will replace most uses of the existing Core context APIs.
 
 ##### Parameters:
 - __state__: An opaque pointer to a state object.
 ##### Return status:
 - `KM_CORE_STATUS_OK`: On success.
-- `KM_CORE_STATUS_INVALID_ARGUMENT`: If non-optional parameters are null.
+- `KM_CORE_STATUS_INVALID_ARGUMENT`: If any parameters are null.
 
 ```c
 */
 KMN_API
 km_core_status
-km_core_state_context_invalidate(
+km_core_state_context_clear(
   km_core_state *state
 );
 

core/src/action.cpp

@@ -16,7 +16,7 @@
 #include "state.hpp"
 #include "option.hpp"
 
-km_core_actions * km::kbp::action_items_to_actions(
+km_core_actions * km::kbp::action_item_list_to_actions_object(
   km_core_action_item const *action_items
 ) {
   assert(action_items != nullptr);
@@ -31,30 +31,30 @@ km_core_actions * km::kbp::action_items_to_actions(
   // Set actions defaults
   std::vector<km_core_context_item> output;
   std::vector<km_core_option_item> options;
-  actions->delete_back = 0;
+  actions->delete_back_codepoints = 0;
   actions->output = nullptr;
   actions->persist_options = nullptr;
-  actions->do_alert = false;
-  actions->emit_keystroke = false;
-  actions->new_caps_lock_state = -1;
+  actions->do_alert = KM_CORE_FALSE;
+  actions->emit_keystroke = KM_CORE_FALSE;
+  actions->new_caps_lock_state = KM_CORE_CAPS_UNCHANGED;
 
   for (; action_items->type != KM_CORE_IT_END; ++action_items) {
     assert(action_items->type < KM_CORE_IT_MAX_TYPE_ID);
 
     switch(action_items->type) {
       case KM_CORE_IT_ALERT:
-        actions->do_alert = true;
+        actions->do_alert = KM_CORE_TRUE;
         break;
       case KM_CORE_IT_BACK:
         switch(action_items->backspace.expected_type) {
           case KM_CORE_BT_UNKNOWN:
             // this is equivalent to emit_keystroke, because the only time we
             // are allowed to do an unknown bksp is when a bksp is passed in
-            actions->emit_keystroke = true;
+            actions->emit_keystroke = KM_CORE_TRUE;
             break;
           case KM_CORE_BT_CHAR:
             if(output.empty()) {
-              actions->delete_back++;
+              actions->delete_back_codepoints++;
             } else {
               auto last_context_item = output.back();
               output.pop_back();
@@ -77,13 +77,13 @@ km_core_actions * km::kbp::action_items_to_actions(
         }
         break;
       case KM_CORE_IT_CAPSLOCK:
-        actions->new_caps_lock_state = action_items->capsLock;
+        actions->new_caps_lock_state = action_items->capsLock ? KM_CORE_CAPS_ON : KM_CORE_CAPS_OFF;
         break;
       case KM_CORE_IT_CHAR:
         output.push_back({KM_CORE_CT_CHAR,{0},{action_items->character}});
         break;
       case KM_CORE_IT_EMIT_KEYSTROKE:
-        actions->emit_keystroke = true;
+        actions->emit_keystroke = KM_CORE_TRUE;
         break;
       case KM_CORE_IT_INVALIDATE_CONTEXT:
         // no-op

core/src/action.hpp

@@ -13,7 +13,7 @@
 namespace km {
 namespace kbp
 {
-  km_core_actions* action_items_to_actions(
+  km_core_actions* action_item_list_to_actions_object(
     km_core_action_item const *action_items
   );
 } // namespace kbp

core/src/km_core_action_api.cpp

@@ -33,7 +33,7 @@ km_core_actions* km_core_state_get_actions(
     return nullptr;
   }
 
-  actions = action_items_to_actions(action_items);
+  actions = action_item_list_to_actions_object(action_items);
   return actions;
 }
 

core/src/km_core_state_api.cpp

@@ -260,7 +260,6 @@ void km_core_state_imx_deregister_callback(km_core_state *state)
   state->imx_deregister_callback();
 }
 
-
 bool is_context_valid(km_core_cp const * context, km_core_cp const * cached_context) {
   km_core_cp const* context_p = context;
   while(*context_p) {
@@ -291,64 +290,62 @@ bool is_context_valid(km_core_cp const * context, km_core_cp const * cached_cont
   // context, so if we match the whole cached context, we can safely return true
   return true;
 }
-km_core_status km_core_state_context_validate(
+
+km_core_context_status km_core_state_context_set_if_needed(
   km_core_state *state,
   km_core_cp const *application_context
 ) {
   assert(state != nullptr);
   assert(application_context != nullptr);
   if(state == nullptr || application_context == nullptr) {
-    return KM_CORE_STATUS_INVALID_ARGUMENT;
+    return KM_CORE_CONTEXT_STATUS_INVALID_ARGUMENT;
   }
 
   size_t buf_size;
-  km_core_status status;
   km_core_context_item* context_items = nullptr;
 
   auto context = km_core_state_context(state);
-  if((status = km_core_context_get(context, &context_items)) != KM_CORE_STATUS_OK) {
-    return status;
+  if(km_core_context_get(context, &context_items) != KM_CORE_STATUS_OK) {
+    return KM_CORE_CONTEXT_STATUS_ERROR;
   }
 
-  if((status = km_core_context_items_to_utf16(context_items, nullptr, &buf_size)) != KM_CORE_STATUS_OK) {
+  if(km_core_context_items_to_utf16(context_items, nullptr, &buf_size)) {
     km_core_context_items_dispose(context_items);
-    return status;
+    return KM_CORE_CONTEXT_STATUS_ERROR;
   }
 
-  km_core_cp* cached_context = new km_core_cp[buf_size];
+  std::unique_ptr<km_core_cp[]> cached_context(new km_core_cp[buf_size]);
 
-  status = km_core_context_items_to_utf16(context_items, cached_context, &buf_size);
+  km_core_status status = km_core_context_items_to_utf16(context_items, cached_context.get(), &buf_size);
   km_core_context_items_dispose(context_items);
 
   if(status != KM_CORE_STATUS_OK) {
-    delete[] cached_context;
-    return status;
+    return KM_CORE_CONTEXT_STATUS_ERROR;
   }
 
-  bool is_valid = is_context_valid(application_context, cached_context);
-
-  delete[] cached_context;
+  bool is_valid = is_context_valid(application_context, cached_context.get());
 
   if(is_valid) {
     // We keep the context as is
-    return KM_CORE_STATUS_OK;
+    return KM_CORE_CONTEXT_STATUS_UNCHANGED;
   }
 
+  km_core_context_item* new_context_items = nullptr;
+
   // We replace the cached context with the current application context
-  status = km_core_context_items_from_utf16(application_context, &context_items);
-  if (status == KM_CORE_STATUS_OK) {
-    km_core_context_set(context, context_items);
-    km_core_context_items_dispose(context_items);
-  } else {
-    // TODO: DebugLog as this is a fail case
+  status = km_core_context_items_from_utf16(application_context, &new_context_items);
+  if (status != KM_CORE_STATUS_OK) {
     km_core_context_clear(context);
+    return KM_CORE_CONTEXT_STATUS_CLEARED;
   }
 
-  return KM_CORE_STATUS_OK;
+  km_core_context_set(context, new_context_items);
+  km_core_context_items_dispose(new_context_items);
+  return KM_CORE_CONTEXT_STATUS_UPDATED;
 }
 
 
-km_core_status km_core_state_context_invalidate(
+km_core_status km_core_state_context_clear(
   km_core_state *state
 ) {
   assert(state != nullptr);

core/tests/unit/kmnkbd/action_api.cpp

@@ -38,9 +38,9 @@ void test_two_backspaces() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 1);
+  assert(actions->delete_back_codepoints == 1);
   assert(std::u32string(actions->output) == U"");
   assert(actions->persist_options == nullptr);
   assert(actions->do_alert == false);
@@ -65,9 +65,9 @@ void test_marker_text_interleaved() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 0);
+  assert(actions->delete_back_codepoints == 0);
   assert(std::u32string(actions->output) == U"ABD");
   assert(actions->persist_options == nullptr);
   assert(actions->do_alert == false);
@@ -85,14 +85,14 @@ void test_alert() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 0);
+  assert(actions->delete_back_codepoints == 0);
   assert(std::u32string(actions->output) == U"");
   assert(actions->persist_options == nullptr);
-  assert(actions->do_alert == true);
-  assert(actions->emit_keystroke == false);
-  assert(actions->new_caps_lock_state == -1);
+  assert(actions->do_alert == KM_CORE_TRUE);
+  assert(actions->emit_keystroke == KM_CORE_FALSE);
+  assert(actions->new_caps_lock_state == KM_CORE_CAPS_UNCHANGED);
 
   try_status(km_core_actions_dispose(actions));
 }
@@ -105,14 +105,14 @@ void test_emit_keystroke() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 0);
+  assert(actions->delete_back_codepoints == 0);
   assert(std::u32string(actions->output) == U"");
   assert(actions->persist_options == nullptr);
-  assert(actions->do_alert == false);
-  assert(actions->emit_keystroke == true);
-  assert(actions->new_caps_lock_state == -1);
+  assert(actions->do_alert == KM_CORE_FALSE);
+  assert(actions->emit_keystroke == KM_CORE_TRUE);
+  assert(actions->new_caps_lock_state == KM_CORE_CAPS_UNCHANGED);
 
   try_status(km_core_actions_dispose(actions));
 }
@@ -126,14 +126,14 @@ void test_invalidate_context() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 0);
+  assert(actions->delete_back_codepoints == 0);
   assert(std::u32string(actions->output) == U"");
   assert(actions->persist_options == nullptr);
-  assert(actions->do_alert == false);
-  assert(actions->emit_keystroke == false);
-  assert(actions->new_caps_lock_state == -1);
+  assert(actions->do_alert == KM_CORE_FALSE);
+  assert(actions->emit_keystroke == KM_CORE_FALSE);
+  assert(actions->new_caps_lock_state == KM_CORE_CAPS_UNCHANGED);
 
   try_status(km_core_actions_dispose(actions));
 }
@@ -152,9 +152,9 @@ void test_persist_opt() {
     end_action_item()
   };
 
-  km_core_actions *actions = km::kbp::action_items_to_actions(action_items);
+  km_core_actions *actions = km::kbp::action_item_list_to_actions_object(action_items);
 
-  assert(actions->delete_back == 0);
+  assert(actions->delete_back_codepoints == 0);
   assert(std::u32string(actions->output) == U"");
   assert(actions->persist_options != nullptr);
   assert(std::u16string(actions->persist_options[0].key) == u"key");
@@ -170,9 +170,9 @@ void test_persist_opt() {
   assert(actions->persist_options[1].value == nullptr);
   assert(actions->persist_options[1].scope == KM_CORE_OPT_UNKNOWN);
 
-  assert(actions->do_alert == false);
-  assert(actions->emit_keystroke == false);
-  assert(actions->new_caps_lock_state == -1);
+  assert(actions->do_alert == KM_CORE_FALSE);
+  assert(actions->emit_keystroke == KM_CORE_FALSE);
+  assert(actions->new_caps_lock_state == KM_CORE_CAPS_UNCHANGED);
 
   try_status(km_core_actions_dispose(actions));
 }
@@ -227,40 +227,40 @@ bool is_identical_context(km_core_cp const *cached_context) {
   return result;
 }
 
-void test_context_validate_identical_context() {
+void test_context_set_if_needed_identical_context() {
   km_core_cp const *application_context = u"This is a test";
   km_core_cp const *cached_context =      u"This is a test";
   setup("k_000___null_keyboard.kmx", cached_context);
-  try_status(km_core_state_context_validate(test_state, application_context));
+  assert(km_core_state_context_set_if_needed(test_state, application_context) == KM_CORE_CONTEXT_STATUS_UNCHANGED);
   assert(is_identical_context(cached_context));
   teardown();
 }
 
-void test_context_validate_different_context() {
+void test_context_set_if_needed_different_context() {
   km_core_cp const *application_context = u"This is a    test";
   km_core_cp const *cached_context =      u"This isn't a test";
   setup("k_000___null_keyboard.kmx", cached_context);
-  try_status(km_core_state_context_validate(test_state, application_context));
+  assert(km_core_state_context_set_if_needed(test_state, application_context) == KM_CORE_CONTEXT_STATUS_UPDATED);
   assert(!is_identical_context(cached_context));
   assert(is_identical_context(application_context));
   teardown();
 }
 
-void test_context_validate_app_context_is_longer() {
+void test_context_set_if_needed_app_context_is_longer() {
   km_core_cp const *application_context = u"Longer This is a test";
   km_core_cp const *cached_context =             u"This is a test";
   setup("k_000___null_keyboard.kmx", cached_context);
-  try_status(km_core_state_context_validate(test_state, application_context));
+  assert(km_core_state_context_set_if_needed(test_state, application_context) == KM_CORE_CONTEXT_STATUS_UNCHANGED);
   // Should be true -- longer, but what exists is identical to cached
   assert(is_identical_context(cached_context));
   teardown();
 }
 
-void test_context_validate_app_context_is_shorter() {
+void test_context_set_if_needed_app_context_is_shorter() {
   km_core_cp const *application_context =      u"is a test";
   km_core_cp const *cached_context =      u"This is a test";
   setup("k_000___null_keyboard.kmx", cached_context);
-  try_status(km_core_state_context_validate(test_state, application_context));
+  assert(km_core_state_context_set_if_needed(test_state, application_context) == KM_CORE_CONTEXT_STATUS_UPDATED);
   // Should be false -- app ctxt is shorter, so doesn't matter that what we have
   // matches
   assert(!is_identical_context(cached_context));
@@ -268,7 +268,7 @@ void test_context_validate_app_context_is_shorter() {
   teardown();
 }
 
-void test_context_validate_cached_context_has_markers() {
+void test_context_set_if_needed_cached_context_has_markers() {
   km_core_cp const *application_context = u"123";
   km_core_cp const *cached_context =      u"123";
   setup("k_000___null_keyboard.kmx", cached_context);
@@ -286,7 +286,7 @@ void test_context_validate_cached_context_has_markers() {
   };
 
   try_status(km_core_context_set(km_core_state_context(test_state), citems));
-  try_status(km_core_state_context_validate(test_state, application_context));
+  assert(km_core_state_context_set_if_needed(test_state, application_context) == KM_CORE_CONTEXT_STATUS_UNCHANGED);
 
   km_core_context_item* citems_new;
 
@@ -304,18 +304,18 @@ void test_context_validate_cached_context_has_markers() {
   teardown();
 }
 
-void test_context_validate() {
-  test_context_validate_identical_context();
-  test_context_validate_different_context();
-  test_context_validate_app_context_is_longer();
-  test_context_validate_app_context_is_shorter();
-  test_context_validate_cached_context_has_markers();
+void test_context_set_if_needed() {
+  test_context_set_if_needed_identical_context();
+  test_context_set_if_needed_different_context();
+  test_context_set_if_needed_app_context_is_longer();
+  test_context_set_if_needed_app_context_is_shorter();
+  test_context_set_if_needed_cached_context_has_markers();
 }
 
-void test_context_invalidate() {
+void test_context_clear() {
   km_core_cp const *cached_context =      u"This is a test";
   setup("k_000___null_keyboard.kmx", cached_context);
-  try_status(km_core_state_context_invalidate(test_state));
+  try_status(km_core_state_context_clear(test_state));
   assert(!is_identical_context(cached_context));
   assert(is_identical_context(u""));
   teardown();
@@ -364,8 +364,8 @@ int main(int argc, char *argv []) {
   test_invalidate_context();
 
   // context -- todo move to another file
-  test_context_validate();
-  test_context_invalidate();
+  test_context_set_if_needed();
+  test_context_clear();
 }
 
 //-------------------------------------------------------------------------------------