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.

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

[Android] Update the cached dimensions when orientation changes #30324

Conversation

ajpaulingalls
Copy link
Contributor

Summary

Currently the dimensions are created once, and then cached. This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
#29105
#29451
#29323

Changelog

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Test Plan

Open up RNTester app. Select the Dimensions API list item. Rotate the device and verify that the dimensions are correct based on orientation.

…ge will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.
@facebook-github-bot
Copy link
Contributor

Hi @ajpaulingalls!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Nov 5, 2020
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3e77e15

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,507,277 +79
android hermes armeabi-v7a 7,099,065 +92
android hermes x86 7,945,421 +87
android hermes x86_64 7,856,485 +92
android jsc arm64-v8a 8,971,996 +93
android jsc armeabi-v7a 8,546,899 +100
android jsc x86 8,971,649 +100
android jsc x86_64 9,522,956 +93

Base commit: 3e77e15

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 5, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@luisnaranjo733
Copy link

luisnaranjo733 commented Nov 6, 2020

Thanks for sharing this fix @ajpaulingalls. I tested against this branch and can confirm that it fixes the issue I was seeing where the Dimensions.addEventListener wasn't firing callbacks in some scenarios. Getting this fix merged into master ASAP would be huge, so that we can push to have it cherry-picked into 0.63.4. This is a really bad bug in 0.63.x

It looks like the test_ios_unit_frameworks_hermes check failed, which seems completely unrelated to your change. Are you able to re-queue that CircleCI build somehow? Maybe it's an intermittent failure.

@ajpaulingalls
Copy link
Contributor Author

It looks like the test_ios_unit_frameworks_hermes check failed, which seems completely unrelated to your change. Are you able to re-queue that CircleCI build somehow? Maybe it's an intermittent failure.

I'm not sure how to kick it off again without making more code changes. Hopefully a real reviewer will see its unrelated and merge it anyway..:)

@acoates-ms acoates-ms added the p: Microsoft Partner: Microsoft label Nov 10, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@ejanzer
Copy link

ejanzer commented Nov 11, 2020

I think #29105 is actually a separate issue with Dimensions.get('screen') specifically - this PR doesn't seem to fix it for me. I'll look into it.

Edit: Nope, nevermind, I think that's just an issue with stale constants in the native module.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @ajpaulingalls in 0e9296b.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 11, 2020
@ajpaulingalls ajpaulingalls deleted the fixDimensionsOnOrientationChange branch November 11, 2020 19:05
alloy pushed a commit to alloy/react-native that referenced this pull request Nov 19, 2020
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
facebook#29105
facebook#29451
facebook#29323

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: facebook#30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
kelset pushed a commit that referenced this pull request Nov 27, 2020
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
#29105
#29451
#29323

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: #30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
brentvatne pushed a commit to expo/react-native that referenced this pull request Dec 3, 2020
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
facebook#29105
facebook#29451
facebook#29323

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: facebook#30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
@thymikee
Copy link
Contributor

Can we close all related issues then?

@kelset
Copy link
Contributor

kelset commented Mar 16, 2021

I think that this is fixed only on master, so for 0.64 and 0.63 is still an issue; until we cherry pick this into those and release patches we should keep them open imho

savv pushed a commit to savv/react-native-savv that referenced this pull request May 18, 2021
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
facebook#29105
facebook#29451
facebook#29323

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: facebook#30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
(cherry picked from commit 0e9296b)
luisnaranjo733 added a commit to luisnaranjo733/react-native that referenced this pull request May 25, 2021
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
facebook#29105
facebook#29451
facebook#29323

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: facebook#30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
kelset pushed a commit that referenced this pull request May 26, 2021
Summary:
Currently the dimensions are created once, and then cached.  This change will reload the dimensions when the device orientation changes to insure that dimension update events follow orientation changed events.

this should help address the following issues, that I know of:
#29105
#29451
#29323

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - Dimension update events are now properly sent following orientation change

Pull Request resolved: #30324

Test Plan: Open up RNTester app.  Select the Dimensions API list item.  Rotate the device and verify that the dimensions are correct based on orientation.

Reviewed By: fkgozali

Differential Revision: D24874733

Pulled By: ejanzer

fbshipit-source-id: 867681ecb009d368a2ae7b67d94d6355e67dea7b
@mjmasn
Copy link
Contributor

mjmasn commented May 27, 2021

I don't think this is fixed in the case where you have e.g. a tablet locked to portrait orientation (at the device level) then open a react-native app using react-native-orientation-locker and do Orientation.lockToLandscape() (not sure about the case where you lock the orientation via AndroidManifest.xml but may be similar). The Dimensions module always returns the width and height reversed both via Dimensions.get('window') and useWindowDimensions. I think that may be what #29105 is referring to @ejanzer

Using the useOrientationChange hook from react-native-orientation-locker returns PORTRAIT and useDeviceOrientationChange returns the 'real' device orientation, neither of which are what we want (as the app is locked to a fixed landscape orientation) so it's hard to even work around the issue (while still accounting for the possibility of split screen / floating window views).

Not strictly a RN issue but it feels like the Dimensions module should take this into account - maybe via Activity.getRequestedOrientation

@Quedo5
Copy link

Quedo5 commented Aug 6, 2021

Plataforma Motor Arco Tamaño (bytes) Diferencia
androide Hermes arm64-v8a 7,507,277 +79
androide Hermes armeabi-v7a 7.099.065 +92
androide Hermes x86 7,945,421 +87
androide Hermes x86_64 7.856.485 +92
androide jsc arm64-v8a 8,971,996 +93
androide jsc armeabi-v7a 8.546.899 +100
androide jsc x86 8,971,649 +100
androide jsc x86_64 9.522.956 +93
Confirmación base: 3e77e15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.