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

TextInput: IME support for textinput #7009

Merged
merged 6 commits into from Aug 9, 2020
Merged

Conversation

sanderland
Copy link
Contributor

See #6898
One bug persists, which is pressing an arrow key in the middle of a composition, which causes misplaced characters.
However this is unusual, desktop only, currently present in kivy as a bug, and seems to be caused by SDL.

@sanderland sanderland marked this pull request as ready for review July 20, 2020 17:30
Copy link
Member

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

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

seems mostly fine to me, ideally i'd like to see tests for it, calling window_on_textedit with known values and checking the resulting text is there should be enough. Seems no documentation change is needed, as users don't have to do anything to benefit from the change, so all fine on that side.

Code is clean, short and readable, so props for that.


def _bind_keyboard(self):
super()._bind_keyboard()
Window.bind(on_textedit=self.window_on_textedit)
Copy link
Member

Choose a reason for hiding this comment

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

hm, maybe this should be at the same place that binds window_on_textinput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried putting it in focus behavior for a bit, but it seems out of place somehow. I am not sure what else uses that superclass though

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at FocusBehavior, it's a keyboard instance that dispatch on_key_* and on_textinput events, so on_textedit should be added to Keyboard? Then you bind to on_textedit on keyboard instance and not to Window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but keyboards don't do IME events, it's the window/OS.

== self._imo_composition
): # should always be true
remove_old_imo_text = (
text[:pcc - len(self._imo_composition)] + text[pcc:]
Copy link
Member

Choose a reason for hiding this comment

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

since you reuse that value, and the expression makes lines too long, it could be worth declaring it as len_imo or something like this before the test.

@@ -2585,6 +2587,55 @@ def keyboard_on_textinput(self, window, text):
self.delete_selection()
self.insert_text(text, False)

# current imo composition in progress by the IME system, or '' if nothing
_imo_composition = StringProperty("")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove "" as empty string is a default value for StringProperty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer being explicit over implicit here.

@sanderland sanderland changed the title IMO support for textinput IME support for textinput Jul 21, 2020
@sanderland
Copy link
Contributor Author

Added a test, which caught the _lines=[] case. Please check if it's handled correctly.

@tshirtman tshirtman merged commit 5c8cb1f into kivy:master Aug 9, 2020
@matham matham added this to the 2.0.0 milestone Oct 28, 2020
@matham matham changed the title IME support for textinput TextInput: IME support for textinput Dec 8, 2020
@matham matham added the Component: Widgets kivy/uix, style.kv label Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Widgets kivy/uix, style.kv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants