-
Notifications
You must be signed in to change notification settings - Fork 81
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
Touches forwarding to underlying interop views on iOS #1426
Touches forwarding to underlying interop views on iOS #1426
Conversation
...ui/src/skikoMain/kotlin/androidx/compose/ui/input/pointer/InteropViewCatchPointerModifier.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/viewinterop/InteropView.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/viewinterop/InteropView.skiko.kt
Show resolved
Hide resolved
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.
LGTM 👍 Just a few docs nitpicks
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/viewinterop/InteropView.skiko.kt
Outdated
Show resolved
Hide resolved
*/ | ||
@InternalComposeUiApi | ||
fun Modifier.interopView(view: InteropView): Modifier = |
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.
The name is too generic. I assume it will show up in suggestions for unrelated cases
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.
Any suggestions? I guess it's exactly what it is, an InteropView
association for a given element.
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.
modifier is no a VIEW. So at least something like interopViewAnchor
.
Wait. We don't need this function in public at all - let's just keep a node. (InteropViewAnchorModifierNode)
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/interop/UIKitView.uikit.kt
Outdated
Show resolved
Hide resolved
private var updateTouchesCount: (count: Int) -> Unit, | ||
private var hitTestInteropView: (point: CValue<CGPoint>, event: UIEvent?) -> InteropView?, | ||
private var onTouchesEvent: (view: UIView, event: UIEvent, phase: CupertinoTouchesPhase) -> Unit, | ||
private var onTouchesCountChange: (count: Int) -> Unit, | ||
private var inBounds: (CValue<CGPoint>) -> Boolean, |
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.
I would consider some simplification of the API here:
- Move all calls to delegate-like interface.
- Omit
inBounds
, and movehitTest
logic out to the class to its owner.
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.
I removed delegate specifically to avoid instantiation of objects that capture this
.
Due to heavy coupling of dependencies in ComposeSceneMediator
it's already a problem to figure out what's the order of lazy
instantiations and I've tried this approach before I efficiently just made this. Delegates implementation required wrapping coupled objects access into lambda to avoid lazy-reentry, which just makes things way too complicated. But I agree with the second point, we can make those things easier. Let's do it in a separate PR?
@@ -87,7 +87,7 @@ internal class IntermediateTextInputUIView( | |||
hideEditMenu() | |||
} | |||
} | |||
var keyboardEventHandler: KeyboardEventHandler? = null | |||
var onPresses: (Set<*>) -> Unit = NoOpOnPresses |
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.
Are presses can be done from another source, different from the attached keyboard?
I would keep keyboard
naming part in API to have more context when reading the code.
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, hardware keyboard too, afaik.
Anyway, a good point, will add it.
I implied, that simple touches forwarding is needed, but some views like MKMapView and UITextField use internal gesture recognisers, so we have to use one too. |
Screen.Recording.2024-07-03.at.12.04.39.movNot ideal, but it works for now |
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.
LGTM, let's just fine tune the naming
*/ | ||
@InternalComposeUiApi | ||
fun Modifier.interopView(view: InteropView): Modifier = |
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.
modifier is no a VIEW. So at least something like interopViewAnchor
.
Wait. We don't need this function in public at all - let's just keep a node. (InteropViewAnchorModifierNode)
@@ -18,6 +18,7 @@ package androidx.compose.ui.viewinterop | |||
|
|||
import androidx.compose.runtime.ComposeNodeLifecycleCallback | |||
|
|||
// TODO: implement this for iOS |
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.
Please add a link to this task
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/interop/UIKitView.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/InteractionUIView.uikit.kt
Outdated
Show resolved
Hide resolved
3dab05d
to
66f99e1
Compare
…/UIKitView.uikit.kt Co-authored-by: Ivan Matkov <[email protected]>
…that could be unchanged)
This reverts commit f2777cb
28dca12
to
c324419
Compare
First approach to allow touches to be processed by both Compose and nested interop views. Other UX improvements will follow. 1. `actual typealias InteropView = UIView` 2. Change `ComposeScene` API to allow hit testing specific `InteropView`. 3. Make `InteractionUIView` a `container` view of `InteropContainer` to enforce it being in the same responder chain as interop views. 4. Remove `InteractionUIView.Delegate` and replace it with callbacks to avoid `by lazy` reentry in `ComposeSceneMediator`, make `InteractionUIView` construction eager. 5. Use `event.allTouches` instead of `event.touchesForView` to avoid receiving an empty list in case `InteractionUIView` is part of responder chain but not the hit test result itself. 6. Remove `KeyboardEventHandler.uikit.kt` to avoid `by lazy` reentry, stick everything in a single lambda. Fixes some of the cases from the domain of JetBrains/compose-multiplatform#4818 `Demo/IosBugs/UIKitRenderSync` now properly registers touches to allow LazyColumn scrolling and native view reaction. - Touches inside interop views are not exclusive to them and are processed on Compose side as well. - ComposeScene `fun hitTestInteropView(position: Offset): Boolean` changed to `fun hitTestInteropView(position: Offset): InteropView?` --------- Co-authored-by: Ivan Matkov <[email protected]>
First approach to allow touches to be processed by both Compose and nested interop views.
Other UX improvements will follow.
actual typealias InteropView = UIView
ComposeScene
API to allow hit testing specificInteropView
.InteractionUIView
acontainer
view ofInteropContainer
to enforce it being in the same responder chain as interop views.InteractionUIView.Delegate
and replace it with callbacks to avoidby lazy
reentry inComposeSceneMediator
, makeInteractionUIView
construction eager.event.allTouches
instead ofevent.touchesForView
to avoid receiving an empty list in caseInteractionUIView
is part of responder chain but not the hit test result itself.KeyboardEventHandler.uikit.kt
to avoidby lazy
reentry, stick everything in a single lambda.Fixes some of the cases from the domain of JetBrains/compose-multiplatform#4818
Testing
Demo/IosBugs/UIKitRenderSync
now properly registers touches to allow LazyColumn scrolling and native view reaction.Release Notes
iOS - Fixes
Multiple Platforms - Breaking changes
fun hitTestInteropView(position: Offset): Boolean
changed tofun hitTestInteropView(position: Offset): InteropView?