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

fix #1350349 cannot input chinese characters when adding text #200

Merged
merged 1 commit into from Nov 24, 2021

Conversation

iangzh
Copy link
Contributor

@iangzh iangzh commented Nov 22, 2021

fix #1350349 cannot input chinese characters when adding text
While we focus on the Canvas, the input method will not be able to changed, and imContext itself supports changing input method. So we need to grab focus for imContext, then make sure imContext can change input method.
While making imContext grabs focus, we also need to give right time to RedrawText, if RedrawText at a not proper time, the input method will chnage but the result of inputing Text will give a unacceptable performance.
So this commit will grab focus for imContext and RedrawText at right time.

Copy link
Member

@cameronwhite cameronwhite left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I've left a couple questions and suggestions in the comments.

Pinta.Tools/Tools/TextTool.cs Show resolved Hide resolved
Pinta.Tools/Tools/TextTool.cs Outdated Show resolved Hide resolved
RedrawText (true, true);
}
} else {
//if (keyHandled)
Copy link
Member

Choose a reason for hiding this comment

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

When pressing and holding down an arrow key to move the cursor, we should be redrawing here so that the user can see how far the cursor is moving.
Maybe this code should distinguish between a key being handled by the imcontext (TryHandleChar()) versus the standard navigation keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, but I have checked that while distinguishing this code will give the ability of input method changing which is more important for users.
And it needs to be focus on is that in TextTool there is no need to redrawtext while holding the arrow key, because it is not BrushTool which needs to redraw the mouse trails, it as TextTool dose not need to show the mouse(cursor) trails, and it will redraw the cursor position and the text while the arrow key down at other position in the canvas.

Copy link
Member

Choose a reason for hiding this comment

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

When the user presses and holds the arrow key to move by several characters, this generates multiple OnKeyDown events for the arrow key before the key is released and the OnKeyUp event occurs. I think the user would expect to see the cursor redraw on each key down event so that they know when to release the key.

Does it cause any issues with imcontext input if this is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we hope if we holds a "w" key, it will show every "w" write on the canvas. We press once "w", it write once "w". And now distinguishing thi code, we holds a "w" key, we will not see the all "w" been drawn, and when releasing the "w" key we will see all "w" we pressed have been drawn on the canvas.
I know it is difficult to choose which is a good way to make TextTool perfect, but we see there have 3 functions, which is 1. press a key once 2. hold a key 3. change the input method, the first function is ok anywhere, the second one and third one have conflicts.
Holding a key but not draw its process can be acceptable when comparing it to disability of changing input method.
And it causes no more other issues with imcontext when I checked the functionality of TextTool.

Copy link
Member

Choose a reason for hiding this comment

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

How does the redrawing interfere with the imcontext? Can we at least redraw when certain keys (e.g. arrow keys) are pressed, or does that also interfere?
I agree that it's important to be able to use the input methods, but we also shouldn't introduce regressions if it can be avoided.

@iangzh
Copy link
Contributor Author

iangzh commented Nov 23, 2021

I have changed the code, pls review again, thank you.

@iangzh
Copy link
Contributor Author

iangzh commented Nov 24, 2021

Hello,cameronwhite.
You can see me new commit, it seems well solved our worries. It seems work well in my computer.

@cameronwhite
Copy link
Member

Thanks! This works much better 👍
The only issue I've noticed is that the cursor line remains visible after pressing Esc to finish editing. I'm wondering if the redraw in OnKeyUp() can just be removed now that we're back to redrawing on key down? Removing it seems to fix the issue in my testing

@iangzh
Copy link
Contributor Author

iangzh commented Nov 24, 2021

Hi, cameronwhite.
I have removed the the redraw in OnKeyUp(), it seems OK. Pls help to check it.

Copy link
Member

@cameronwhite cameronwhite left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much for working on this!

@cameronwhite cameronwhite merged commit 49a4f16 into PintaProject:master Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants