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

IME implementation in WPF #2749

Closed
wants to merge 10 commits into from
Closed

IME implementation in WPF #2749

wants to merge 10 commits into from

Conversation

a-marmer
Copy link

IME implementation in WPF which supports Japanese, Chinese, Korean languages.
Instruction how to test install it on Windows 10 https://www.coscom.co.jp/learnjapanese801/msime_win10_en1.html

@amaitland
Copy link
Member

Please remove unnessicary unrelated code changes. There are quite a few at first glance.

What versions of Windows have you tested on?

@a-marmer
Copy link
Author

Which are the unrelated code changes?
It is tested on Windows 7, Windows 10.

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

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

Comments inline, please review and fix.

CefSharp/IBrowserHost.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/Internals/WpfKeyboardHandler.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/IME/NativeIME.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/IME/IMEHandler.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/IME/IMEHandler.cs Outdated Show resolved Hide resolved
CefSharp.Example/Filters/FindReplaceResponseFilter.cs Outdated Show resolved Hide resolved
CefSharp.WinForms.Example/BrowserTabUserControl.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
@amaitland
Copy link
Member

@a-marmer View your proposed changes using the Files Changed tab above, aka https://github.com/cefsharp/CefSharp/pull/2749/files

@amaitland
Copy link
Member

Has this been tested on HighDPI displays?

Initially will need to split this out into it's own keyboard handler so it's optional. If that's sorted in the next day or two it can be inclu7in the 73 release for people to test.

Personally IME is not something I'm familiar with, nor do I have a requirement for using it.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@a-marmer
Copy link
Author

It was tested with HighDPI and it works.. Though if DPI is changed when application is running, PresentationSourceChangedHandler is not always called and then IME window position is not correct.
I tested SystemEvents.DisplaySettingsChanged += OnDisplaySettingsChanged; and OnDisplaySettingsChanged is always called. Maybe to use OnDisplaySettingsChanged?

CefSharp3.sln builds on my machine, it is not clear why build fails with appveyor, do you know why?

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

Inline functions are not supported by the default compiler used with VS2015

@AppVeyorBot
Copy link

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

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

Lots of issues remain unresolved, new ones added.

CefSharp.Wpf/WM.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/Internals/WpfIMEKeyboardHandler.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Outdated Show resolved Hide resolved
CefSharp.Wpf/ChromiumWebBrowser.cs Show resolved Hide resolved
@AppVeyorBot
Copy link

@amaitland
Copy link
Member

I've taken your PR and squashed it into 840af40

I've restructured things in 6ed722a you can review and comment here. I haven't had time to test anything. Code is now self contained, it's in the Experimental namespace as I'm not really comfortable with the code as a whole, you are unable to explain key sections which obviously isn't your fault, as I don't know what they do it's hard to give the stamp of approval. Everything is fairly consistent at least now.

@amaitland amaitland added this to the 75.0.0 milestone May 3, 2019
This was referenced May 4, 2019
@amaitland
Copy link
Member

This was effectively merged in #2757

There has been some follow up in the form of #2782

If further changes are required then please submit a followup PR, thanks 👍

@amaitland amaitland closed this May 25, 2019
@chris-araman chris-araman deleted the IME branch September 11, 2019 17:23
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.

3 participants