Add IME support #4614

kanryu · 2024-02-06T11:50:05Z

Description:

As everyone knows, fyne does not currently support IME. This is fundamentally caused by the fact that glfw does not support IME, and cannot be resolved by fyne's efforts alone. So I solved this problem by adding a hack to both go-gl/glfw and fyne. Please check it on your PC.

Fixes #618

Supported environments

supported
- Windows
- Mac
- Linux(X11, Wayland)
not supported
- Android
- iOS

How to try this

git clone https://github.com/kanryu/fyne
git clone https://github.com/kanryu/glfw
cd fyne
go mod edit -replace github.com/go-gl/glfw/v3.3/glfw=../glfw/v3.4/glfw
go run cmd/fyne_demo/main.go

Fyne.Demo.2024-02-06.19-48-28.mp4

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style and have Since: line.
Any breaking changes have a deprecation path or have been discussed.
Check for binary size increases when importing new modules.

andydotxyz

Thanks for working on this really complex task.
I have some suggestions inline.

andydotxyz · 2024-02-06T17:22:30Z

canvasobject.go

@@ -75,6 +75,14 @@ type Focusable interface {
 	TypedKey(*KeyEvent)
 }

+// Preeditable describes any CanvasObject that can respond to IME inputs.
+// Originally, it should be a method of Focusable, but only a few widgets need PreeditChanged,
+// and adding a method to Focusable will have a big impact on others, so defined as a new interface.


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 implement PreeditChanged to handle IME input?

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.

andydotxyz · 2024-02-06T17:23:38Z

internal/driver/common/canvas.go

@@ -191,6 +191,15 @@ func (c *Canvas) Focused() fyne.Focusable {
 	return mgr.Focused()
 }

+// Preeditable return the current preeditable object only when focused


This will crash if the input is not Preeditable. It should not be needed as it will always be the same object returned by Focused()

This will crash if the input is not Preeditable.

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.

It would only happen if you started to interact with an input that had not implemented that interface.
Something like the checkbox I would think for example.

andydotxyz · 2024-02-06T17:25:15Z

internal/driver/glfw/window_desktop.go

@@ -677,6 +678,32 @@ func (w *window) charInput(viewport *glfw.Window, char rune) {
 	w.processCharInput(char)
 }

+var counterIme int
+
+// IME


A comment should not be needed here. Where comments are added it usually implies naming is insufficient.

andydotxyz · 2024-02-06T17:25:35Z

internal/driver/glfw/window_desktop.go

+	focusedBlock int,
+	caret int,
+) {
+	fmt.Println(


Please don't leave in debug

@andydotxyz ok, removed.

andydotxyz · 2024-02-06T17:26:30Z

internal/driver/glfw/window_desktop.go

+		"focusedBlock", focusedBlock,
+		"caret", caret,
+	)
+	w.processPreedit(preeditString)


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

@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.

OK. Should we mark this as a draft PR then instead of one ready for review?

andydotxyz · 2024-02-06T17:27:04Z

widget/entry.go

@@ -102,6 +105,7 @@ type Entry struct {

 // NewEntry creates a new single line entry widget.
 func NewEntry() *Entry {
+	//	e := &Entry{Wrapping: fyne.TextTruncate, PreeditText: "(日本語)"}


Please don't leave dead code in comments

@andydotxyz OK, removed

andydotxyz · 2024-02-06T17:27:27Z

widget/entry.go

-	Text     string
+	shortcut    fyne.ShortcutHandler
+	Text        string
+	PreeditText string


Why is this a public field? it feels like it should be an internal implementation if required at all?

@andydotxyz OK, privated

andydotxyz · 2024-02-06T17:31:12Z

widget/entry.go

 	// Convert column, row into x,y
 	getCoordinates := func(column int, row int) (float32, float32) {
 		sz := provider.lineSizeToColumn(column, row)
-		return sz.Width, sz.Height*float32(row) - theme.InputBorderSize() + theme.InnerPadding()
+		szpe := preedit.lineSizeToColumn(column, row)
+		return float32(math.Max(float64(sz.Width), float64(r.cursor.Position().X+szpe.Width))), sz.Height*float32(row) - theme.InputBorderSize() + theme.InnerPadding()


Are you sure this is correct? Isn't the pre-edit only on the current cursor line? This seems to add the "szpe" irrespective of the row being measured...

@andydotxyz OK, Removed.

…LE is set

kanryu · 2024-02-14T02:47:36Z

Full input is now available on Mac.

qizhanchan · 2024-04-06T04:34:54Z

I cloned the latest branch and run the demo, the entry's 'select all' focus effect is confusing.

Jacalz · 2024-04-06T09:22:15Z

@kanryu If you are experiencing a bug on develop, report it as an issue instead of as a comment on a PR please. Thanks.

kanryu · 2024-04-06T15:12:37Z

I cloned the latest branch and run the demo, the entry's 'select all' focus effect is confusing.

@qizhanchan This issue is not caused by this PR's fix, but also occurs with the original Fyne Entry. So it's a Fyne problem.

kanryu · 2024-04-06T15:16:04Z

If you are experiencing a bug on develop, report it as an issue instead of as a comment on a PR please. Thanks.

@Jacalz Which problem in this PR are you pointing out? This PR involves a very large source code modification.

Jacalz · 2024-04-06T15:25:23Z

Ah, sorry @kanryu. I meant to be tagging @qizhanchan in my message. I assumed that he was referring to a bug on the develop branch.

andydotxyz

Sorry it has taken so long to get back on this.

I can't figure out how to test this - is there info about the versions of GLFW etc missing form go.mod?

andydotxyz · 2024-04-08T09:49:47Z

internal/driver/glfw/window_desktop.go

@@ -765,11 +807,15 @@ func (w *window) create() {
 		win.SetRefreshCallback(w.refresh)
 		win.SetContentScaleCallback(w.scaled)
 		win.SetCursorPosCallback(w.mouseMoved)
+		win.SetPreeditCallback(w.preedit)


This won't compile for me. I guess the go.mod is missing some alterations?

andydotxyz · 2024-04-08T09:51:43Z

internal/driver/glfw/loop_desktop.go

@@ -11,8 +11,11 @@ import (
 	"github.com/go-gl/glfw/v3.3/glfw"
 )

+const GLFW_X11_ONTHESPOT glfw.Hint = 0x00052002


Won't this be declared in the GLFW project?
I guess it relates to my other comment about not being sure of how to compile or which GLFW version is needed

@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.

@kanryu If this is under active development but not completed, please mark the PR as draft.

@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.
Other than that, I think this PR is of a quality worthy of being tested by many people.

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.

@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.

@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.

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.

andydotxyz · 2024-04-23T14:59:38Z

Converting to draft as this is not in a state where we would be reviewing for merge.

dweymouth and others added 6 commits November 8, 2023 17:44

add myself to github sponsors

8955049

Merge branch 'release/v2.4.x'

dc34dad

Merge branch 'master' of github.com:fyne-io/fyne

e572ca8

Fix merge glitch

a09bbd7

add myself to github sponsors

0ace386

Support IME of first step

3864d4e

andydotxyz requested changes Feb 6, 2024

View reviewed changes

utt11 and others added 6 commits February 7, 2024 15:57

interface cast assuming form errors

85a122a

PreeditCandidate

77bc7ee

IME windows can now be displayed accurately on Windows OS.

48b1eed

Specifying the correct IME Candidate Window coordinates when FYNE_SCA…

a641ac2

…LE is set

Correctly supports multiple IME inputs

5401a91

Confirmed to work perfectly on Mac

059c417

Jacalz changed the title ~~IME support for 1.5 billion users~~ Add IME support Feb 14, 2024

support IME for linux(X11)

5c65f42

andydotxyz reviewed Apr 8, 2024

View reviewed changes

andydotxyz marked this pull request as draft April 23, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IME support #4614

Add IME support #4614

kanryu commented Feb 6, 2024 •

edited

Loading

andydotxyz left a comment

andydotxyz Feb 6, 2024

kanryu Feb 6, 2024

andydotxyz Feb 6, 2024

kanryu Feb 6, 2024 •

edited

Loading

andydotxyz Feb 7, 2024

andydotxyz Feb 6, 2024

andydotxyz Feb 6, 2024

kanryu Feb 14, 2024

andydotxyz Feb 6, 2024

kanryu Feb 6, 2024

andydotxyz Feb 7, 2024

andydotxyz Feb 6, 2024

kanryu Feb 14, 2024

andydotxyz Feb 6, 2024

kanryu Feb 14, 2024

andydotxyz Feb 6, 2024

kanryu Feb 14, 2024

kanryu commented Feb 14, 2024

qizhanchan commented Apr 6, 2024 •

edited

Loading

Jacalz commented Apr 6, 2024

kanryu commented Apr 6, 2024 •

edited

Loading

kanryu commented Apr 6, 2024

Jacalz commented Apr 6, 2024

andydotxyz left a comment •

edited

Loading

andydotxyz Apr 8, 2024

andydotxyz Apr 8, 2024

kanryu Apr 8, 2024

Jacalz Apr 8, 2024 •

edited

Loading

kanryu Apr 8, 2024

Jacalz Apr 8, 2024 •

edited

Loading

andydotxyz Apr 8, 2024

andydotxyz commented Apr 23, 2024

Add IME support #4614

Are you sure you want to change the base?

Add IME support #4614

Conversation

kanryu commented Feb 6, 2024 • edited Loading

Description:

Supported environments

How to try this

Checklist:

Where applicable:

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanryu Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanryu commented Feb 14, 2024

qizhanchan commented Apr 6, 2024 • edited Loading

Jacalz commented Apr 6, 2024

kanryu commented Apr 6, 2024 • edited Loading

kanryu commented Apr 6, 2024

Jacalz commented Apr 6, 2024

andydotxyz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Apr 23, 2024

kanryu commented Feb 6, 2024 •

edited

Loading

kanryu Feb 6, 2024 •

edited

Loading

qizhanchan commented Apr 6, 2024 •

edited

Loading

kanryu commented Apr 6, 2024 •

edited

Loading

andydotxyz left a comment •

edited

Loading

Jacalz Apr 8, 2024 •

edited

Loading

Jacalz Apr 8, 2024 •

edited

Loading