flowkey / UIKit-cross-platform

Cross-platform Swift implementation of UIKit, mostly for Android
MIT License
594 stars 40 forks source link

Overhaul SDLActivity.surfaceChanged #322

Closed rikner closed 3 years ago

rikner commented 3 years ago

Type of change: Bug fix

Motivation

Exit from surfaceChanged() early if the requested orientation is not yet the actual orientation.

This fixes layouting issues, since we are not destroying and then recreating our UIScreen.main anymore when the orientation changes. The problem with that was that onNativeResize() could not set the new screen resolution for the Android_Window, because it is null at that point (see here). Instead now we skip until the next surfaceChanged(), where the requested orientation matches the actual one and create the Screen only once with the correct dimensions.

Props to @michaelknoch who came up with the idea!

Note I

~~I tested this fix on top of c8abdd213701a197a687bd7977cb19e0b057061c and not on the latest master. When rebasing onto the latest master, I realized there is a crash when entering the flowkey player. Looks like it got introduced in 196ab113ab457a2c393689317b9fe6c067c75fd7. So that would need to get fixed, before we can use the latest UIKit master.~~ edit: PR is open

Note II

I want to add, that it might better to call nativeDestroyScreen() after onNativeResize() instead of the other way around now (see here). But anyway the order wouldn't really matter now with this fix in place (I guess) and also I'm not entirely sure if it would break something else.

Note III

This should also fix some crashes we saw in UIView, where it accessed UIScreen when it was null.

Screenrecording (before)

https://user-images.githubusercontent.com/2410252/109198589-b3340100-779e-11eb-889d-db70e457d980.mp4

Screenrecording (fix)

https://user-images.githubusercontent.com/2410252/109198636-c0e98680-779e-11eb-961e-20eaa0d1c877.mp4

Please check if the PR fulfills these requirements

codecov[bot] commented 3 years ago

Codecov Report

Merging #322 (819a221) into master (44f70d6) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   51.15%   51.15%           
=======================================
  Files          87       87           
  Lines        2895     2895           
=======================================
  Hits         1481     1481           
  Misses       1414     1414           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 44f70d6...819a221. Read the comment docs.