Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(windows): handle CRLF specific to windows #10697

Closed
wants to merge 8 commits into from
33 changes: 33 additions & 0 deletions windows/src/engine/keyman32/appcontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,36 @@ ContextItemToAppContext(km_core_context_item *contextItems, PWSTR outBuf, DWORD
delete[] buf;
return TRUE;
}

FormatContextResult
format_context_for_core(LPCWSTR windows_context, LPWSTR core_context, uint32_t output_size) {
rc-swag marked this conversation as resolved.
Show resolved Hide resolved
if (windows_context == nullptr || core_context == nullptr) {
return frERROR;
}
// Return early if windowsContext does not contain '\r\n'
if (wcsstr(windows_context, L"\r\n") == nullptr) {
return frNO_CHANGE;
}

auto windows_context_length = wcsnlen_s(windows_context, MAXCONTEXT);

if (output_size < windows_context_length) {
rc-swag marked this conversation as resolved.
Show resolved Hide resolved
return frERROR;
}

LPCWSTR win_ptr = windows_context;
LPWSTR core_ptr = core_context;
while (*win_ptr != L'\0') {
if (*win_ptr == L'\r' && *(win_ptr + 1) == L'\n') {
win_ptr++; // skip '\r'

} else {
*core_ptr++ = *win_ptr++;
}
}
*core_ptr++ = L'\0';
return frUPDATED;
}



24 changes: 24 additions & 0 deletions windows/src/engine/keyman32/appcontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,30 @@ class AppContext {
*/
BOOL ContextItemToAppContext(km_core_context_item *contextItems, PWSTR outBuf, DWORD len);

/**
* frERROR: If an error occurs, such as null pointers or buffer to small.
rc-swag marked this conversation as resolved.
Show resolved Hide resolved
* frUPDATED: If the context has been modified.
* frNO_CHANGE: If no modifications were made.
*/
enum FormatContextResult { frERROR, frUPDATED, frNO_CHANGE };

/**
* Pre-processes the given wide-character string context for passing to the
* Keyman core. Currently, this consists of replacing "\r\n" sequences with
* "\n".
*
* @param windows_context The wide-character string to be pre-processed. It
* must be null-terminated.
* @param[in,out] core_context The buffer to store the pre-processed string
* for the Keyman core.
* @param output_size The size of the buffer core_context, in number of wide
* characters.
* @return FormatContextResult Indicates the result of the pre-processing
* operation:
*/
FormatContextResult format_context_for_core(LPCWSTR windows_context, LPWSTR core_context, uint32_t output_size);




#endif
9 changes: 8 additions & 1 deletion windows/src/engine/keyman32/kmprocess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,16 @@ BOOL fOutputKeystroke;
static BOOL
Process_Event_Core(PKEYMAN64THREADDATA _td) {
WCHAR application_context[MAXCONTEXT];
WCHAR core_context[MAXCONTEXT];
LPWSTR context_ptr = application_context;
if (_td->app->ReadContext(application_context)) {
// pre-process any windows specific text
if (format_context_for_core(application_context, core_context, MAXCONTEXT) == frUPDATED) {
context_ptr = core_context;
}
km_core_context_status result;
result = km_core_state_context_set_if_needed(_td->lpActiveKeyboard->lpCoreKeyboardState, reinterpret_cast<const km_core_cp *>(application_context));
result = km_core_state_context_set_if_needed(
_td->lpActiveKeyboard->lpCoreKeyboardState, reinterpret_cast<const km_core_cp*>(context_ptr));
if (result == KM_CORE_CONTEXT_STATUS_ERROR || result == KM_CORE_CONTEXT_STATUS_INVALID_ARGUMENT) {
SendDebugMessageFormat(0, sdmGlobal, 0, "Process_Event_Core: km_core_state_context_set_if_needed returned [%d]", result);
}
Expand Down
32 changes: 22 additions & 10 deletions windows/src/engine/keyman32/kmprocessactions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,33 @@
#include "pch.h"

/**
* Adds characters to the queue to be inserted into the application
* Adds converts the characters to UTF-16 and inserts CR as it places them
* in the queue.
* @param app A pointer to the AITIP instance.
* @param outputString A null-terminated string of characters to insert into the application
* @param core_output A null-terminated string of characters in UTF-32 format to insert into the application
*/

// When the windows platform code is updated to not use QueueAction
// This function would be better to take as input the km_core_usv*
// string form the core and generate a new buffer that is
// windows formated (CRLF) and encoded as UTF-16. Currently it just
// does both in one parse while adding it to the queue to reduce the double
// processing.

static void
processUnicodeChar(AITIP* app, const km_core_usv* outputString) {
while (*outputString) {
if (Uni_IsSMP(*outputString)) {
app->QueueAction(QIT_CHAR, (Uni_UTF32ToSurrogate1(*outputString)));
app->QueueAction(QIT_CHAR, (Uni_UTF32ToSurrogate2(*outputString)));
process_output_string(AITIP* app, const km_core_usv* core_output) {
while (*core_output) {
if (*core_output == L'\n') {
// Insert '\r' for Windows platform applications
app->QueueAction(QIT_CHAR, L'\r');
}
rc-swag marked this conversation as resolved.
Show resolved Hide resolved
if (Uni_IsSMP(*core_output)) {
app->QueueAction(QIT_CHAR, (Uni_UTF32ToSurrogate1(*core_output)));
app->QueueAction(QIT_CHAR, (Uni_UTF32ToSurrogate2(*core_output)));
} else {
app->QueueAction(QIT_CHAR, *outputString);
app->QueueAction(QIT_CHAR, *core_output);
}
outputString++;
core_output++;
}
}

Expand Down Expand Up @@ -110,7 +122,7 @@ BOOL ProcessActions(BOOL* emitKeystroke)
_td->CoreProcessEventRun = FALSE;

processBack(_td->app, core_actions->code_points_to_delete, core_actions->deleted_context);
processUnicodeChar(_td->app, core_actions->output);
process_output_string(_td->app, core_actions->output);
if (core_actions->persist_options != NULL) {
processPersistOpt(core_actions, _td->lpActiveKeyboard);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,34 @@ TEST(AppContext, AppContext_Delete) {
testContext.Delete();

}

// Test pre context function

TEST(AppContext, format_context_for_core) {
wchar_t windows_context[] = L"Hello\r\nWorld\r\nYou Rock";
wchar_t core_context[MAXCONTEXT];
wchar_t expected_string[] = L"Hello\nWorld\nYou Rock";
EXPECT_TRUE(format_context_for_core(windows_context, core_context, MAXCONTEXT) == frUPDATED);
EXPECT_STREQ(expected_string, core_context);
}

// No CRLF `\r\n` found
TEST(AppContext, format_context_for_core_unchanged) {
wchar_t windows_context[] = L"Hello World You Rock";
wchar_t core_context[MAXCONTEXT];
core_context[0] = L'\0'; // for test just to verify it wasn't changed
wchar_t expected_string[] = L"";
EXPECT_EQ(format_context_for_core(windows_context, core_context, MAXCONTEXT), frNO_CHANGE);
EXPECT_STREQ(expected_string, core_context);
}

//// Core Context too short
TEST(AppContext, format_context_for_core_too_short) {
wchar_t windows_context[] = L"Hello\r\nWorld\r\nYou Rock";
wchar_t core_context[2];
core_context[0] = L'\0'; // for test just to verify it wasn't changed
wchar_t expected_string[] = L"";
EXPECT_EQ(format_context_for_core(windows_context, core_context, 2), frERROR);
//EXPECT_STREQ(expected_string, core_context);
}

Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,46 @@ class KMPROCESSACTIONS : public ::testing::Test {
// Now that the AITIP no longer contains a context
// this test is just reduced to checking the size of the
// action queue
TEST_F(KMPROCESSACTIONS, processUnicodeChartest) {
TEST_F(KMPROCESSACTIONS, processOutputStringtest) {
// just check something has been added to the que
WCHAR callbuf[MAXCONTEXT];
AITIP testApp;
int expectedQueueSize = 3; // 1 + surrogate pair
km_core_usv output_string[] = {0x0041, 0x010000, 0};
processUnicodeChar(&testApp, &output_string[0]);
process_output_string(&testApp, &output_string[0]);
int queueSize = testApp.GetQueueSize();
EXPECT_EQ(queueSize, expectedQueueSize);
}

// Test where there is no LF `\n' most common case
TEST_F(KMPROCESSACTIONS, processOutputStringNoChange) {
WCHAR callbuf[MAXCONTEXT];
AITIP testApp;
int expectedQueueSize = 20;
// L"Hello World You Rock";
km_core_usv output_string[] = {0x0048, 0x0065, 0x006C, 0x006C, 0x006F, 0x0020, 0x0057, 0x006F, 0x0072, 0x006C, 0x0064,
0x0020, 0x0059, 0x006F, 0x0075, 0x0020, 0x0052, 0x006F, 0x0063, 0x006B, 0};
//km_core_usv output_string[] = {0x0041, 0x010000, 0};
process_output_string(&testApp, &output_string[0]);
int queueSize = testApp.GetQueueSize();
EXPECT_EQ(queueSize, expectedQueueSize);

}

// Test inserting '\r'
TEST_F(KMPROCESSACTIONS, processOutputStringInsertCR) {
WCHAR callbuf[MAXCONTEXT];
AITIP testApp;
int expectedQueueSize = 22;
// L"Hello\nWorld\nYou Rock";
km_core_usv output_string[] = {0x0048, 0x0065, 0x006C, 0x006C, 0x006F, 0x000A, 0x0057, 0x006F, 0x0072, 0x006C, 0x0064,
0x000A, 0x0059, 0x006F, 0x0075, 0x0020, 0x0052, 0x006F, 0x0063, 0x006B, 0x0000};
// km_core_usv output_string[] = {0x0041, 0x010000, 0};
process_output_string(&testApp, &output_string[0]);
int queueSize = testApp.GetQueueSize();
EXPECT_EQ(queueSize, expectedQueueSize);
}




Loading