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

Korean IME append extension issue #13788

Conversation

80ROkWOC4j
Copy link
Contributor

@80ROkWOC4j 80ROkWOC4j commented Jun 16, 2023

Related issues
#11582
#12225
#12366

Currently, there is a bug in notepad++'s add extension feature only for Korean input after it was changed to hooking-based in the commit below.
b5a5baf#diff-eeb5624a35a43795da4eb970149a9ce7d22858b678a242affd2357520ea3e9f2R607

Bug

  1. Attempting to save via Enter appends the last character to the extension.
  2. Candidate mode is similar, with more varied issues depending on IME.

Cause

  1. the hooking function is executed before the Hangul composition is completed and the last character is added after the extension.
  2. Same for Candidate mode.

Workaround

It is almost impossible to fix issue 2 while maintaining the current Enter hooking
Exiting Candidate Mode can be done by pressing Enter, ESC, number key, or clicking on a candidate character, but there is too much code to cover all of these cases.
In addition, the Windows input framework is fragmented into IMM and TSF, and various IMEs have different implementations, so it is almost impossible to determine the state of Candidate or Hangul composition through IME hooking.
I have seen differences in the events fired by different Windows versions and different IME programs for the same IME behavior.
This PR solves problem 1 and partially solves problem 2 by not saving with Enter when in Hangul mode.

Changes

When in Candidate/Hangul mode, the user completes the combination with the first Enter, encounters the text with the extension, and must press Enter again to save.
In this case, the same user experience can be achieved by sending one more Enter keystroke in the code section, but I think it's not intuitively understandable for non-Korean developers and I'm not sure this won't cause other problems, so I excluded it.

Now
ezgif com-gif-maker(3)

Now candidate
notepp_now_candidate

Now candidate, Windows' lastest version of Korean IME
notepp_now_candidate2

This PR
ezgif com-gif-maker(2)
If the user enters Enter in Korean IME mode, the user must enter Enter once more to complete the save.

This PR candidate
ezgif com-gif-maker(1)

@80ROkWOC4j 80ROkWOC4j changed the title Korean ime append extension issue 202207 Korean IME append extension issue Jun 16, 2023
@rdipardo
Copy link
Contributor

This is almost identical to #12016, except the IsHangul lambda function does not simulate an extra press of the Enter key the way the first iteration did.

@80ROkWOC4j
Copy link
Contributor Author

The more I dug into the issue with candidate mode, the more I realized it was more complicated than I initially thought. I also tried IME hooking to cleanly solve more common input situations, but I couldn't find a consistent solution across IMEs or Windows versions, so I decided to simplify the old code and defer saving due to Enter when in Korean mode.

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

_keyboardLayoutLanguage is never initialized, since it->second->_keyboardLayoutLanguage = PRIMARYLANGID(LOWORD(hkl)); is not run in any moment.

@donho donho added suspended and removed suspended labels Jun 26, 2023
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

  1. Initialize 2 member variables.
  2. Detect input language while dialog is launched and initialize _keyboardLayoutLanguage correctely.

@80ROkWOC4j 80ROkWOC4j requested a review from donho July 5, 2023 16:09
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Initialize the member variables in the definition of class.

@80ROkWOC4j 80ROkWOC4j requested a review from donho July 6, 2023 12:36
@donho donho added the accepted label Jul 7, 2023
@donho donho closed this in 6330a68 Jul 7, 2023
@xomx xomx mentioned this pull request Jan 12, 2024
@donho
Copy link
Member

donho commented Apr 15, 2024

@80ROkWOC4j
FYI: The merged PR has been reverted (906f6e4) due to the regression.

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

Successfully merging this pull request may close these issues.

None yet

3 participants