fix(android/platform): reinitialise screen metrics on orientation change - #6164
vakrilov merged 5 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Do you think you can unify how appModule.on("cssChanged", ...) and appModule.on("orientationChanged", ...) events are handled. They can even call the same callback that re-inits the metrics. It is best that subscriptions happens on a single place (currently the "cssChanged" is handled on first get of metrics prop)
Sorry, something went wrong.
|
@vakrilov , just to confirm, you mean to move the cssChanged sub in the constructor? I was thinking about this, but since I wasn't sure if there might be a reason for the delayed sub, I decided not to touch the existing code 😉 |
Sorry, something went wrong.
|
Can you move the attaching the orientation changed event into the private get metrics() method. The idea here is that if nobody accesses the metrics filed - we won't listen to css/orientation events. As soon as someone does access them - we need to listen to those events so that we invalidate the cached metrics in case of an event. |
Sorry, something went wrong.
|
Personally don't like subscriptions to happen inside a property since someone from outside could potentially nullify this._metrics and cause a double sub, but will see to change it a bit later. |
Sorry, something went wrong.
|
test |
Sorry, something went wrong.
|
@PeterStaev I can see your point and frankly - the constructor seems more logical place to put the subscriptions from a code-readability standpoint too. I think it is a kind of micro optimization to avoid subscribing if not necessary. The metrics() is private so it should be enough from external tinkering (as far as anything can be safe in JS 😄 ). |
Sorry, something went wrong.
|
I checked the commits and it was moved to the dedicated property, because it was causing problems with generating android-snapshots. The snapshot should be OK now, even if we do the subscriptions are done in the constructor, because with webpack@4 we changed how the vendor-chunk is generated, but I don't think its worth it to move it again. |
Sorry, something went wrong.
…nge (#6164) * fix(android/platform): reinitialise screen metrics on orientation change * fix(android/platform): reinitialise screen metrics on orientation change
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sorry, something went wrong.
PR Checklist
What is the current behavior?
When getting the screen width/height on android those are the same, no matter if you rotate the device or not. Unlike iOS where the properties always reflect the actual device width/height.
What is the new behavior?
platform.screen.mainScreen width and height properties will always show the actual values like on iOS.
Fixes #4588.