fix: manually enable device orientation notifications #1543
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As docs (https://developer.apple.com/documentation/uikit/uidevice/1620041-begingeneratingdeviceorientation?language=objc) say: call to this method is necessary to receive correct values from call to [[UIDevice currentDevice] orientation]. However, even w/o this fix, it seems to me that call to orienation methods returns correct value...
kkafar
changed the title
1 task
kkafar
added a commit
that referenced
this pull request
Nov 18, 2022
## Description UIKit documentation states that in order to successfully use orientation in iOS apps the orientation has to be enabled via [the `beginGeneratingDeviceOrientationNotifications ` method](https://developer.apple.com/documentation/uikit/uidevice/1620041-begingeneratingdeviceorientation?language=objc). This was addressed in #1543. This PR attempts to make this change internal and also removing an additional `react-native-screens` installation step from README. PR made based on code from `expo-screen-orientation` package [from this line](https://github.com/expo/expo/blob/519ac3d86864da9dd920a9ff8d0c880f6ec81a58/ios/versioned/sdk46/EXScreenOrientation/EXScreenOrientation/ABI46_0_0EXScreenOrientationRegistry.m#L38). Fixes react-navigation/react-navigation.github.io#1177 Note by @kkafar: For now I went with placing the code in ctor/dtor of `RNSScreen` view manager. The view manager construction & lifecycle is managed by React Native's internals (and is executed during application startup). This behaviour is considered stable (changing this would be braking change for RN Paper architecture). I considered also: 1. Creating a native module, so we can execute required code in its lifetime (ctor, dtor) methods -- but it seemed too much... Also we would have to load this module from JS (prevent lazy loading). 2. Using `dispatch_once` in `+ (voi)enforceDesiredDeviceOrientation` method - and calling only the `begin...` method (as there would be no place to call the responding `end...` method. 3. Calling `begin...`/`end...` methods every time `enforceDesiredDeviceOrientation` is being called. 4. Adding the code to ctor/dtor of `RNSScreenWindowTraits` and creating a static object. But I had objections. I feared that compiler might optimise-out (remove) this static variable since it is unused. I've found some [mails](https://gcc.gnu.org/pipermail/gcc-help/2021-April/140125.html) form [gcc-help mailing list](https://gcc.gnu.org/mailman/listinfo/gcc-help) indicating that it might be the case. ## Changes - Moved `beginGeneratingDeviceOrientationNotifications` and `endGeneratingDeviceOrientationNotifications` methods from applications `AppDelegate`s to `RNSScreenManager` `init`/`dealloc` methods. ## Testing I attached a breakpoint to see that added code ends actually being called. Orientation playground in `Example/` app. ## Checklist - [x] Ensured that CI passes Co-authored-by: Kacper Kafara <[email protected]>
mccoyplayer
pushed a commit
to mccoyplayer/reactScreen
that referenced
this pull request
Feb 9, 2024
## Description UIKit documentation states that in order to successfully use orientation in iOS apps the orientation has to be enabled via [the `beginGeneratingDeviceOrientationNotifications ` method](https://developer.apple.com/documentation/uikit/uidevice/1620041-begingeneratingdeviceorientation?language=objc). This was addressed in software-mansion/react-native-screens#1543. This PR attempts to make this change internal and also removing an additional `react-native-screens` installation step from README. PR made based on code from `expo-screen-orientation` package [from this line](https://github.com/expo/expo/blob/519ac3d86864da9dd920a9ff8d0c880f6ec81a58/ios/versioned/sdk46/EXScreenOrientation/EXScreenOrientation/ABI46_0_0EXScreenOrientationRegistry.m#L38). Fixes react-navigation/react-navigation.github.io#1177 Note by @kkafar: For now I went with placing the code in ctor/dtor of `RNSScreen` view manager. The view manager construction & lifecycle is managed by React Native's internals (and is executed during application startup). This behaviour is considered stable (changing this would be braking change for RN Paper architecture). I considered also: 1. Creating a native module, so we can execute required code in its lifetime (ctor, dtor) methods -- but it seemed too much... Also we would have to load this module from JS (prevent lazy loading). 2. Using `dispatch_once` in `+ (voi)enforceDesiredDeviceOrientation` method - and calling only the `begin...` method (as there would be no place to call the responding `end...` method. 3. Calling `begin...`/`end...` methods every time `enforceDesiredDeviceOrientation` is being called. 4. Adding the code to ctor/dtor of `RNSScreenWindowTraits` and creating a static object. But I had objections. I feared that compiler might optimise-out (remove) this static variable since it is unused. I've found some [mails](https://gcc.gnu.org/pipermail/gcc-help/2021-April/140125.html) form [gcc-help mailing list](https://gcc.gnu.org/mailman/listinfo/gcc-help) indicating that it might be the case. ## Changes - Moved `beginGeneratingDeviceOrientationNotifications` and `endGeneratingDeviceOrientationNotifications` methods from applications `AppDelegate`s to `RNSScreenManager` `init`/`dealloc` methods. ## Testing I attached a breakpoint to see that added code ends actually being called. Orientation playground in `Example/` app. ## Checklist - [x] Ensured that CI passes Co-authored-by: Kacper Kafara <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
According to the Apple docs a call to ... must be made for
UIDeviceorientationproperty to work correctly.I checked and neither we call
[[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications]nor any dependency does this for us (I looked up in node_modules withgrep --include=\*.{m,mm} -rnw './node_modules/' -e "beginGeneratingDeviceOrientationNotifications"), however it worked properly. I also checked withgeneratesDeviceOrientationNotificationsmethod whether orientation notifications were enabled or not. They were on. This nudges me towards conclusion that it possible that these notifications are enabled in some internal Apple code.However, according to issue #1540 there are some cases when orientation notifications are not enabled "by default", thus we need to manually register for them. As documentation states it is fine to nest calls to the
begin...method as long as we also match it with call to theend...method.Fixes #1540
Changes
Updated
README.mdto include extended installation instructions.I recommended adding calls to
[[UIDevice currentDevice] {begin,end}GeneratingDeviceOrientationNotifications]toAppDelegate.mto lifecycle methods.Updated AppDelegates in our example applications.
Test code and steps to reproduce
e.g.
Test1198-- navigate to second screen, rotate your device to change interface orientation & go back to first screen -- it's orientation should be correctly adjusted.Checklist