Skip to content

feat: Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method - #41935

Closed
okwasniewski wants to merge 2 commits into
react:mainfrom
okwasniewski:feat/optimize-keywindow
Closed

feat: Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method#41935
okwasniewski wants to merge 2 commits into
react:mainfrom
okwasniewski:feat/optimize-keywindow

Conversation

@okwasniewski

@okwasniewskiokwasniewski commented Dec 14, 2023

Copy link
Copy Markdown
Contributor

Summary:

This PR optimises RCTKeyWindow() calls in RCTForceTouchAvailable method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.

Before:

On the video you can see that this function is being called 350 times just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.

rctkeywindowConverted.mp4

After: the function is called only few times at the start of the app to get initial layout measurements.

Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly

Changelog:

[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method

Test Plan:

CI Green

@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 14, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@okwasniewski

Copy link
Copy Markdown
ContributorAuthor

Hey! Is there anything I can do to get this merged? I can't see the internal linter error, unfortunately. cc: @cipolleschi

@cipolleschi

Copy link
Copy Markdown
Contributor

Taking care of it!

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Dec 19, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in 90fb73e.

facebook-github-bot pushed a commit that referenced this pull request Jan 5, 2024
…window apps (#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
## Changelog:
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: #42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 14, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Jan 31, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Jan 31, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Feb 6, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Feb 6, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Apr 5, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Apr 5, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Jun 24, 2024
…window apps (react#42036)
Summary:
This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (react#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.
This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate`
bypass-github-export-checks
[IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps
Pull Request resolved: react#42036
Test Plan:
Checkout RNTester example for Alerts and LoadingView.
https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b
Reviewed By: huntie
Differential Revision: D52431720
Pulled By: cipolleschi
fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b
okwasniewski added a commit to callstack/react-native-visionos that referenced this pull request Jun 24, 2024
…eact#41935)
Summary:
This PR optimises RCTKeyWindow() calls in `RCTForceTouchAvailable` method. This method was calling RCTKeyWindow hundreds of times while scrolling on the screen.
Before:
On the video you can see that this function is being called **350 times** just from simple list scrolling. RCTKeyWindow is looping over app windows so it's not a cheap operation.
https://github.com/facebook/react-native/assets/52801365/5b69cbd6-d148-4d06-b672-bd7b60472c13
After: the function is called only few times at the start of the app to get initial layout measurements.
Solution: I think we can check just once for the force touch capabilities as devices can't change it on the fly
bypass-github-export-checks
## Changelog:
[IOS] [FIXED] - Optimise RCTKeyWindow() calls in RCTForceTouchAvailable method
Pull Request resolved: react#41935
Test Plan: CI Green
Reviewed By: dmytrorykun
Differential Revision: D52172510
Pulled By: cipolleschi
fbshipit-source-id: 881a3125a2af4376ce65d785d8eee09c7d8f1f16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.p: CallstackPartner: CallstackPartnerShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@okwasniewski@facebook-github-bot@cipolleschi