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

Fix topLeftOffset calculation on iOS in Split View #678

Merged

Conversation

paxbun
Copy link

@paxbun paxbun commented Jul 16, 2023

Proposed Changes

  • Made ComposeWindow.getTopLeftOffset return the offset between ComposeWindow and the current window, not the current screen, following Skiko's behavior. Before this change, when the application was in the Split View or a Slide Over window (especially when the window is on the right), Composables were not responding to touch events. Also, when the application was in a Slide Over window on the left, Composables slightly off (on the left upper part) from the touch position were responding.

Testing

Test: Tested using the Android & iOS template on the following devices:

  • (Physical device) iPad Pro, 11-inch (3rd generation) (iOS 16.5.1)
  • (Simulator) iPad Pro, 12.9-inch (5th generation) (iOS 16.0)
  • (Simulator) iPad Pro, 12.9-inch (6th generation) (iOS 16.4)

App.kt is modified as follows:

@Composable
fun App() {
    MaterialTheme {
        val tapPositions = remember { mutableStateListOf<Offset>() }
        val canvasPaint = remember { Paint().apply { color = Color.Black } }
        Column {
            Button(onClick = tapPositions::clear) {
                Text("Clear")
            }
            Box(Modifier.fillMaxSize().pointerInput(tapPositions) {
                detectTapGestures(onPress = {
                    tapPositions.add(it)
                })
            }) {
                Canvas(Modifier.fillMaxSize()) {
                    tapPositions.forEach { tapPosition ->
                        drawContext.canvas.drawCircle(tapPosition, 10.0f, canvasPaint)
                    }
                }
            }
        }
    }
}

ContentView.swift is modified as follows to ensure that this still works even when ComposeWindow is inserted as a subview.

struct ContentView: View {
    var body: some View {
        VStack {
            ZStack {
                Color.gray
                Text("SwiftUI padding")
            }.frame(height: 50.0)
            HStack {
                Color.red.frame(width: 20.0)
                ComposeView()
                    .ignoresSafeArea(.all, edges: .bottom) // Compose has own keyboard handler
            }
        }
    }
}

Issues Fixed

Fixes: JetBrains/compose-multiplatform#3100

Run Result

Before the fix (1.4.1):

Before.Physical.Device.mov

After the fix:

After.Physical.Device.mov

Behavior Differences of the Simulator and Physical Devices

The behavior of the simulator and physical devices are different when the application is in the right Split View. On the simulator, the origin of UIScreen.mainScreen's coordinate space is at the top center of the screen, while on physical devices, the origin is at the top left corner of the screen. Here's the experiment result with modified getTopLeftOffset as follows:

    private fun getTopLeftOffset(): Offset {
        val topLeftPoint =
            view.coordinateSpace().convertPoint(
                point = CGPointMake(0.0, 0.0),
                toCoordinateSpace = UIScreen.mainScreen.coordinateSpace()
            )
        return topLeftPoint.useContents {
            val offset = DpOffset(x.dp, y.dp).toOffset(density)
            println("getTopLeftOffset = $offset")
            offset
        }
    }

Physical Device:

Physical.Device.Split.RIght.mp4

Simulator:

Simulator.Split.Right.mov

Due to this, even without this fix, the app responds to touch events normally when on the right Split View on the simulator, while not on physical devices.

Fixed ComposeWindow.getTopLeftOffset returning the wrong offset when
the application is in Split View or a Slide Over window.
@paxbun paxbun marked this pull request as ready for review July 16, 2023 23:43
@paxbun
Copy link
Author

paxbun commented Jul 17, 2023

I want to make unit tests for this, but it seems that only benchmark has XCTest integration and there is no reliable way to run the app on a Split View or a Slide Over window with XCTest.

@elijah-semyonov
Copy link

elijah-semyonov commented Jul 17, 2023

Thanks for PR, we are currently investigating whether we need to process touches in a coordinate space different from UIView space at all, so this logic could be redundant.

@paxbun
Copy link
Author

paxbun commented Jul 17, 2023

@elijah-semyonov That's okay, would the coordinate space fix be included in 1.5.0?

@elijah-semyonov elijah-semyonov merged commit 82dc1f8 into JetBrains:jb-main Jul 17, 2023
@elijah-semyonov
Copy link

elijah-semyonov commented Jul 17, 2023

@paxbun Merged it. It will be included in 1.5.0 as it is, or we will ditch coordinate space logic altogether, as I've mentioned earlier.
Either way, your use case will work properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants