Skip to content

Conversation

@lukhnos
Copy link
Collaborator

@lukhnos lukhnos commented Dec 17, 2025

This fixes a regression introduced in #225 that consumes the Shift+[1-9] keys when there are candidates. Key::ascii alone is not enough to determine that it's produced from a Shift+[1-9] key, and such keys must be not be consumed in KeyHandler when the candidate panel is visible. The regression also caused Space to be consumed unconditionally, casuing Space to fail to work as the page flip key in the candidate panel.

This also exits the NumberInput state when a non-alphanumeric key is pressed when the number composition buffer is empty. This fixes the problem that the number prompt ("[數字]") got force-committed by the client app when fcitx5 was told that the key was not handled.

Copilot AI review requested due to automatic review settings December 17, 2025 18:34
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses a couple of regressions in the NumberInput state. The changes correctly prevent the key handler from consuming keys meant for the candidate panel (like Shift+[1-9] or Space) when candidates are visible. It also fixes an issue where non-printable keys like Esc would cause the client to incorrectly commit the preedit text when the number buffer is empty. The new logic correctly exits the NumberInput state in this case. I've found a minor discrepancy between the described behavior and the implementation for printable non-alphanumeric keys on an empty buffer, and I've left a comment with a suggestion to align them. Overall, these are good improvements to the user experience.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a regression in NumberInput key handling that was consuming keys like Shift+[1-9] and Space that should be handled by the candidate panel. The changes restructure the key handling logic to properly delegate to the candidate panel when candidates are visible, and to exit the NumberInput state when non-alphanumeric keys are pressed with an empty number buffer.

  • Reordered key handling logic to check for candidates before rejecting keys
  • Added logic to return unhandled (false) when candidates are visible, allowing the candidate panel to process keys like Space (page flip) and Shift+[1-9] (candidate selection)
  • Changed behavior for non-alphanumeric, non-printable keys (ESC, cursor keys) to exit NumberInput state when the number buffer is empty

@zonble
Copy link
Collaborator

zonble commented Dec 18, 2025

I have a quetion. If we remove the flow of handling ESC key, how to let the users to canccel the number input state?

@lukhnos
Copy link
Collaborator Author

lukhnos commented Dec 18, 2025

I have a quetion. If we remove the flow of handling ESC key, how to let the users to canccel the number input state?

Good question. My original change would pass the ESC key to the candidate key handler, which would also cancel the candidate panel—but then let the key handler enter the Inputting state. The effect was the same, but it was not techncially correct, as the state should be Empty again. So I reverted that part, and Esc is now handled explicitly, like before.

I have made some changes to McBopomofo.cpp though. With my changes in KeyHandler, the keys in NumberInput state can only be unhandled if there are candidates (i.e. if the input buffer is not empty). In that case, the key will then always correctly pass to the candidate key handler. Hence the change in McBopomofo.cpp, which removes a check that is no longer needed, as it will always be false.

@lukhnos lukhnos requested a review from zonble December 18, 2025 17:36
keyEvent.filterAndAccept();
return;
}
InputStates::NumberInput* currentNumberInput =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This condition will never be true, since the key can only be unhandled if there are candidates in the NumberInput state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I meant to put this on line 722, but put in on 719 instead.)

This fixes a regression introduced in openvanilla#225
that consumes the Shift+[1-9] keys when there are candidates.
`Key::ascii` alone is not enough to determine that it's produced from a
Shift+[1-9] key, and such keys must be not be consumed in KeyHandler
when the candidate panel is visible. The regression also caused Space
to be consumed unconditionally, casuing Space to fail to work as the
page flip key in the candidate panel.

This also exits the NumberInput state when a non-alphanumeric key
(excluding Esc) is pressed when the number composition buffer is empty.
This fixes the problem that the number prompt ("[數字]") got
force-committed by the client app when fcitx5 was told that the key was
not handled.
@zonble zonble merged commit 04dd165 into openvanilla:master Dec 19, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants