Skip to content

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.

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

Closed
wants to merge 1 commit into from
Closed

Address windows deprecation warning #8327

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion WooCommerce/Classes/Extensions/UIApplication+Woo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,15 @@ extension UIApplication {
/// Returns the keyWindow. Accessing `UIApplication.shared.keyWindow` is deprecated from iOS 13.
///
var currentKeyWindow: UIWindow? {
UIApplication.shared.windows.filter {$0.isKeyWindow}.first
// See https://stackoverflow.com/a/58031897/809944
UIApplication
.shared
// Get all of the currently connected UIScene instances
.connectedScenes
// Filter the connected UIScene instances for those that are UIWindowScene instances
// and map to their keyWindow property
.compactMap { ($0 as? UIWindowScene)?.keyWindow }
Copy link
Contributor Author

@mokagio mokagio Dec 6, 2022

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:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Products/SKU Scanner/BarcodeScannerViewController.swift

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
}
}

Copy link
Contributor

@crazytonyli crazytonyli Dec 7, 2022

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?

Copy link
Contributor Author

@mokagio mokagio Dec 7, 2022 •
edited
Loading

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.

Copy link
Member

@koke koke Dec 7, 2022

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.shouldDisableAnimations is true. For this case, we could as well iterate through all UIApplication.shared.connectedScenes and set the layer speed for all their key windows.
  • Every other instance happens on a UIViewController, which could use view.window instead?

Copy link
Contributor

@ealeksandrov ealeksandrov Dec 16, 2022

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.

.first
}
}

Expand Down