-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add IME support #4614
base: develop
Are you sure you want to change the base?
Add IME support #4614
Changes from all commits
8955049
dc34dad
e572ca8
a09bbd7
0ace386
3864d4e
85a122a
77bc7ee
48b1eed
a641ac2
5401a91
059c417
5c65f42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
github: [fyne-io, andydotxyz, toaster, Jacalz, changkun] | ||
github: [fyne-io, andydotxyz, toaster, Jacalz, changkun, dweymouth, lucor] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,18 @@ func (c *Canvas) Focused() fyne.Focusable { | |
return mgr.Focused() | ||
} | ||
|
||
// Preeditable return the current preeditable object only when focused | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will crash if the input is not Preeditable. It should not be needed as it will always be the same object returned by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I didn't notice it when running fyne_demo. Thank you for pointing that out. I had a chance to learn the correct casting method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would only happen if you started to interact with an input that had not implemented that interface. |
||
func (c *Canvas) Preeditable() fyne.Preeditable { | ||
focused := c.Focused() | ||
if focused == nil { | ||
return nil | ||
} | ||
if preeditable, ok := focused.(fyne.Preeditable); ok { | ||
return preeditable | ||
} | ||
return nil | ||
} | ||
|
||
// FocusGained signals to the manager that its content got focus. | ||
// Valid only on Desktop. | ||
func (c *Canvas) FocusGained() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,11 @@ import ( | |
"github.com/go-gl/glfw/v3.3/glfw" | ||
) | ||
|
||
const GLFW_X11_ONTHESPOT glfw.Hint = 0x00052002 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this be declared in the GLFW project? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andydotxyz Thanks for trying. As I wrote at the beginning of this PR, in order to check the operation of this PR, you need to clone both the modified fyne and glfw and change go.mod. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kanryu If this is under active development but not completed, please mark the PR as draft. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Jacalz It's difficult to explain in a few words because the work spanned multiple projects, but achieving this PR required major modifications to go-gl/glfw, and they effectively refused to merge. Because of this, we are currently unable to proceed with this PR any further. It's up to the people at the Fyne project to decide whether or not this PR should be marked as draft. Not that I would complain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kanryu I am entirely aware of the situation. My personal opinion is that if a PR can't be tested as it is, i.e. isn't ready to be merged, it should be a draft. Those that are interested in testing it can do so but we maintainers don't have to remember that it isn't ready. It makes it easier to grasp which pull requests are waiting for reviews. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This can be done with a replace directive in the go.mod so that this PR can simply be checked out and tested without pre-requisites. That also makes it clear of the requirements to compile. |
||
|
||
func (d *gLDriver) initGLFW() { | ||
initOnce.Do(func() { | ||
glfw.InitHint(GLFW_X11_ONTHESPOT, 1) // enable IME callbacks, hint must be set before Init() | ||
err := glfw.Init() | ||
if err != nil { | ||
fyne.LogError("failed to initialise GLFW", err) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ package glfw | |
import ( | ||
"bytes" | ||
"context" | ||
"fmt" | ||
"image" | ||
_ "image/png" // for the icon | ||
"runtime" | ||
|
@@ -677,6 +678,47 @@ func (w *window) charInput(viewport *glfw.Window, char rune) { | |
w.processCharInput(char) | ||
} | ||
|
||
var counterIme int | ||
|
||
// IME | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment should not be needed here. Where comments are added it usually implies naming is insufficient. |
||
func (w *window) imeStatus(_ *glfw.Window) { | ||
counterIme++ | ||
if preeditable := w.canvas.Preeditable(); preeditable != nil { | ||
// This callback function is called by the focused widget and tells the system the exact canvas coordinates | ||
// of the text cursor on the window. The system will display an IME Candidate Window with those coordinates | ||
preeditable.ReceiveCursorPositionChangedCallback(func(pos fyne.Position, size fyne.Size) { | ||
canvasScale := w.canvas.scale | ||
w.viewport.SetPreeditCursorRectangle(int(pos.X*canvasScale), int(pos.Y*canvasScale), 100, int(size.Height)) | ||
w.viewport.UpdatePreeditCursorRectangle() | ||
}) | ||
} | ||
} | ||
|
||
func (w *window) preedit( | ||
_ *glfw.Window, | ||
preeditCount int, | ||
preeditString string, | ||
blockCount int, | ||
blockSizes string, | ||
focusedBlock int, | ||
caret int, | ||
) { | ||
w.processPreedit(preeditString) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like it would be easier to just inline processPreedit here as it is barely a single line and only called from this location There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andydotxyz Unfortunately processPreedit and Preeditable.PreeditChanged are temporary interfaces. This is because the original Preedit response requires a more complex response. It is necessary to additionally display the conversion candidates for input content provided by the IME and the IME's own caret. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Should we mark this as a draft PR then instead of one ready for review? |
||
} | ||
|
||
func (w *window) preeditCandidate( | ||
glwin *glfw.Window, | ||
candidatesCount int, | ||
selectedIndex int, | ||
pageStart int, | ||
pageSize int, | ||
) { | ||
candidate := glwin.GetPreeditCandidate(selectedIndex) | ||
if candidate != nil { | ||
fmt.Println("candidates", *candidate) | ||
} | ||
} | ||
|
||
func (w *window) focused(_ *glfw.Window, focused bool) { | ||
w.processFocused(focused) | ||
} | ||
|
@@ -765,11 +807,15 @@ func (w *window) create() { | |
win.SetRefreshCallback(w.refresh) | ||
win.SetContentScaleCallback(w.scaled) | ||
win.SetCursorPosCallback(w.mouseMoved) | ||
win.SetPreeditCallback(w.preedit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't compile for me. I guess the |
||
win.SetImeStatusCallback(w.imeStatus) | ||
win.SetPreeditCandidateCallback(w.preeditCandidate) | ||
win.SetMouseButtonCallback(w.mouseClicked) | ||
win.SetScrollCallback(w.mouseScrolled) | ||
win.SetKeyCallback(w.keyPressed) | ||
win.SetCharCallback(w.charInput) | ||
win.SetFocusCallback(w.focused) | ||
win.SetInputMode(glfw.ImeOwnerDraw, glfw.False) | ||
|
||
w.canvas.detectedScale = w.detectScale() | ||
w.canvas.scale = w.calculatedScale() | ||
|
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 agree with this sentiment - adding it to Focusable would be a breaking change.
However can it be worded less like a workaround.
i.e. "Widgets that implement
Focusable
may also want to implementPreeditChanged
to handle IME input?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.
That comment certainly makes the relationship with Focusable clearer. People who see this code for the first time may not understand the deep relationship between Focusable and Preeditable.