-
Notifications
You must be signed in to change notification settings - Fork 70
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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)) { | ||
when (e.cause) { | ||
FocusEvent.Cause.TRAVERSAL_FORWARD -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is moved to |
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This particular check fixes JetBrains/compose-multiplatform#4919 |
||
focusManager.takeFocus(FocusDirection.Next) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} | ||
else -> Unit | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -156,6 +156,19 @@ public fun <T : Component> SwingPanel( | |
} | ||
} | ||
|
||
/** | ||
* true, if the event is handled by SwingPanel. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Returns whether the event is handled by SwingPanel" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
* | ||
|
@@ -194,67 +207,77 @@ private class FocusSwitcher<T : Component>( | |
private val interopComponent: SwingInteropComponent<T>, | ||
private val focusManager: FocusManager, | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 A thing I would extract is |
||
private val backwardRequester = FocusRequester() | ||
private val forwardRequester = FocusRequester() | ||
private var isRequesting = false | ||
private val backwardTracker = FocusTracker { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New features of
|
||
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() | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.