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.

Sign up for GitHub

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

Jump to bottom

fix(developer): handle line breaks in debugger correctly #13334

Open
wants to merge 5 commits into
base: chore/add-build-edit-action-for-delphi-projects
Choose a base branch
from
Open

fix(developer): handle line breaks in debugger correctly #13334

wants to merge 5 commits into from
+51 −27

Conversation

mcdurdin
Copy link
Member

The debugger memo takes input with \n but internally represents this as \r\n, so we need to strip out \r in order to track caret position correctly.

Fixes: #13292

User Testing

  • TEST_LINE_BREAK: start a debugger session for a keyboard. Type text, and include multiple lines of content (press Enter to add a new line). Move through the text and verify that the cursor position tracks correctly. Use backspace to delete text and ensure that deletion works across line breaks. Finally, select text, including line breaks, and ensure that deleting it works correctly.

@mcdurdin
fix(developer): handle line breaks in debugger correctly
3c86ac3 
The debugger memo takes input with \n but internally represents this as
\r\n, so we need to strip out \r in order to track caret position
correctly.

Fixes: #13292
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Feb 24, 2025
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 24, 2025 •
edited
Loading

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the B18S2 milestone Feb 24, 2025
mcdurdin
mcdurdin commented Feb 24, 2025
View reviewed changes
developer/src/tike/child/UfrmDebug.pas
case BackspaceType of
KM_CORE_BT_MARKER:
begin
Assert(m >= 1, AssertionMessage);
Assert(memo.Text[m] = #$FFFC, AssertionMessage);
Assert(m <= t.Length, AssertionMessage);
Copy link
Member Author

Choose a reason for hiding this comment

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

Additional safety check

darcywong00
darcywong00 reviewed Feb 24, 2025
View reviewed changes
developer/src/tike/debug/Keyman.System.Debug.DebugUtils.pas
begin
n := 0;
SetLength(Result, Length(Memo.Text)+1);
t := Memo.GetTextCR;
SetLength(Result, t.Length+1);
Copy link
Contributor

Choose a reason for hiding this comment

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

CI failed with

debug\Keyman.System.Debug.DebugUtils.pas(26): error E2018: Record, object or class type required 
debug\Keyman.System.Debug.DebugUtils.pas(31): error E2018: Record, object or class type required 
child\UfrmDebug.pas(37): error F2063: Could not compile used unit 'Keyman.System.Debug.DebugUtils.pas'

Is this?

Suggested change
SetLength(Result, t.Length+1);
SetLength(Result, Length(t) + 1);

Copy link
Member Author

Choose a reason for hiding this comment

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

I've fixed this by using System.SysUtils (missed from the selective commit)

@mcdurdin mcdurdin linked an issue Feb 24, 2025 that may be closed by this pull request
bug(developer): debugger appears to get mixed up on CRLF #13292
Open
darcywong00
darcywong00 approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

@dinakaranr
Copy link

Test Results

I tested this issue with the attached KeymanDeveloper"18.0.198-beta-test-13334" build(24/02/2025) on Windows 10. Here I am sharing my observation.

  • TEST_LINE_BREAK (Passed):
  1. Install Keyman attached the KeymanDeveloper"18.0.198-beta-test-13334" PR build on 24/02/2025.
  2. Open the "gff_amheric" and "Hindi modular" keyboard projects loaded in the Keyman developer.
  3. Click the "*.knm" link and then a new tab opens.
  4. Click on the "Build" tab.
  5. Compile the keyboard by clicking the "Compile Keyboard" button.
  6. Verified that the compile success message appeared.
  7. Open the debugger mode by clicking the "Start Debugging" button.
  8. A Debugger window appeared.
  9. Enter multiple lines in the debugger's box.
  10. Press the "Enter" key to add a new line.
  11. Move the cursor position between multiple lines using the "left and right" arrow.
  12. Verified that the cursor position works correctly.
  13. Press the "Backspace" key to delete text.
  14. Verified that the text deletes work after pressing the delete key across multiple lines.
  15. Select multiple lines.
  16. Press the "delete" keys
  17. Verified that the text was deleted across multiple lines.
    It works well. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darcywong00 darcywong00 darcywong00 approved these changes

@SabineSIL SabineSIL Awaiting requested review from SabineSIL SabineSIL is a code owner

@markcsinclair markcsinclair Awaiting requested review from markcsinclair markcsinclair is a code owner

@srl295 srl295 Awaiting requested review from srl295 srl295 is a code owner

Assignees
No one assigned
Labels
developer/ide/ developer/ fix has-user-test
Projects
Keyman
Status: No status
Milestone
B18S2
Development

Successfully merging this pull request may close these issues.

bug(developer): debugger appears to get mixed up on CRLF
3 participants
@mcdurdin @dinakaranr @darcywong00