zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.3k stars 655 forks source link

Replace unmaintained dep `react-native-orientation`. #4118

Closed chrisbobbe closed 4 years ago

chrisbobbe commented 4 years ago

When we upgrade to RN v0.60.0 (that's #3548), we'll get this warning:

warn The following packages use deprecated "rnpm" config that will stop working from next release:
  - react-native-orientation: https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
Please notify their maintainers about it. You can find more details at https://github.com/react-native-community/cli/blob/master/docs/configuration.md#migration-guide.

rn-fetch-blob is easy to fix; we just have to upgrade to v0.11; I'm planning to do that in my PR for #3548.

react-native-orientation is unmaintained, though. The fix for this warning is very simple, but the PR for it has gathered dust, and the repo isn't getting updates.

This will take some assembly in native code, which should all be spelled out in the replacement library's installation instructions.

I recommend trying Expo's ScreenOrientation; Expo libraries are generally frequently maintained, and we can newly use them since #4016 landed. If we're going with that, we'll follow the instructions for a "bare React Native app" (ours isn't "managed" by Expo), and use yarn add expo-screen-orientation instead of npm install. Since Expo uses TypeScript instead of Flow, we'll have to put together a libdef, unless FlowTyped has one (I don't think it does). We have a new doc for that, and it would be nice for that to be updated with anything new we learn along the way.

chrisbobbe commented 4 years ago

Huh, it would seem that we may actually want DeviceMotion, not ScreenOrientation...

From the ScreenOrientation doc:

Screen Orientation is defined as the orientation in which graphics are painted on the device. For example, the figure below has a device in a vertical and horizontal physical orientation, but a portrait screen orientation. For physical device orientation, see the orientation section of Device Motion.

chrisbobbe commented 4 years ago

Huh, it would seem that we may actually want DeviceMotion, not ScreenOrientation...

Ah, no, actually (at least on iOS; haven't tested on Android yet) — when I followed some alternate suggestions for the AppDelegate.m, defaulting to a lock of ALL orientations allowed, it does indeed respond to physical device rotation

And, it works on Android too!

gnprice commented 4 years ago

Cool. I suspect what that means is: "orientation" means how the display is oriented on the device -- which is often controlled by how the device is oriented physically, but may not be (e.g. with "orientation lock" mode.)

So if you really are interested in how the device is oriented physically relative to gravity, you want the other thing -- but we're interested in how the display is oriented on the device.

chrisbobbe commented 4 years ago

but may not be (e.g. with "orientation lock" mode.)

Yeah. What isn't clear in their setup instructions for what to put in AppDelegate.m is that the "default orientation" really seems to mean an orientation you'd like to lock to, and have difficulty changing to a different lock later. So I locked to allowing "all" orientations.

The line in the instructions that threw me off, I think, is this, from here:

or if you want to change the default screen orientation, with:

chrisbobbe commented 4 years ago

Hmm, that's not quite it. That tweak seems to affect the supported device orientations, which makes sense—not "orientations you'd like to lock to and have difficulty changing later". See the Apple doc. (Incidentally, it would be quite easy to support all orientations except upside-down, if we think it's a bit silly to allow the upside-down orientation.)

gnprice commented 4 years ago

Yeah. What isn't clear in their setup instructions for what to put in AppDelegate.m is that the "default orientation" really seems to mean an orientation you'd like to lock to, and have difficulty changing to a different lock later. So I locked to allowing "all" orientations.

The line in the instructions that threw me off, I think, is this

That line does sound pretty explicit!

Have you been trying this empirically? (Seems like the cases of interest are tatus quo, simple/default version of their setup, and version with initWithDefaultScreenOrientationMask:UIInterfaceOrientationMaskAll.) What did you find the behavior turns out to be?

There's this other bit of docs on this Expo module -- though on a different part of its API:

OrientationLock.DEFAULT -- The default orientation. On iOS, this will allow all orientations except Orientation.PORTRAIT_DOWN. On Android, this lets the system decide the best orientation.

chrisbobbe commented 4 years ago

Have you been trying this empirically?

OK, just took another look at this. Turns out there's some unexplained variation (addressed toward the bottom)!

- UIViewController *rootViewController = [UIViewController new];
+ UIViewController *rootViewController = [[EXScreenOrientationViewController alloc] init];

The screen orientation does not respond to the physical device's rotation, and our listener isn't triggered.

- UIViewController *rootViewController = [UIViewController new];
+ UIViewController *rootViewController =  [[EXScreenOrientationViewController alloc] initWithDefaultScreenOrientationMask:UIInterfaceOrientationMaskAll];

The screen orientation responds to the physical device's rotation, and our listener gets triggered. 👍

So, that seems consistent with the Apple doc that says we're specifying the "supported" device orientations.

The "unexplained variation" is in the behavior of ScreenOrientation.lockAsync(orientationLock) for valid values of orientationLock (0-9).

I've set up this interval:

    setInterval(() => {
      ScreenOrientation.lockAsync(Math.floor(Math.random() * 10));
    }, 1000);

and I'm not seeing consistent behavior within any particular one of those three cases. Sometimes it does rotate randomly every second, sometimes it doesn't. I do restart the Xcode build each time (since it's a change to the native code). I've tried clearing the build folder and building again. Sometimes it changes from the unexpected behavior to the expected behavior when I do something like disable remote JS debugging. Sometimes not. Weird.

Anyway, we're not interested in using lockAsync, and we see the behavior we want, in that third case. So I don't think I necessarily need to pin this down further; what do you think, @gnprice?

gnprice commented 4 years ago

Anyway, we're not interested in using lockAsync, and we see the behavior we want, in that third case. So I don't think I necessarily need to pin this down further; what do you think, @gnprice?

Yep, sgtm! Thanks for pinning down the behavior of the part we need.