Access local window instead of via UIApplication or UIApplicationDelegate#25672
Access local window instead of via UIApplication or UIApplicationDelegate#25672crazytonyli wants to merge 9 commits into
UIApplication or UIApplicationDelegate#25672Conversation
The quick-action handler already holds the shared presenter; use its rootViewController to dismiss rather than reaching for the app delegate's window.
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 32848 | |
| Version | PR #25672 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 78c2d08 | |
| Installation URL | 6ii6ifvmr4u10 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 32848 | |
| Version | PR #25672 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 78c2d08 | |
| Installation URL | 4772nrds2omsg |
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
One small change, but then this is good.
One other thing – it seems ReaderDetailFeaturedImageView might not be used anywhere?
| func startLoading() { | ||
| activityIndicatorView.startAnimating() | ||
| UIApplication.shared.mainWindow?.isUserInteractionEnabled = false | ||
| view.window?.isUserInteractionEnabled = false |
There was a problem hiding this comment.
It would be super rare, but it's possible for this view to change and break the entire window.
I wonder if we should either:
- Not disable the entire window
- Capture the window reference until
stopLoading?
There was a problem hiding this comment.
it's possible for this view to change and break the entire window.
Do you mean like calling startLoading without calling stopLoading afterward?
Capture the window reference until stopLoading?
Is the concern that view.window in stopLoading becomes nil, like it's called after the view controller is dismissed?
The concerns seem to be around making these two functions more error-proof for the future. I think one solution could be getting rid of the startLoading and stopLoading functions and moving them inline to the caller. They are private and used in one place after all. 😆
Not disable the entire window
I'm not very keen on this change, because I just checked tracks that the involved interaction (disconnecting Jetpack) only happens once per day, so it's probably not worth changing the UX.
Good catch. I'll remove it. |
4 participants
There are many places in the app that access
UIWindowinstance via the app-wide APIUIApplicationorUIApplicationDelegate. This PR refactors some of those usages to rely on local context via view or view controller.