prevent from publishing dimensions change event when app changes state #11

lbaldy · 2022-05-24T07:25:49Z

Summary

An issue raised in Expensify/App#2727 has to be fixed in react native core. This impacts IOS only.

Changelog

a) The body of the _interfaceOrientationDidChange:

Here we make sure we run this function when the app is in active and also it was switching to fullscreeen or is not running in fullscreen.

- (void)_interfaceOrientationDidChange
{
  UIInterfaceOrientation nextOrientation = [RCTSharedApplication() statusBarOrientation];
  UIApplicationState appState = [RCTSharedApplication() applicationState];
  BOOL isRunningInFullScreen = CGRectEqualToRect([UIApplication sharedApplication].delegate.window.frame, [UIApplication sharedApplication].delegate.window.screen.bounds);

  // Update when we go from portrait to landscape, or landscape to portrait
  if ((((UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) &&
       !UIInterfaceOrientationIsPortrait(nextOrientation)) ||
      (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) &&
       !UIInterfaceOrientationIsLandscape(nextOrientation)) || (isRunningInFullScreen != _isFullscreen || !isRunningInFullScreen)))
      && appState == UIApplicationStateActive) {
      #pragma clang diagnostic push
      #pragma clang diagnostic ignored "-Wdeprecated-declarations"
          [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
                                                                                body:RCTExportedDimensions(_moduleRegistry)];
            _currentInterfaceOrientation = nextOrientation;
            _isFullscreen = isRunningInFullScreen;
      #pragma clang diagnostic pop
  }


}

b) _interfaceFrameDidChange

Here we make sure we run it when the app is in active state.

- (void)_interfaceFrameDidChange
{
  NSDictionary *nextInterfaceDimensions = RCTExportedDimensions(_moduleRegistry);
    UIApplicationState appState = [RCTSharedApplication() applicationState];

  if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions]) && appState == UIApplicationStateActive) {
    #pragma clang diagnostic push
    #pragma clang diagnostic ignored "-Wdeprecated-declarations"
        [[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
                                                                              body:nextInterfaceDimensions];
          _currentInterfaceDimensions = nextInterfaceDimensions;
    #pragma clang diagnostic pop
  }
}

c) Add the _isFullscreen variable to hold the previous state for multitasking view.

@implementation RCTDeviceInfo {
  UIInterfaceOrientation _currentInterfaceOrientation;
  NSDictionary *_currentInterfaceDimensions;
  BOOL _isFullscreen;
}

d) We need to make sure to run the interfaceOrientationDidChange instead of interfaceFrameDidChange when the UIApplicationDidBecomeActiveNotification occurs.

  [[NSNotificationCenter defaultCenter] addObserver:self
                                           selector:@selector(interfaceOrientationDidChange)
                                               name:UIApplicationDidBecomeActiveNotification
                                             object:nil];

Test Plan

Test Preparation

Make sure you have a working version of E/App.
Open App/src/components/withWindowDimensions.js and update the constructor by changing this line:

this.onDimensionChange = _.debounce(this.onDimensionChange.bind(this), 100);

to

this.onDimensionChange = this.onDimensionChange.bind(this);

Open the NewExpensify.xcodeproj in xCode.
Open the RCTDeviceInfo.mm file and replace it's contents with the file from this PR.
Select your device of choice (I suggest starting with iPad mini) and run the app though xCode.
From this point you can move to the test scenarios described below.

iPad Mini tests:

Reproduction + Fix test video (Test 1): https://youtu.be/jyzoNHLYHPo
Reproduction + Fix test video (Test 2): https://youtu.be/CLimE-Fba-g

Test 1:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background.
Put app back to foreground - make sure the side menu doesn't flicker.

Test 2:

Launch app in portrait, open chat - no sidebar visible.
Switch to landscape - sidebar shows.
Put app to background. Switch orientation back to portrait.
Put app back to foreground - make sure the side menu hides again as it should be in portrait.

iPad Pro tests:

Reproduction + Fix test video (Test 3, Test 4): https://youtu.be/EJkUUQCiLRg

iPad mini test 1 applies.

Scenario 2 does not as the screen is too wide in both orientations and iPad pro shows sidebar always.

Test 3:

launch the app.
Make sure you're in landscape mode.
See split screen with some other app. Make sure the side bar is visible.
Play with the size of the view, resize it a bit. When the view shrinks it should hide the sidebar, when it grows it should show it.
Move the app to background and back to foreground, please observe there are no flickers.

Test 4:

Launch the app.
Make sure you're in landscape mode.
Make the multitasking view and make Expensify app a slide over app.
Move back to fullscreen/split screen. Make sure the menu is shown accordingly
Move the app to background and back to foreground, please observe there are no flickers.

iPhone:

Non reg with and without the fix video: https://youtu.be/kuv9in8vtbk

Please perform standard smoke tests on transformation changes.

…ent when it doesn't apply

parasharrajat · 2022-05-24T17:06:59Z

I see that PR can be improved in the following ways.

Formatting of OP.
It would be good to link existing RN issues to the PR.
Videos and reproduction.

lbaldy · 2022-05-25T07:51:44Z

@parasharrajat I added the videos of me reproducing the issue and testing this with the fix applied. Had to upload to youtube due to size constrains here on Github.

Formatting of OP.
I updated the formatting to make the PR description a little bit more legible. If you see anything else that should be improved please let me know.
It would be good to link existing RN issues to the PR.
I am not sure which issues you are referring to? I linked the original issue from the Expensify/App repo already.
Videos and reproduction.
Added.

Please let me know if you believe anything else has to be imoproved/corrected.

Thanks.

parasharrajat · 2022-05-25T11:43:17Z

I will test this today.

lbaldy · 2022-06-02T19:56:40Z

Hi @parasharrajat any news on this one?

parasharrajat · 2022-06-02T20:16:41Z

On my list. Sorry for the delay. I broke my Mac past week so things got piled up.

parasharrajat · 2022-06-05T10:07:32Z

It would be good to link existing RN issues to the PR.

Existing issues about this bug we are trying to solve. Please search RN codebase and link any whatever looks the same. In case there are none, create a new one which we can refer to while submitting the PR to RN.

lbaldy · 2022-06-05T20:14:35Z

@parasharrajat I updated the original post.

I added the facebook#29290 which was mentioned in the original Expensify/App#2727.

parasharrajat · 2022-06-06T14:29:24Z

I will start reviewing this in few minutes. Code looks fine to me.

parasharrajat

I see your test cases. But you didn't mention test process. How can a person test these code changes on E/App? Please add this detail.

Also, add steps to test your changes in standard testing example app of RN.

parasharrajat · 2022-06-06T16:54:52Z

React/CoreModules/RCTDeviceInfo.mm

+ if ((((UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) &&
 !UIInterfaceOrientationIsPortrait(nextOrientation)) ||
 (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) &&
- !UIInterfaceOrientationIsLandscape(nextOrientation))) {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
- [[_moduleRegistry moduleForName:"EventDispatcher"]
- sendDeviceEventWithName:@"didUpdateDimensions"
- body:RCTExportedDimensions(_moduleRegistry, _bridge)];
-#pragma clang diagnostic pop
+ !UIInterfaceOrientationIsLandscape(nextOrientation)) || (isRunningInFullScreen != _isFullscreen || !isRunningInFullScreen)))


Let's break this if condition in more readable form. Also, improve the comment to indicate every case.

done, in b0c3bc7

parasharrajat · 2022-06-06T16:59:05Z

React/CoreModules/RCTDeviceInfo.mm

-
- _currentInterfaceDimensions = nextInterfaceDimensions;


Why are we moving these inside the if blocks?

I added a comment in code, but that's moved inside so that we make sure that the state between JS and Obj-C is synced. Meaning we only update the 'Native' state when it's published to JS.

parasharrajat · 2022-06-06T17:02:39Z

We need to make sure to run the interfaceOrientationDidChange instead of interfaceFrameDidChange when the UIApplicationDidBecomeActiveNotification occurs.

Why?

lbaldy · 2022-06-06T20:05:39Z

We need to make sure to run the interfaceOrientationDidChange instead of interfaceFrameDidChange when the UIApplicationDidBecomeActiveNotification occurs.

Why?

So that in case of this event we also have the extended handler, which considers the fullscreen as well as active state.

lbaldy · 2022-06-06T20:08:58Z

Updated

parasharrajat · 2022-06-06T20:42:01Z

~~Interesting, I am trying to test the iOS build in landscape mode. I commented the code that was used to prevent the landscape but still app is not going to landscape. This is giving me nuts.~~

Sorry, ignore this. I was doing totally different thing. 🗣️

parasharrajat

I am running final tests. Looking good.

React/CoreModules/RCTDeviceInfo.mm

parasharrajat

LGTM.

cc: @iwiznia Please review this PR.

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

sketchydroide · 2022-06-07T10:29:02Z

React/CoreModules/RCTDeviceInfo.mm

+
+
+ BOOL isActive = appState == UIApplicationStateActive;
+ BOOL isChangingFullscreen = (isRunningInFullScreen != _isFullscreen || !isRunningInFullScreen);


this is a weird condition, and the name does not seem to match the condition
If we are keeping the condition logic, I think we need a comment to explain why this is not simply
BOOL isChangingFullscreen = isRunningInFullScreen != _isFullscreen;
Also the condition logic could be simplified to
BOOL isChangingFullscreen = !isRunningInFullScreen || !_isFullscreen;
And again I'm not entirely sure that condition makes sense

This condition indeed can be shortened to what you suggested, good catch. Thanks.

The condition is needed and called this way because it catches two scenarios:

a) Simple resize when the app isn't in the fullscreen mode right now - the !isRunningInFullScreen
b) Resize when the app gets from slide over mode to fullscreen - isRunningInFullScreen != _isFullscreen

Anyway, I commented this line with the above.

sketchydroide

I have a question about one of the conditions about isChangingFullscreen

The rest seems good

… name, add more comments describing it

lbaldy · 2022-06-07T20:15:16Z

Updated

sketchydroide

LGTM

parasharrajat

LGTM

🎀 👀 🎀 C+ reviewed

lbaldy · 2022-06-09T09:01:59Z

@parasharrajat is there any action pending on me to get this one merged? Or shall we simply wait for inputs from @iwiznia.

Justicea83

LGTM!

sketchydroide · 2022-06-09T10:05:09Z

@lbaldy I think @iwiznia is still reviwieng this PR, he asked me to take a look as I'm more confortable with ObjC, but I think his input is still important

parasharrajat · 2022-06-09T12:22:58Z

shall we simply wait for inputs from @iwiznia.

This one.

fix useWindowDimensions, prevent from publishing dimensions change ev…

8653851

…ent when it doesn't apply

lbaldy mentioned this pull request May 24, 2022

[$16,000] Let's fix a React Native issue -- useWindowDimensions() returns swapped height and width in iOS Expensify/App#2727

Closed

5 tasks

parasharrajat suggested changes Jun 6, 2022

View reviewed changes

update code structure and add comments

b0c3bc7

parasharrajat suggested changes Jun 6, 2022

View reviewed changes

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

React/CoreModules/RCTDeviceInfo.mm Outdated Show resolved Hide resolved

parasharrajat reviewed Jun 6, 2022

View reviewed changes

iwiznia self-requested a review June 6, 2022 22:25

lbaldy and others added 3 commits June 7, 2022 09:15

improve comments

e21080d

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

simplify comments

b4b38d2

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

update comments

818a073

sketchydroide reviewed Jun 7, 2022

View reviewed changes

sketchydroide suggested changes Jun 7, 2022

View reviewed changes

sketchydroide self-requested a review June 7, 2022 10:34

shorten the condition, rename the variable to give it more meaningful…

27e65a5

… name, add more comments describing it

sketchydroide approved these changes Jun 8, 2022

View reviewed changes

parasharrajat approved these changes Jun 8, 2022

View reviewed changes

Justicea83 approved these changes Jun 9, 2022

View reviewed changes

iwiznia approved these changes Jun 9, 2022

View reviewed changes

iwiznia merged commit f690dc1 into Expensify:master Jun 9, 2022

fabioh8010 mentioned this pull request Jul 5, 2023

Update App to React Native 0.72 version Expensify/App#18507

Merged

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent from publishing dimensions change event when app changes state #11

prevent from publishing dimensions change event when app changes state #11

lbaldy commented May 24, 2022 •

edited

Loading

parasharrajat commented May 24, 2022

lbaldy commented May 25, 2022

parasharrajat commented May 25, 2022

lbaldy commented Jun 2, 2022

parasharrajat commented Jun 2, 2022 •

edited

Loading

parasharrajat commented Jun 5, 2022

lbaldy commented Jun 5, 2022

parasharrajat commented Jun 6, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Jun 6, 2022

lbaldy Jun 6, 2022

parasharrajat Jun 6, 2022 •

edited

Loading

lbaldy Jun 6, 2022

parasharrajat commented Jun 6, 2022

lbaldy commented Jun 6, 2022

lbaldy commented Jun 6, 2022

parasharrajat commented Jun 6, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment

sketchydroide Jun 7, 2022 •

edited

Loading

lbaldy Jun 7, 2022

sketchydroide left a comment

lbaldy commented Jun 7, 2022

sketchydroide left a comment

parasharrajat left a comment

lbaldy commented Jun 9, 2022

Justicea83 left a comment

sketchydroide commented Jun 9, 2022

parasharrajat commented Jun 9, 2022



		BOOL isActive = appState == UIApplicationStateActive;
		BOOL isChangingFullscreen = (isRunningInFullScreen != _isFullscreen \|\| !isRunningInFullScreen);

prevent from publishing dimensions change event when app changes state #11

prevent from publishing dimensions change event when app changes state #11

Conversation

lbaldy commented May 24, 2022 • edited Loading

Summary

Changelog

Test Plan

Test Preparation

iPad Mini tests:

iPad Pro tests:

iPhone:

parasharrajat commented May 24, 2022

lbaldy commented May 25, 2022

parasharrajat commented May 25, 2022

lbaldy commented Jun 2, 2022

parasharrajat commented Jun 2, 2022 • edited Loading

parasharrajat commented Jun 5, 2022

lbaldy commented Jun 5, 2022

parasharrajat commented Jun 6, 2022 • edited Loading

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat Jun 6, 2022

Choose a reason for hiding this comment

lbaldy Jun 6, 2022

Choose a reason for hiding this comment

parasharrajat Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

lbaldy Jun 6, 2022

Choose a reason for hiding this comment

parasharrajat commented Jun 6, 2022

lbaldy commented Jun 6, 2022

lbaldy commented Jun 6, 2022

parasharrajat commented Jun 6, 2022 • edited Loading

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

sketchydroide Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

lbaldy Jun 7, 2022

Choose a reason for hiding this comment

sketchydroide left a comment

Choose a reason for hiding this comment

lbaldy commented Jun 7, 2022

sketchydroide left a comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

lbaldy commented Jun 9, 2022

Justicea83 left a comment

Choose a reason for hiding this comment

sketchydroide commented Jun 9, 2022

parasharrajat commented Jun 9, 2022

lbaldy commented May 24, 2022 •

edited

Loading

parasharrajat commented Jun 2, 2022 •

edited

Loading

parasharrajat commented Jun 6, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Jun 6, 2022 •

edited

Loading

parasharrajat commented Jun 6, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

sketchydroide Jun 7, 2022 •

edited

Loading