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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Jun 9, 2022
Merged

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

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8653851
fix useWindowDimensions, prevent from publishing dimensions change ev…
lbaldy May 24, 2022
b0c3bc7
update code structure and add comments
lbaldy Jun 6, 2022
e21080d
improve comments
lbaldy Jun 7, 2022
b4b38d2
simplify comments
lbaldy Jun 7, 2022
818a073
update comments
lbaldy Jun 7, 2022
27e65a5
shorten the condition, rename the variable to give it more meaningful…
lbaldy Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 36 additions & 21 deletions React/CoreModules/RCTDeviceInfo.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ @interface RCTDeviceInfo () <NativeDeviceInfoSpec, RCTInitializing>
@implementation RCTDeviceInfo {
UIInterfaceOrientation _currentInterfaceOrientation;
NSDictionary *_currentInterfaceDimensions;
BOOL _isFullscreen;
}

@synthesize bridge = _bridge;
Expand Down Expand Up @@ -60,7 +61,7 @@ - (void)initialize
_currentInterfaceDimensions = RCTExportedDimensions(_moduleRegistry, _bridge);

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

Expand Down Expand Up @@ -174,21 +175,31 @@ - (void)interfaceOrientationDidChange
- (void)_interfaceOrientationDidChange
{
UIInterfaceOrientation nextOrientation = [RCTSharedApplication() statusBarOrientation];
UIApplicationState appState = [RCTSharedApplication() applicationState];
BOOL isRunningInFullScreen = CGRectEqualToRect([UIApplication sharedApplication].delegate.window.frame, [UIApplication sharedApplication].delegate.window.screen.bounds);


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

@sketchydroide sketchydroide Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

BOOL isOrientationChanging = (UIInterfaceOrientationIsPortrait(_currentInterfaceOrientation) &&
!UIInterfaceOrientationIsPortrait(nextOrientation)) || (UIInterfaceOrientationIsLandscape(_currentInterfaceOrientation) &&
!UIInterfaceOrientationIsLandscape(nextOrientation));

// Update when we go from portrait to landscape, or landscape to portrait
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
// Also update when the fullscreen state changes (multitasking) and only when the app is in active state.
if ((isOrientationChanging || isChangingFullscreen) && isActive) {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
[[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
body:RCTExportedDimensions(_moduleRegistry, _bridge)];
// We only want to track the current _currentInterfaceOrientation and _isFullscreen only
// when it happens and only when it is published.
_currentInterfaceOrientation = nextOrientation;
_isFullscreen = isRunningInFullScreen;
#pragma clang diagnostic pop
}

_currentInterfaceOrientation = nextOrientation;

}

- (void)interfaceFrameDidChange
Expand All @@ -202,16 +213,20 @@ - (void)interfaceFrameDidChange
- (void)_interfaceFrameDidChange
{
NSDictionary *nextInterfaceDimensions = RCTExportedDimensions(_moduleRegistry, _bridge);

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

BOOL isActive = appState == UIApplicationStateActive;
// update and publish the even only when the app is in active state
if (!([nextInterfaceDimensions isEqual:_currentInterfaceDimensions]) && isActive) {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
[[_moduleRegistry moduleForName:"EventDispatcher"] sendDeviceEventWithName:@"didUpdateDimensions"
body:nextInterfaceDimensions];
// We only want to track the current _currentInterfaceOrientation and _isFullscreen only
// when it happens and only when it is published.
_currentInterfaceDimensions = nextInterfaceDimensions;
#pragma clang diagnostic pop
}

_currentInterfaceDimensions = nextInterfaceDimensions;
Comment on lines -213 to -214
Copy link
Member

@parasharrajat parasharrajat Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we moving these inside the if blocks?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

- (std::shared_ptr<TurboModule>)getTurboModule:(const ObjCTurboModule::InitParams &)params
Expand Down