Skip to content

Commit

Permalink
Fix Korean text input (JetBrains#406)
Browse files Browse the repository at this point in the history
Fixes JetBrains#2600

1. We shouldn't skip input events with empty text, they will clear the current uncommitted text. In case of Korean, Swing will send a clear event of the previous char when we press Space, and send a separate "KEY_TYPED" event for the last character:
- press q -> q uncommitted
- press w -> commit q, w uncommitted
- press Space -> discard committed text (event with empty text), send KEY_TYPED event with w and KEY_TYPED event with Space

2. Add tests for test input. I traced Swing events on each OS to mimic integration tests (It is difficult to write real input tests)
  • Loading branch information
igordmn authored Feb 23, 2023
1 parent ea93bb3 commit 4d58d74
Show file tree
Hide file tree
Showing 6 changed files with 745 additions and 61 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -331,17 +331,14 @@ internal class ComposeLayer(
_component.addInputMethodListener(object : InputMethodListener {
override fun caretPositionChanged(event: InputMethodEvent?) {
if (isDisposed) return
if (event != null) {
catchExceptions {
platform.textInputService.onInputEvent(event)
}
}
// Which OSes and which input method could produce such events? We need to have some
// specific cases in mind before implementing this
}

override fun inputMethodTextChanged(event: InputMethodEvent) {
if (isDisposed) return
catchExceptions {
platform.textInputService.onInputEvent(event)
platform.textInputService.inputMethodTextChanged(event)
}
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,47 +106,25 @@ internal class PlatformInput(private val component: PlatformComponent) :
}
}

fun onInputEvent(event: InputMethodEvent) {
fun inputMethodTextChanged(event: InputMethodEvent) {
if (!event.isConsumed) {
when (event.id) {
InputMethodEvent.INPUT_METHOD_TEXT_CHANGED -> {
replaceInputMethodText(event)
event.consume()
}
InputMethodEvent.CARET_POSITION_CHANGED -> {
inputMethodCaretPositionChanged(event)
event.consume()
}
}
replaceInputMethodText(event)
event.consume()
}
}

private fun inputMethodCaretPositionChanged(
@Suppress("UNUSED_PARAMETER") event: InputMethodEvent
) {
// Which OSes and which input method could produce such events? We need to have some
// specific cases in mind before implementing this
}

private fun replaceInputMethodText(event: InputMethodEvent) {
currentInput?.let { input ->
if (event.text == null) {
return
}
val committed = event.text.toStringUntil(event.committedCharacterCount)
val composing = event.text.toStringFrom(event.committedCharacterCount)
val committed = event.text?.toStringUntil(event.committedCharacterCount).orEmpty()
val composing = event.text?.toStringFrom(event.committedCharacterCount).orEmpty()
val ops = mutableListOf<EditCommand>()

if (needToDeletePreviousChar && isMac && input.value.selection.min > 0 && composing.isEmpty()) {
needToDeletePreviousChar = false
ops.add(DeleteSurroundingTextInCodePointsCommand(1, 0))
}

// newCursorPosition == 1 leads to effectively ignoring of this parameter in EditCommands
// processing. the cursor will be set after the inserted text.
if (committed.isNotEmpty()) {
ops.add(CommitTextCommand(committed, 1))
}
ops.add(CommitTextCommand(committed, 1))
if (composing.isNotEmpty()) {
ops.add(SetComposingTextCommand(composing, 1))
}
Expand Down
48 changes: 28 additions & 20 deletions ui/ui/src/desktopTest/kotlin/androidx/compose/ui/TestUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -22,34 +22,21 @@ import androidx.compose.ui.graphics.ImageBitmap
import androidx.compose.ui.graphics.Paint
import androidx.compose.ui.graphics.painter.BitmapPainter
import androidx.compose.ui.graphics.painter.Painter
import java.awt.AWTEvent
import java.awt.Component
import java.awt.Container
import java.awt.EventQueue
import java.awt.Image
import java.awt.Toolkit
import java.awt.Window
import java.awt.event.InvocationEvent
import java.awt.event.InputMethodEvent
import java.awt.event.KeyEvent
import java.awt.event.MouseEvent
import java.awt.event.MouseWheelEvent
import java.awt.font.TextHitInfo
import java.awt.image.BufferedImage
import java.awt.image.MultiResolutionImage
import java.lang.reflect.InvocationTargetException
import java.lang.reflect.Method
import java.util.Objects
import java.util.concurrent.atomic.AtomicReference
import java.text.AttributedString
import javax.swing.Icon
import javax.swing.ImageIcon
import javax.swing.JFrame
import javax.swing.SwingUtilities
import kotlin.coroutines.resume
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlinx.coroutines.yield
import org.jetbrains.annotations.NonNls
import org.jetbrains.annotations.TestOnly
import org.jetbrains.skiko.MainUIDispatcher

fun testImage(color: Color): Painter = run {
val bitmap = ImageBitmap(100, 100)
Expand All @@ -72,19 +59,40 @@ internal val isMacOs = os.startsWith("mac")

fun Window.sendKeyEvent(
code: Int,
char: Char = code.toChar(),
id: Int = KeyEvent.KEY_PRESSED,
location: Int = KeyEvent.KEY_LOCATION_STANDARD,
modifiers: Int = 0
): Boolean {
val event = KeyEvent(
// if we would just use `focusOwner` then it will be null if the window is minimized
mostRecentFocusOwner,
KeyEvent.KEY_PRESSED,
id,
0,
modifiers,
code,
code.toChar(),
KeyEvent.KEY_LOCATION_STANDARD
char,
location
)
mostRecentFocusOwner!!.dispatchEvent(event)
return event.isConsumed
}

fun Window.sendInputEvent(
text: String?,
committedCharacterCount: Int,
): Boolean {
val event = InputMethodEvent(
// if we would just use `focusOwner` then it will be null if the window is minimized
mostRecentFocusOwner,
InputMethodEvent.INPUT_METHOD_TEXT_CHANGED,
0,
text?.let(::AttributedString)?.iterator,
committedCharacterCount,
TextHitInfo.leading(0),
TextHitInfo.leading(0)
)
dispatchEvent(event)
mostRecentFocusOwner!!.dispatchEvent(event)
return event.isConsumed
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class DesktopInputComponentTest {

val familyEmoji = "\uD83D\uDC68\u200D\uD83D\uDC69\u200D\uD83D\uDC66\u200D\uD83D\uDC66"

input.onInputEvent(
input.inputMethodTextChanged(
InputMethodEvent(
DummyComponent,
InputMethodEvent.INPUT_METHOD_TEXT_CHANGED,
Expand Down Expand Up @@ -104,7 +104,7 @@ class DesktopInputComponentTest {
component.enabledInput!!.getSelectedText(null)
input.charKeyPressed = false

input.onInputEvent(
input.inputMethodTextChanged(
InputMethodEvent(
DummyComponent,
InputMethodEvent.INPUT_METHOD_TEXT_CHANGED,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import kotlinx.coroutines.yield
import org.jetbrains.skiko.MainUIDispatcher
import org.junit.Assume.assumeFalse

@OptIn(ExperimentalCoroutinesApi::class)
internal fun runApplicationTest(
/**
* Use delay(500) additionally to `yield` in `await*` functions
Expand All @@ -45,9 +44,15 @@ internal fun runApplicationTest(
* (non-flaky).
*
* We have to use `useDelay` in some Linux Tests, because Linux can behave in
* non-deterministic way when we change position/size very fast (see the snippet below)
* non-deterministic way when we change position/size very fast (see the snippet below).
*/
useDelay: Boolean = false,
// TODO ui-test solved this issue by passing InfiniteAnimationPolicy to CoroutineContext. Do the same way here
/**
* Hint for `awaitIdle` that the content contains animations (ProgressBar, TextField cursor, etc).
* In this case, we use `delay` instead of waiting for state changes to end.
*/
hasAnimations: Boolean = false,
timeoutMillis: Long = 30000,
body: suspend WindowTestScope.() -> Unit
) {
Expand All @@ -57,7 +62,7 @@ internal fun runApplicationTest(
withTimeout(timeoutMillis) {
val exceptionHandler = TestExceptionHandler()
withExceptionHandler(exceptionHandler) {
val scope = WindowTestScope(this, useDelay, exceptionHandler)
val scope = WindowTestScope(this, useDelay, hasAnimations, exceptionHandler)
scope.body()
scope.exitTestApplication()
}
Expand Down Expand Up @@ -100,6 +105,7 @@ internal class TestExceptionHandler : Thread.UncaughtExceptionHandler {
internal class WindowTestScope(
private val scope: CoroutineScope,
private val useDelay: Boolean,
private val hasAnimations: Boolean,
private val exceptionHandler: TestExceptionHandler
) : CoroutineScope by CoroutineScope(scope.coroutineContext + Job()) {
var isOpen by mutableStateOf(true)
Expand Down Expand Up @@ -132,8 +138,13 @@ internal class WindowTestScope(
awaitEDT()

Snapshot.sendApplyNotifications()
for (recomposerInfo in Recomposer.runningRecomposers.value - initialRecomposers) {
recomposerInfo.state.takeWhile { it > Recomposer.State.Idle }.collect()

if (hasAnimations) {
delay(500)
} else {
for (recomposerInfo in Recomposer.runningRecomposers.value - initialRecomposers) {
recomposerInfo.state.takeWhile { it > Recomposer.State.Idle }.collect()
}
}

exceptionHandler.throwIfCaught()
Expand Down
Loading

0 comments on commit 4d58d74

Please sign in to comment.