Skip to content

Comments

fix(developer): handle CRLF as LF internally in LDML debugger#15616

Open
mcdurdin wants to merge 1 commit intomasterfrom
fix/developer/15601-crlf-in-ldml-debugger
Open

fix(developer): handle CRLF as LF internally in LDML debugger#15616
mcdurdin wants to merge 1 commit intomasterfrom
fix/developer/15601-crlf-in-ldml-debugger

Conversation

@mcdurdin
Copy link
Member

While the debugger memo internally uses CRLF, in all references, CRLF should be converted to LF for consistent text manipulation operations. This follows a similar fix in the kmn debugger in #13334.

Fixes: #15601
Relates-to: #13334

User Testing

TEST_CRLF: Try to reproduce the scenario in #15601, and attempt various ways to confuse the debugger's char position with the Enter key. Pass if the debugger and debugger character grid match expected output.

While the debugger memo internally uses CRLF, in all references, CRLF
should be converted to LF for consistent text manipulation operations.
This follows a similar fix in the kmn debugger in #13334.

Fixes: #15601
Relates-to: #13334
@github-project-automation github-project-automation bot moved this to Todo in Keyman Feb 24, 2026
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Feb 24, 2026
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 24, 2026

User Test Results

Test specification and instructions

  • TEST_CRLF (OPEN)
Results Template
# Test Results

* **TEST_CRLF (OPEN):** notes

Test Artifacts

Copy link
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although the commit message/PR title and description is misleading: we're handling CRLF as CR internally, not LF.

procedure TfrmLdmlKeyboardDebug.UpdateCharacterGrid; // I4808
var
start, len: Integer;
t: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah - these 90s variable names! Can we rename it to text or something more meaningful?

Suggested change
t: string;
text: string;


context := Copy(memo.Text, lhs.Length + 1, selection.Start - lhs.Length);
rhs := Copy(memo.Text, lhs.Length + context.Length + 1, MaxInt);
context := Copy(memo.GetTextCR, lhs.Length + 1, selection.Start - lhs.Length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin.ai:

Missing memo.Text to memo.GetTextCR conversion on line 585 causes incorrect string slicing with newlines

On line 585, lhs is computed using memo.Text (which returns text with CRLF line endings), but selection.Start is in RichEdit's CR-only coordinate space (from EM_GETSEL). This lhs value is then used on lines 590–591 with memo.GetTextCR (CR-only), creating a coordinate mismatch.

Root Cause and Impact

RichEdit's EM_GETSEL returns positions where each newline counts as 1 character (CR-only). However, memo.Text returns text with CRLF (2 characters per newline). So Copy(memo.Text, 1, selection.Start) will extract fewer characters than intended when the text contains newlines — each newline in memo.Text takes 2 chars but selection.Start only allocated 1 char per newline.

This means lhs.Length will be wrong, causing the subsequent context and rhs computations on lines 590–591 (which correctly use memo.GetTextCR) to slice at wrong positions. The reconstructed string lhs + output + rhs on line 625 will be corrupted, leading to garbled text in the debug memo whenever the content includes line breaks.

Every other memo.Textmemo.GetTextCR conversion was done in this PR, but this one was missed.

end;
Exit;
end;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin.ai:

Removed Ctrl guard in DoHandleShortcut causes shortcuts to fire on regular keypresses

The old code had a guard if GetKeyState(VK_CONTROL) >= 0 then ... Exit; that ensured the Ctrl+key shortcut handling (SelectAll, Copy, Paste, Cut, Undo) only executed when Ctrl was actually held down. This guard was removed in the PR.

Root Cause and Impact

DoHandleShortcut is called from DoEmitKeystroke (line 467), which runs whenever actions.emit_keystroke <> 0 (line 643). This happens when the LDML keyboard does not match a keystroke. If a user types a regular 'a', 'c', 'v', 'x', or 'z' key on a keyboard that doesn't define rules for those keys, emit_keystroke is set to 1, the keystroke is dispatched to the memo (inserting the character), and then DoHandleShortcut is called.

Without the Ctrl guard, the case vk of Ord('A'): memo.SelectAll; ... block at lines 431–438 now matches on the virtual key code alone. For example, pressing 'A' without Ctrl would both insert 'a' into the memo AND call memo.SelectAll, selecting all text. Similarly, pressing 'V' would insert 'v' AND paste from clipboard, 'Z' would insert 'z' AND undo, etc.

The fix for removing the Enter key handling (which was the apparent goal) should have preserved the Ctrl guard while only removing the Enter-specific block inside it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

bug(developer): wrong output in test area for minimal ldml keyboard

2 participants