Convert AppDelegate and WMFSettingsViewController to Swift #3992
Conversation
tonisevener
left a comment
tonisevener
left a comment
There was a problem hiding this comment.
👏
@mdholloway Thanks so much for taking the time to do this, it looks like a lot of work! Most of my feedback is very small style stuff. Some comments may not be as explanatory as they could be, so please let me know if you have any questions.
| } | ||
| return _window | ||
| } | ||
| set(newWindow) { |
There was a problem hiding this comment.
We can probably remove this setter code if it's not used, and only have the getter.
| } | ||
| return _window | ||
| } | ||
| set(newWindow) { |
There was a problem hiding this comment.
I don't think we need this setter, can you remove?
| } | ||
| private var _window: UIWindow? | ||
|
|
||
| var appViewController: WMFAppViewController? |
There was a problem hiding this comment.
This could use a private.
| * Register default application preferences. | ||
| * @note This must be loaded before application launch so unit tests can run | ||
| */ | ||
| private static let setDefaultsForTesting: Void = UserDefaults.standard.register(defaults: ["WMFAutoSignTalkPageDiscussions": true]) |
There was a problem hiding this comment.
Looks like overriding that load class method like the legacy code fails in Swift. I looked a bit into registering defaults and I think it's safe to move this call (just the UserDefaults.standard.register(defaults: ["WMFAutoSignTalkPageDiscussions": true]) part) into the didFinishLaunching method. With the way it's set up here, I don't think this ever gets called.
| // MARK: UIApplicationDelegate | ||
|
|
||
| func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey : Any]? = nil) -> Bool { | ||
| self.registerBackgroundTasksForApplication(application) |
There was a problem hiding this comment.
No need for the self here.
| } | ||
|
|
||
| public override func scrollViewWillEndDragging(_ scrollView: UIScrollView, withVelocity: CGPoint, targetContentOffset: UnsafeMutablePointer<CGPoint>) { | ||
| navigationBarHider.scrollViewWillEndDragging(scrollView, withVelocity: withVelocity, targetContentOffset: targetContentOffset) |
There was a problem hiding this comment.
The withVelocity parameter name is odd here, we should look into why we can't pass in something like velocity here.
|
|
||
| // MARK: KVO | ||
|
|
||
| override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) { |
There was a problem hiding this comment.
This method can be removed if suggestion above is done.
|
|
||
| public override func apply(theme: Theme) { | ||
| super.apply(theme: theme) | ||
| if (viewIfLoaded == nil) { |
There was a problem hiding this comment.
guard viewIfLoaded != nil else is preferred here.
| private let backgroundFetchInterval: TimeInterval = 10800 // 3 Hours | ||
| private let backgroundAppRefreshTaskIdentifier = "org.wikimedia.wikipedia.appRefresh" | ||
|
|
||
| var window: UIWindow? { |
There was a problem hiding this comment.
I see a warning here:
Property 'window' nearly matches optional requirement 'window' of protocol 'UIApplicationDelegate'
I would rather not silence the warning, and I think we are at (or past) the point now where we should switch to using scene delegates. A new project template in Xcode sets this up by default, and we are in the midst of making the iOS 13 minimum deployment target transition where scenes were introduced.
We should be able to implement a scene delegate while still keeping the "Enable Multiple Windows" setting to NO in Info.plist, so I don't think this would dovetail into a ton of work for iPad. This work should probably still be done in a separate PR though. We can leave the warning here, add a TODO comment and make a Phab task representing this work.
| 67F73E752267B9070079DEEF /* TalkPageReplyListViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 67F73E742267B9070079DEEF /* TalkPageReplyListViewController.swift */; }; | ||
| 67F73E792267B9510079DEEF /* TalkPageTopicNewViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 67F73E782267B9500079DEEF /* TalkPageTopicNewViewController.swift */; }; | ||
| 67F9AE4923AD2F38003D4F5E /* Array+SafeIndex.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A3EE1532267DC3800709CF6 /* Array+SafeIndex.swift */; }; | ||
| 7016040E2684DE32009C84EC /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7016040D2684DE32009C84EC /* AppDelegate.swift */; }; |
There was a problem hiding this comment.
Can you also add the new files (main.swift, AppDelegate.swift, WMFSettingsViewController.swift) to our other 3 targets? These are just some alternative builds, they won't compile without these checked:
|
Going to go ahead and close this one due to other team priorities. |
2 participants
I tried my hand at converting a couple of classes to Swift. The motivation for this was to eliminate the need for Objective-C interoperability in the EventPlatformClient class, but wholesale conversion of all callers to Swift proved to be too much to take on right now, so I ended up taking a different approach to resolving our immediate need (#3991).
Disclaimer: I kept the logic pretty close to how it was in the Obj-C classes, the automated tests pass, and the app seems to be working well, but I'm not an iOS or Swift expert by any means, so please review with care!