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

Adapt focus interop code to Compose 1.7 changes #1398

Merged
merged 5 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions compose/ui/ui/api/desktop/ui.api
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,7 @@ public final class androidx/compose/ui/ComposeScene {
public final fun getLayoutDirection ()Landroidx/compose/ui/unit/LayoutDirection;
public final fun getRoots ()Ljava/util/Set;
public final fun hasInvalidations ()Z
public final fun moveFocus-3ESFkO8 (I)Z
public final fun releaseFocus ()V
public final fun render (Lorg/jetbrains/skia/Canvas;J)V
public final fun requestFocus ()V
public final fun sendKeyEvent-ZmokQxo (Ljava/lang/Object;)Z
public final fun sendPointerEvent-BGSDPeU (IJJJILandroidx/compose/ui/input/pointer/PointerButtons;Landroidx/compose/ui/input/pointer/PointerKeyboardModifiers;Ljava/lang/Object;Landroidx/compose/ui/input/pointer/PointerButton;)V
public static synthetic fun sendPointerEvent-BGSDPeU$default (Landroidx/compose/ui/ComposeScene;IJJJILandroidx/compose/ui/input/pointer/PointerButtons;Landroidx/compose/ui/input/pointer/PointerKeyboardModifiers;Ljava/lang/Object;Landroidx/compose/ui/input/pointer/PointerButton;ILjava/lang/Object;)V
Expand Down Expand Up @@ -3501,8 +3498,7 @@ public final class androidx/compose/ui/scene/ComposeSceneContext$Companion {

public abstract interface class androidx/compose/ui/scene/ComposeSceneFocusManager : androidx/compose/ui/focus/FocusManager {
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved
public abstract fun getFocusRect ()Landroidx/compose/ui/geometry/Rect;
public abstract fun releaseFocus ()V
public abstract fun requestFocus ()V
public abstract fun getHasFocus ()Z
}

public abstract interface class androidx/compose/ui/scene/ComposeSceneLayer {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ class ComposePanel @ExperimentalComposeUiApi constructor(
skiaLayerAnalytics = skiaLayerAnalytics,
windowContainer = windowContainer
).apply {
focusManager.releaseFocus()
setBounds(0, 0, width, height)
contentComponent.isFocusable = isFocusable
contentComponent.isRequestFocusEnabled = isRequestFocusEnabled
Expand All @@ -207,19 +206,12 @@ class ComposePanel @ExperimentalComposeUiApi constructor(
_focusListeners.forEach(contentComponent::addFocusListener)
contentComponent.addFocusListener(object : FocusListener {
override fun focusGained(e: FocusEvent) {
// The focus can be switched from the child component inside SwingPanel.
// In that case, SwingPanel will take care of it.
if (!isParentOf(e.oppositeComponent)) {
focusManager.requestFocus()
if (!e.isTemporary && !e.isFocusGainedHandledBySwingPanel(this@ComposePanel)) {
Copy link

Choose a reason for hiding this comment

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

A bit confusing that this is called from ComposePanel (the function is about "SwingPanel"). Is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is ugly, but this was the fastest way to implement SwingPanel back in the days. I didn't change the logic, just added a name. There is no other purpose besides using for SwingPanel.

A good alternative exists, but it needs investigation. I described it in the KDoc of this function.

when (e.cause) {
FocusEvent.Cause.TRAVERSAL_FORWARD -> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is moved to ComposeSceneMediator, as it is valid for all containers using it

focusManager.moveFocus(FocusDirection.Next)
}
FocusEvent.Cause.TRAVERSAL_BACKWARD -> {
focusManager.moveFocus(FocusDirection.Previous)
}
FocusEvent.Cause.UNKNOWN, FocusEvent.Cause.ACTIVATION -> {
focusManager.moveFocus(FocusDirection.Enter)
if (!focusManager.hasFocus) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This particular check fixes JetBrains/compose-multiplatform#4919

focusManager.takeFocus(FocusDirection.Next)
Copy link
Collaborator Author

@igordmn igordmn Jun 10, 2024

Choose a reason for hiding this comment

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

FocusDirection.Enter doesn't work anymore (probably a commonMain bug), but there is no semantic difference with Next here

}
}
else -> Unit
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public fun <T : Component> SwingPanel(
DisposableEffect(Unit) {
val focusListener = object : FocusListener {
override fun focusGained(e: FocusEvent) {
if (interopComponent.container.isParentOf(e.oppositeComponent)) {
if (e.isFocusGainedHandledBySwingPanel(interopComponent.container)) {
when (e.cause) {
FocusEvent.Cause.TRAVERSAL_FORWARD -> focusSwitcher.moveForward()
FocusEvent.Cause.TRAVERSAL_BACKWARD -> focusSwitcher.moveBackward()
Expand Down Expand Up @@ -156,6 +156,19 @@ public fun <T : Component> SwingPanel(
}
}

/**
* true, if the event is handled by SwingPanel.
Copy link

Choose a reason for hiding this comment

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

"Returns whether the event is handled by SwingPanel"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

*
* The focus can be switched from the child component inside SwingPanel.
* In that case, SwingPanel will take care of it.
*
* The alternative that needs more investigation is to
* not use ComposePanel as next/previous focus element for SwingPanel children
* (see [SwingPanelContainer.focusComponent])
*/
internal fun FocusEvent.isFocusGainedHandledBySwingPanel(container: Container) =
container.isParentOf(oppositeComponent)

/**
* A container for [SwingPanel]'s component. Takes care about focus and clipping.
*
Expand Down Expand Up @@ -194,67 +207,77 @@ private class FocusSwitcher<T : Component>(
private val interopComponent: SwingInteropComponent<T>,
private val focusManager: FocusManager,
) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to extract this class into separate file (with name something like SwingInteropFocusSwitcher) at some point. Not a blocker

Copy link
Collaborator Author

@igordmn igordmn Jun 10, 2024

Choose a reason for hiding this comment

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

I wouldn't move it out of this file, until the file size is manageable (<1000 lines). Because this logic is strictly bound to SwingPanel.

A thing I would extract is FocusTracker - it might be useful for other platforms. But only when it is needed - there is a chance that other implementations should use the Android way.

private val backwardRequester = FocusRequester()
private val forwardRequester = FocusRequester()
private var isRequesting = false
private val backwardTracker = FocusTracker {
Copy link
Collaborator Author

@igordmn igordmn Jun 10, 2024

Choose a reason for hiding this comment

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

New features of FocusSwitcher:

  • simple refactoring
  • avoiding recursive calls (can happen with empty panels with the new code from the upstream)

val container = interopComponent.container
val component = container.focusTraversalPolicy.getFirstComponent(container)
if (component != null) {
component.requestFocus(FocusEvent.Cause.TRAVERSAL_FORWARD)
} else {
moveForward()
}
}

fun moveBackward() {
try {
isRequesting = true
backwardRequester.requestFocus()
} finally {
isRequesting = false
private val forwardTracker = FocusTracker {
val component = interopComponent.container.focusTraversalPolicy.getLastComponent(interopComponent.container)
if (component != null) {
component.requestFocus(FocusEvent.Cause.TRAVERSAL_BACKWARD)
} else {
moveBackward()
}
}

fun moveBackward() {
backwardTracker.requestFocusWithoutEvent()
focusManager.moveFocus(FocusDirection.Previous)
}

fun moveForward() {
try {
isRequesting = true
forwardRequester.requestFocus()
} finally {
isRequesting = false
}
forwardTracker.requestFocusWithoutEvent()
focusManager.moveFocus(FocusDirection.Next)
}

@Composable
fun Content() {
EmptyLayout(
Modifier
.focusRequester(backwardRequester)
.onFocusEvent {
if (it.isFocused && !isRequesting) {
focusManager.clearFocus(force = true)

val container = interopComponent.container
val component = container.focusTraversalPolicy.getFirstComponent(container)
if (component != null) {
component.requestFocus(FocusEvent.Cause.TRAVERSAL_FORWARD)
} else {
moveForward()
}
}
}
.focusTarget()
)
EmptyLayout(
Modifier
.focusRequester(forwardRequester)
.onFocusEvent {
if (it.isFocused && !isRequesting) {
focusManager.clearFocus(force = true)

val component = interopComponent.container.focusTraversalPolicy.getLastComponent(interopComponent.container)
if (component != null) {
component.requestFocus(FocusEvent.Cause.TRAVERSAL_BACKWARD)
} else {
moveBackward()
}
EmptyLayout(backwardTracker.modifier)
EmptyLayout(forwardTracker.modifier)
}

/**
* A helper class that can help:
* - to prevent recursive focus events
* (a case when we focus the same element inside `onFocusEvent`)
* - to prevent triggering `onFocusEvent` while requesting focus somewhere else
*/
private class FocusTracker(
private val onNonRecursiveFocused: () -> Unit
) {
private val requester = FocusRequester()

private var isRequestingFocus = false
private var isHandlingFocus = false

fun requestFocusWithoutEvent() {
try {
isRequestingFocus = true
requester.requestFocus()
} finally {
isRequestingFocus = false
}
}

val modifier = Modifier
.focusRequester(requester)
.onFocusEvent {
if (!isRequestingFocus && !isHandlingFocus && it.isFocused) {
try {
isHandlingFocus = true
onNonRecursiveFocused()
} finally {
isHandlingFocus = false
}
}
.focusTarget()
)
}
.focusTarget()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import androidx.compose.ui.awt.AwtEventListener
import androidx.compose.ui.awt.AwtEventListeners
import androidx.compose.ui.awt.OnlyValidPrimaryMouseButtonFilter
import androidx.compose.ui.awt.SwingInteropContainer
import androidx.compose.ui.awt.isFocusGainedHandledBySwingPanel
import androidx.compose.ui.awt.runOnEDTThread
import androidx.compose.ui.focus.FocusDirection
import androidx.compose.ui.focus.FocusManager
Expand Down Expand Up @@ -68,6 +69,9 @@ import java.awt.Toolkit
import java.awt.event.ContainerEvent
import java.awt.event.ContainerListener
import java.awt.event.FocusEvent
import java.awt.event.FocusEvent.Cause.TRAVERSAL
import java.awt.event.FocusEvent.Cause.TRAVERSAL_BACKWARD
import java.awt.event.FocusEvent.Cause.TRAVERSAL_FORWARD
import java.awt.event.FocusListener
import java.awt.event.InputEvent
import java.awt.event.InputMethodEvent
Expand Down Expand Up @@ -215,8 +219,20 @@ internal class ComposeSceneMediator(
// We don't reset focus for Compose when the component loses focus temporary.
// Partially because we don't support restoring focus after clearing it.
// Focus can be lost temporary when another window or popup takes focus.
if (!e.isTemporary) {
scene.focusManager.requestFocus()
if (!e.isTemporary && !e.isFocusGainedHandledBySwingPanel(container)) {
when (e.cause) {
TRAVERSAL_BACKWARD -> {
if (!focusManager.takeFocus(FocusDirection.Previous)) {
platformContext.parentFocusManager.moveFocus(FocusDirection.Previous)
}
}
TRAVERSAL, TRAVERSAL_FORWARD -> {
if (!focusManager.takeFocus(FocusDirection.Next)) {
platformContext.parentFocusManager.moveFocus(FocusDirection.Next)
}
}
else -> Unit
}
}
}

Expand Down Expand Up @@ -651,7 +667,14 @@ internal class ComposeSceneMediator(
}
override val parentFocusManager: FocusManager = DesktopFocusManager()
override fun requestFocus(): Boolean {
return contentComponent.hasFocus() || contentComponent.requestFocusInWindow()
// Don't check hasFocus(), and don't check the returning result
// Swing returns "false" if the window isn't visible or isn't active,
// but the component will always receive the focus after activation.
//
// if we return false - we don't allow changing the focus, and it breaks requesting
// focus at start and in inactive mode
contentComponent.requestFocusInWindow()
return true
}

override val rootForTestListener
Expand Down
Loading
Loading