-
Notifications
You must be signed in to change notification settings - Fork 119
Address windows deprecation warning
#8327
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
Conversation
The compiler said: > 'windows' was deprecated in iOS 15.0: Use UIWindowScene.windows on a > relevant window scene instead
Generated by 🚫 dangerJS |
| .connectedScenes | ||
| // Filter the connected UIScene instances for those that are UIWindowScene instances | ||
| // and map to their keyWindow property | ||
| .compactMap { ($0 as? UIWindowScene)?.keyWindow } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the option to split this into one computed var for currentWindowScene and one that calls keyWindow on it.
I suggest this because I noticed one instance in which we call this currentKeyWindow only to then access the windowScene of the result:
Line 209 in f629e6c
| let orientation = UIApplication.shared.currentKeyWindow?.windowScene?.interfaceOrientation |
For additional reference, we also have the code below that uses windowScene, although that one does it starting bottom-up from a UIViewController, rather than top-down from UIApplication.
woocommerce-ios/WooCommerce/Classes/Extensions/UIViewController+AppReview.swift
Lines 25 to 40 in f629e6c
| private extension UIViewController { | |
| /// Attempts to return the scene in with the current controller lives | |
| /// If its view is attached to a window, it will use the scene from that window | |
| /// Otherwise, it looks at it's ancestors for a valid window to find the scene | |
| var currentScene: UIWindowScene? { | |
| if let window = view.window { | |
| return window.windowScene | |
| } | |
| if let parent = parent { | |
| return parent.currentScene | |
| } | |
| return nil | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should delete this re-implementation? The original keyWindow API has been deprecated for three years. Maybe it's time to stop relying on the no longer true concept that there is one "key window" across the whole application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 🤔 I think the solution might vary on a case by case basis across the various usages of this currentKeyWindow computed property.
I left a comment in a pre-existing issue on the topic that I didn't know about till your comment prompted me to do more research.
@koke, as the author of that issue, have you got any additional context or ideas how to go about this?
I value the removal of the warning and I'm tempted to argue for merging this to reduce the warnings noise. At the same time, I'm well to aware that if we silence it, we might not address the lack of multi window support for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a lot of experience dealing with windows and scenes, but I agree with @crazytonyli that the best approach is probably to get rid of this method. Looking at usage, I see two distinct use cases:
- To speed up animations when
ProcessConfiguration.shouldDisableAnimationsis true. For this case, we could as well iterate through allUIApplication.shared.connectedScenesand set the layer speed for all their key windows. - Every other instance happens on a
UIViewController, which could useview.windowinstead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a draft to remove currentKeyWindow: #8440 as discussed above. It's mostly done, but I'll do additional testing next week on specific views functionality.
You can test the changes from this Pull Request by:
|
|
Closing this in favor of |
|
thanks @ealeksandrov 🎉 🙇♂️ |
Description
The compiler said:
Testing instructions
I looked at the code using this property but I'm not user how to exercise the app to trigger one of those code paths.
@woocommerce/ios-developers can some help validate this tweak? Thanks!
RELEASE-NOTES.txtif necessary.