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

fix(mac): delete correct number of characters from current context when processing BMP or SMP deletes #11086

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

sgschantz
Copy link
Contributor

@sgschantz sgschantz commented Mar 27, 2024

When processing keystrokes for compliant apps (which have the ability to replace text while inserting characters from Keyman), the number of characters to delete must be determined correctly whether the context contains BMP code points or surrogate pairs. Also, if there is any text selected when the character is typed, the selection should also be replaced by the newly inserted character.

Fixes #1643

User Testing

  • TEST_REPLACE_BMP_CHARACTER: typing a character with one selected causes it to be replaced
  1. Open the Stickies app and create a new Note
  2. Switch to the EuroLatin (SIL) keyboard
  3. Type ab
  4. Select the letter 'b' that was just typed
  5. Type x
  6. Confirm that the b was replaced by x, and the text now reads 'ax'
  • TEST_REPLACE_SURROGATE_PAIR_CHARACTER: processing a character that causes the replacement of a surrogate pair produces the correct, uncorrupted output
  1. Open the Stickies app and create a new Note
  2. Switch to the Malar Tirhuta keyboard
  3. Type k
  4. Confirm that the letter 𑒏𑓂 was typed
  5. Type e
  6. Confirm that output now reads: 𑒏𑒹
  7. Confirm that the text can be selected and successfully copied into a TextEdit document
  • TEST_REPLACE_MULTIPLE_CHARACTERS: processing a character that causes the replacement of multiple characters produces the correct output (multiple deletes are returned from core because this key combination causes the Khmer characters to be re-ordered)
  1. Open the Stickies app and create a new Note
  2. Switch to the Khmer Angkor keyboard
  3. Hold down shift and type p
  4. Type ;
  5. Confirm that output now reads: ភើ
  6. Type jl
  7. Confirm that the text now reads ភ្លើ

@sgschantz sgschantz added the mac/ label Mar 27, 2024
@sgschantz sgschantz added this to the B17S4 milestone Mar 27, 2024
@sgschantz sgschantz self-assigned this Mar 27, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Mar 27, 2024

User Test Results

Test specification and instructions

  • TEST_REPLACE_BMP_CHARACTER (PASSED): Tested with the attached PR build (Keyman 17.0.291-beta-test-11086) on an macOS Sonoma and here is my observation: 1. Opened Stickies app and created new Note. 2. Switched to EurLatin (SIL) keyboard. 3. Typed ab. Selected the letter 'b' then typed x. 4. Verified that the letter 'b' was replaced by x and the text 'ax' appeared on the Note. (notes)
  • TEST_REPLACE_SURROGATE_PAIR_CHARACTER (PASSED): 1. Opened the Stickies app and created a new Note. 2. Installed Malar Tirhuta Keyboard. Switched to Malar Tirhuta keyboard. 3. Typed 'k'. Then, typed the letter 'e'. Confirmed that it showed the expected result on the Note. 4. Copied then pasted the text in the TextEdit document and verified that it is showing the correct output. (notes)
  • TEST_REPLACE_MULTIPLE_CHARACTERS (PASSED): 1. Opened the Stickies app and created a new Note. 2. Switched to Khmer Angkor keyboard. 3. Hold down the Shift key then type 'p' letter. Typed ; . 4. Then, typed jl. 5. Verified that the text outputs the expected result on the Note. (notes)

Test Artifacts

@github-actions github-actions bot added the fix label Mar 27, 2024
@sgschantz sgschantz linked an issue Mar 28, 2024 that may be closed by this pull request
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Mar 28, 2024
@sgschantz sgschantz marked this pull request as ready for review March 28, 2024 03:29
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM, this makes sense to me and I think it all lines up. I wasn't sure I spotted tests with complex selection vs text replacement scenarios though?

@bharanidharanj
Copy link

Test Results

  • TEST_REPLACE_BMP_CHARACTER (PASSED): Tested with the attached PR build (Keyman 17.0.291-beta-test-11086) on an macOS Sonoma and here is my observation: 1. Opened Stickies app and created new Note. 2. Switched to EurLatin (SIL) keyboard. 3. Typed ab. Selected the letter 'b' then typed x. 4. Verified that the letter 'b' was replaced by x and the text 'ax' appeared on the Note.

  • TEST_REPLACE_SURROGATE_PAIR_CHARACTER (PASSED): 1. Opened the Stickies app and created a new Note. 2. Installed Malar Tirhuta Keyboard. Switched to Malar Tirhuta keyboard. 3. Typed 'k'. Then, typed the letter 'e'. Confirmed that it showed the expected result on the Note. 4. Copied then pasted the text in the TextEdit document and verified that it is showing the correct output.

  • TEST_REPLACE_MULTIPLE_CHARACTERS (PASSED): 1. Opened the Stickies app and created a new Note. 2. Switched to Khmer Angkor keyboard. 3. Hold down the Shift key then type 'p' letter. Typed ; . 4. Then, typed jl. 5. Verified that the text outputs the expected result on the Note.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 28, 2024
@mcdurdin mcdurdin modified the milestones: B17S4, B17S5 Mar 30, 2024
@sgschantz
Copy link
Contributor Author

sgschantz commented Apr 1, 2024

LGTM, this makes sense to me and I think it all lines up. I wasn't sure I spotted tests with complex selection vs text replacement scenarios though?

Which scenarios are you referring to? The focus of this PR is to fix #1643. I am hoping to not introduce any selection-related issues, but I have not attempted to research all selection scenarios and test those as part of this change. There is also an open issue #10093 that deals with that, though it specifies legacy apps in the description.

@mcdurdin
Copy link
Member

mcdurdin commented Apr 1, 2024

Which scenarios are you referring to? The focus of this PR is to fix #1643. I am hoping to not introduce any selection-related issues, but I have not attempted to research all selection scenarios and test those as part of this change.

I was going off this in the issue description:

Also, if there is any text selected when the character is typed, the selection should also be replaced by the newly inserted character.

@sgschantz sgschantz merged commit f6b9158 into beta Apr 1, 2024
6 checks passed
@sgschantz sgschantz deleted the fix/mac/1643-mishandled-SMP branch April 1, 2024 06:38
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.299-beta

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

Successfully merging this pull request may close these issues.

bug(mac): generated backspace of SMP character corrupts text
4 participants