-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Fix IME submissions dropping leading digits #4359
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
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review this |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
abb5595 to
593336d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a couple of small tweaks on this based on reading through the interaction between the chat composer and the paste_burst code. I kept the self.active = true and removed the call to clear_after_explicit_paste(). I intentionally let an empty string be handled as Some("") instead of None.
The calling code in chat composer calls the flush_before_modified() and then immediately calls the handle_paste() which calls clear_after_explicit_paste(). These are temporally coupled in a bit of a weird way. This logic could be improved to remove the temporal coupling, but for now I'd prefer to leave that there and not have code that calls this public method internally as it makes it more difficult to reason about the state machine and this makes the interactions temporally and directly coupled.
The Some("") path gets treated as a noop paste, which is fine - it shouldn't cause problems as inserting a blank string does nothing, and resetting the other parts of state is seems cheap.
The self.active is probably unnecessary as the later call from chat_composer to clear_after... will set this to false, but I left it there mostly to keep this as-is and only change the problematic part.
Can you please pull this updated copy down and take a quick look. I think it should still work as it did previous, but testing on real language specific keyboards can sometimes highlight other problems. I'm only able to test using the Romanji keyboard settings, so I'm not sure that I'm reproducing your issue 100% accurately.
Once you've tested the changes I made, I'll go ahead and merge this.
Side note:
Ideally this struct probably should be refactored a bit to an enum something like:
enum PasteBurst {
Inactive,
Active { last_plain_char_time, ... }
}Or possibly a few more states that have to do with how many keys are held / buffered to make it a bit more clear how the state machine works. But that's an exercise for another time.
I also slapped a quick rebase on main to bring this up to date.
|
Thank you for update! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR
Summary
Fixes #4356
Testing