feat(windows): apply suggestions from code review

Co-authored-by: Marc Durdin <marc@durdin.net>

common/core/desktop/include/keyman/keyboardprocessor.h

@@ -131,8 +131,8 @@ typedef struct km_kbp_options     km_kbp_options;
 //
 typedef struct km_kbp_option_item  km_kbp_option_item;
 
-// Callback function used to to access 3rd pary library functions
-// via the Keyman Platform
+// Callback function used to to access Input Method eXtension library functions
+// from Keyman Core
 //
 typedef KMN_API uint8_t (*km_kbp_keyboard_imx_platform)(km_kbp_state*, uint32_t, void*);
 
@@ -801,8 +801,8 @@ void km_kbp_keyboard_key_list_dispose(km_kbp_keyboard_key *key_list);
 
 
 /**
- * Returns the list of libraries and function calls names that will be called by
- * the library. The matching dispose call needs to be called to free the memory.
+ * Returns the list of IMX libraries and function names that are referenced by
+ * the keyboard. The matching dispose call needs to be called to free the memory.
  */
 KMN_API
 km_kbp_status km_kbp_keyboard_get_imx_list(km_kbp_keyboard const *keyboard, km_kbp_keyboard_imx** imx_list);
@@ -814,13 +814,13 @@ KMN_API
 void km_kbp_keyboard_imx_list_dispose(km_kbp_keyboard_imx *imx_list);
 
 /**
- * Register callback from the platform engine.
+ * Register the IMX callback endpoint for the client.
  */
 KMN_API
 void km_kbp_state_imx_register_callback(km_kbp_state *state, km_kbp_keyboard_imx_platform imx_callback, void *callback_object);
 
 /**
- * De-register call callback for platform engine
+ * De-register IMX callback endpoint for the client.
  */
 KMN_API
 void km_kbp_state_imx_deregister_callback(km_kbp_state *state);
@@ -930,8 +930,8 @@ km_kbp_state_context(km_kbp_state *state);
 ```
 ### `kbp_state_get_intermediate_context`
 ##### Description:
-Get access to the state object's keyboard processor's intermediate context.
-That is the context "now" in the keyboardprocessor part way through processing a key stroke.
+Get access to the state object's keyboard processor's intermediate context. This context
+is used during an IMX callback, part way through processing a keystroke.
 ##### Return:
 A pointer to an context item array. Must be disposed of by a call
 to `km_kbp_context_items_dispose`.
@@ -973,17 +973,16 @@ km_kbp_state_action_items(km_kbp_state const *state,
 ```
 ### `km_kbp_state_queue_action_items`
 ##### Description:
-Queue and the action in the current keyboard processor.
-`km_kbp_process_event`.
+Queue actions for the current keyboard processor state; normally
+used in IMX callbacks called during `km_kbp_process_event`.
 ##### Return:
 - `KM_KBP_STATUS_OK`: On success.
 - `KM_KBP_STATUS_INVALID_ARGUMENT`:
-In the event the `state` or `in action` pointer are null.
+In the event the `state` or `action_items` pointer are null.
 ##### Parameters:
-- __state__: A pointer to the opaque `km_kbp_state` object to be queried.
-- __action_items__:
-A pointer to a action item list: The action items to be added to
-the keyboardprocessor queue.
+- __state__:        A pointer to the opaque `km_kbp_state` object to be queried.
+- __action_items__: The action items to be added to the keyboardprocessor 
+                    queue. Must be terminated with a `KM_KBP_IT_END` entry. 
 
 ```c
 */

common/core/desktop/src/km_kbp_keyboard_api.cpp

@@ -99,16 +99,18 @@ void km_kbp_keyboard_key_list_dispose(km_kbp_keyboard_key *key_list)
   delete[] key_list;
 }
 
- km_kbp_status km_kbp_keyboard_get_imx_list(km_kbp_keyboard const *keyboard, km_kbp_keyboard_imx** imx_list)
- {
-   assert(keyboard); assert(imx_list);
-  if (!keyboard || !imx_list)
+km_kbp_status km_kbp_keyboard_get_imx_list(
+  km_kbp_keyboard const *keyboard,
+  km_kbp_keyboard_imx** imx_list
+) {
+  assert(keyboard); assert(imx_list);
+  if (!keyboard || !imx_list) {
     return KM_KBP_STATUS_INVALID_ARGUMENT;
-
+  }
+  
   *imx_list = keyboard->get_imx_list();
   return KM_KBP_STATUS_OK;
-
- }
+}
 
 void km_kbp_keyboard_imx_list_dispose(km_kbp_keyboard_imx *imx_list)
 {

common/core/desktop/src/km_kbp_state_api.cpp

@@ -69,11 +69,13 @@ km_kbp_context *km_kbp_state_context(km_kbp_state *state)
   return static_cast<km_kbp_context *>(&state->context());
 }
 
-km_kbp_status
-kbp_state_get_intermediate_context(km_kbp_state *state, km_kbp_context_item ** context_items){
+km_kbp_status kbp_state_get_intermediate_context(
+  km_kbp_state *state, 
+  km_kbp_context_item ** context_items
+) {
   assert(state);
   assert(context_items);
-  if (!state|| !context_items){
+  if (!state || !context_items) {
     return KM_KBP_STATUS_INVALID_ARGUMENT;
   }
   auto & processor = state->processor();
@@ -97,27 +99,30 @@ km_kbp_action_item const * km_kbp_state_action_items(km_kbp_state const *state,
   return state->actions().data();
 }
 
-km_kbp_status
-km_kbp_state_queue_action_items(km_kbp_state *state,
-                         km_kbp_action_item const *action_items){
+km_kbp_status km_kbp_state_queue_action_items(
+  km_kbp_state *state,
+  km_kbp_action_item const *action_items
+) {
   assert(state);
   assert(action_items);
-  if (!state|| !action_items)  return KM_KBP_STATUS_INVALID_ARGUMENT;
+  if (!state|| !action_items) {
+    return KM_KBP_STATUS_INVALID_ARGUMENT;
+  }
 
   auto & processor = state->processor();
 
-
-  for (; action_items->type != KM_KBP_IT_END; ++action_items)
-  {
-    if (action_items->type >= KM_KBP_IT_MAX_TYPE_ID)
+  for (; action_items->type != KM_KBP_IT_END; ++action_items) {
+    if (action_items->type >= KM_KBP_IT_MAX_TYPE_ID) {
       return KM_KBP_STATUS_INVALID_ARGUMENT;
+    }
 
-    if (!processor.queue_action(action_items))
+    if (!processor.queue_action(action_items)) {
       return KM_KBP_STATUS_KEY_ERROR;
+    }
   }
 
   return KM_KBP_STATUS_OK;
-  }
+}
 namespace {
   char const * action_item_name_lut[] = {
     "",
@@ -234,18 +239,23 @@ km_kbp_status km_kbp_state_to_json(km_kbp_state const *state,
 
 }
 
-void km_kbp_state_imx_register_callback(km_kbp_state *state, km_kbp_keyboard_imx_platform imx_callback, void *callback_object)
-{
+void km_kbp_state_imx_register_callback(
+  km_kbp_state *state,
+  km_kbp_keyboard_imx_platform imx_callback,
+  void *callback_object
+) {
   assert(state);
-  if (!state)
+  if (!state) {
     return;
+  }
   state->imx_register_callback(imx_callback, callback_object);
 }
 
 void km_kbp_state_imx_deregister_callback(km_kbp_state *state)
 {
   assert(state);
-  if (!state)
+  if (!state) {
     return;
+  }
   state->imx_deregister_callback();
 }

common/core/desktop/src/kmx/kmx_processor.cpp

@@ -106,10 +106,10 @@ kmx_processor::update_option(
   return option(scope, key, value);
 }
 
-bool
-kmx_processor::queue_action(km_kbp_action_item const * action_item
+bool kmx_processor::queue_action(
+  km_kbp_action_item const * action_item
 ) {
-   DebugLog("Action type is [%d].\n", action_item->type);
+  DebugLog("Action type is [%d].\n", action_item->type);
   switch (action_item->type) {
   case KM_KBP_IT_END:
     // error should not queue empty item
@@ -174,7 +174,7 @@ kmx_processor::process_event(
   // via the queue_action method.
   bool has_internal_actions = ((vk == VK_SPACE) && (!_kmx.GetActions()->IsQueueEmpty()));
 
-  if (!has_internal_actions){
+  if (!has_internal_actions) {
     // Construct a context buffer from the items
     std::u16string ctxt;
     auto cp = state->context();
@@ -205,7 +205,7 @@ kmx_processor::process_event(
       // We need to output the default keystroke
       state->actions().push_emit_keystroke();
     }
-  } else{
+  } else {
     state->actions().clear();
   }
 
@@ -299,8 +299,8 @@ km_kbp_attr const & kmx_processor::attributes() const {
 km_kbp_context_item * kmx_processor::get_intermediate_context() {
   KMX_WCHAR *buf = _kmx.GetContext()->BufMax(MAXCONTEXT);
   km_kbp_context_item *citems = nullptr;
-  if (!ContextItemsFromAppContext(buf, &citems)){
-      citems = new km_kbp_context_item(KM_KBP_CONTEXT_ITEM_END);
+  if (!ContextItemsFromAppContext(buf, &citems)) {
+    citems = new km_kbp_context_item(KM_KBP_CONTEXT_ITEM_END);
   }
   return citems;
 }

common/core/desktop/src/processor.hpp

@@ -67,33 +67,32 @@ namespace kbp
      * @param  bool  return true if action item list is successfully processed
      */
     virtual bool
-    queue_action(km_kbp_action_item const* action_item
-    ) = 0;
+    queue_action(km_kbp_action_item const* action_item) = 0;
 
-/**
- * Returns the keyboardprocessor context as an array of
- * km_kbp_context_items. Caller is responsible for freeing
- * the memory
- * @return km_kbp_context_item*
- */
+  /**
+   * Returns the keyboardprocessor context as an array of
+   * km_kbp_context_items. Caller is responsible for freeing
+   * the memory
+   * @return km_kbp_context_item*
+   */
     virtual km_kbp_context_item *
     get_intermediate_context() = 0;
 
-  /**
-   * Returns the list of keys that belong to the keyboard rules. The matching dispose
-   * call needs to be called to free the memory.
-   *
-   * @return km_kbp_keyboard_key*
-   */
+   /**
+    * Returns the list of keys that belong to the keyboard rules. The matching dispose
+    * call needs to be called to free the memory.
+    *
+    * @return km_kbp_keyboard_key*
+    */
     virtual km_kbp_keyboard_key *
     get_key_list() const = 0;
 
 
-   /** Get the imx list of external libraries and functions
-      * this keyboard calls.
-    *
-    * @return km_kbp_keyboard_imx*
-    */
+    /** Get the imx list of external libraries and functions
+     * this keyboard calls.
+     *
+     * @return km_kbp_keyboard_imx*
+     */
     virtual km_kbp_keyboard_imx *
     get_imx_list() const = 0;
 

common/core/desktop/src/state.cpp

@@ -50,22 +50,25 @@ state::state(km::kbp::abstract_processor & ap, km_kbp_option_item const *env)
   _imx_object = nullptr;
 }
 
-void state::imx_register_callback(km_kbp_keyboard_imx_platform imx_callback_fp, void *callback_object){
+void state::imx_register_callback(
+  km_kbp_keyboard_imx_platform imx_callback_fp, 
+  void *callback_object
+) {
   assert(imx_callback_fp);
-  if(!imx_callback_fp){
+  if(!imx_callback_fp) {
     return;
   }
   _imx_callback = imx_callback_fp;
   _imx_object = callback_object;
 }
 
-void state::imx_deregister_callback(){
+void state::imx_deregister_callback() {
   _imx_callback = nullptr;
   _imx_object = nullptr;
 }
 
-void state::imx_callback(uint32_t store_no){
-  if (_imx_callback==nullptr){
+void state::imx_callback(uint32_t store_no) {
+  if (_imx_callback==nullptr) {
     return;
   }
   _imx_callback(static_cast<km_kbp_state *>(this), store_no, _imx_object);

windows/src/engine/keyman32/appint/appint.cpp

@@ -297,11 +297,9 @@ ContextItemToAppContext(km_kbp_context_item *contextItems, PWSTR outBuf, DWORD l
     AppContext context;
     context.Set(buf);
     context.Get(outBuf, len);
-    delete[] buf;
-    return TRUE;
   } else {
     wcscpy_s(outBuf, wcslen(buf) + 1, buf);
-    delete[] buf;
-    return TRUE;
   }
+  delete[] buf;
+  return TRUE;
 }

windows/src/engine/keyman32/calldll.cpp

@@ -357,8 +357,9 @@ extern "C" uint8_t IM_CallBackCore(km_kbp_state *km_state, uint32_t UniqueStoreN
     return FALSE;
   }
   LPINTKEYBOARDINFO lpkbi = (LPINTKEYBOARDINFO)(callbackObject);
-  if (!lpkbi->lpCoreKeyboard)
-    return 0; // False
+  if (!lpkbi->lpCoreKeyboard) {
+    return FALSE;
+  }
   // Iterate through hooks to find the third party library function to call
   BOOL found = FALSE;
   DWORD n = 0;
@@ -403,7 +404,7 @@ extern "C" BOOL _declspec(dllexport) WINAPI KMSetOutput(PWSTR buf, DWORD backlen
 
   if (!Globals::get_CoreIntegration()) {  // TODO: 5442 Remove If and fix indent
     while (backlen-- > 0)
-      _td->app->QueueAction(QIT_BACK, 0);
+      _td->app->QueueAction(QIT_BACK, BK_DEFAULT);
     while (*buf)
       _td->app->QueueAction(QIT_CHAR, *buf++);
     SendDebugMessageFormat(0, sdmKeyboard, 0, "KMSetOutput: Exit");