Check for both compose string and result string. #29620
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I'm trying to write a test for this. |
gspencergoog
force-pushed
the
fix_sogou_ime
branch
3 times, most recently
from
c62330a
to
c3ca85c
Compare
There was a problem hiding this comment.
Looks good modulo one comment. I'm fine with us fixing that up later in order to get this issue fixed, but would be good to file a bug and add a TODO in that case.
Also, looks like there are a couple cases where mock expectations are not met, causing the test to fail.
| @@ -217,6 +217,9 @@ class WindowWin32 { | |||
| // Used to process key messages. Exposed for dependency injection. | |||
| virtual uint32_t Win32MapVkToChar(uint32_t virtual_key); | |||
|
|
|||
| // Accessor so that it can be mocked in tests. | |||
| virtual TextInputManagerWin32* get_text_input_manager(); | |||
There was a problem hiding this comment.
I wonder if we could do this via injection instead? That would avoid adding a public method here and avoid the possibility of someone accidentally breaking things by using that raw private var in some future change to WindowWin32, and potentially FlutterWindowWin32.
There was a problem hiding this comment.
OK, I agree that the temptation of using the raw private is possibly bad: I'll redo it as injection. I did think about injection as an option, but the mocking framework wanted to use pointers, and I didn't want to have to inject a heap allocated object, and passing mutable references seems like bad form. I'll probably inject it as a unique_ptr.
gspencergoog
force-pushed
the
fix_sogou_ime
branch
from
c3ca85c
to
75b6d92
Compare
gspencergoog
force-pushed
the
fix_sogou_ime
branch
from
75b6d92
to
cc1df8c
Compare
Description
It turns out that unlike the Microsoft IME, Sogou sends a single
IME_COMPOSITIONmessage that has both theGCS_COMPSTRandGCS_RESULTSTRflags set, and we check for a composition string before result string, with an "else" clause to check for the result string, so ifGCS_COMPSTRis set, we never check for a result string.Related Issues