From 816cbe7436f412e621046c3c74fcd414275427f1 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 8 Feb 2016 17:34:26 +0100 Subject: [PATCH] Refactor ImmuTablePresenter/Controller/VC The main goal is to remove `visible` from `ImmuTablePresenter`, since it doesn't really belong there. Presenter is a thing that can "present" view controllers: usually a UIViewController, but implemented as a protocol so it's testable. But then I realized, the Controller doesn't really need a reference to Presenter, or the parent View Controller. The only thing that needs that is ImmuTable (for the callbacks), and we can pass the presenter when requesting the table view model (renamed). This is similar to the previous solution, but avoids the chicken-and-egg problem when initiailizing. And because of this, it removes 4 assertions/preconditions that are now guaranteed at compile time. The `visible` observable is still exposed on ImmuTableViewController, but the VC is the one pausing the subscription to tableViewModel and errorMessage, instead of relying on the controller. I like this slightly less semantically, but it makes everything much simpler with initialization. --- .../Utility/ImmuTableViewController.swift | 41 +++++++++---------- .../AccountSettingsViewController.swift | 11 +---- .../Me/MyProfileViewController.swift | 11 +---- .../Classes/ViewRelated/SettingsCommon.swift | 17 +++----- 4 files changed, 26 insertions(+), 54 deletions(-) diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index d66b345a2226..1f39d07a2a77 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -5,7 +5,6 @@ import WordPressShared typealias ImmuTableRowControllerGenerator = ImmuTableRow -> UIViewController protocol ImmuTablePresenter: class { - var visible: Observable { get } func push(controllerGenerator: ImmuTableRowControllerGenerator) -> ImmuTableAction } @@ -20,11 +19,10 @@ extension ImmuTablePresenter where Self: UIViewController { } protocol ImmuTableController { - var presenter: ImmuTablePresenter? { get set } var title: String { get } var immuTableRows: [ImmuTableRow.Type] { get } - var immuTable: Observable { get } var errorMessage: Observable { get } + func tableViewModelWithPresenter(presenter: ImmuTablePresenter) -> Observable } /// Generic view controller to present ImmuTable-based tables @@ -41,32 +39,31 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { private var errorAnimator: ErrorAnimator! - let controller: ImmuTableController? + let controller: ImmuTableController private let bag = DisposeBag() // MARK: - Table View Controller - init(controller: ImmuTableController? = nil) { + init(controller: ImmuTableController) { self.controller = controller super.init(style: .Grouped) - self.controller?.presenter = self - if let controller = self.controller { - title = controller.title - registerRows(controller.immuTableRows) - controller.immuTable - .observeOn(MainScheduler.instance) - .subscribeNext({ [weak self] in - self?.handler.viewModel = $0 - }) - .addDisposableTo(bag) - controller.errorMessage - .observeOn(MainScheduler.instance) - .subscribeNext({ [weak self] in - self?.errorMessage = $0 - }) - .addDisposableTo(bag) - } + title = controller.title + registerRows(controller.immuTableRows) + controller.tableViewModelWithPresenter(self) + .pausable(visible) + .observeOn(MainScheduler.instance) + .subscribeNext({ [weak self] in + self?.handler.viewModel = $0 + }) + .addDisposableTo(bag) + controller.errorMessage + .pausable(visible) + .observeOn(MainScheduler.instance) + .subscribeNext({ [weak self] in + self?.errorMessage = $0 + }) + .addDisposableTo(bag) } required init?(coder aDecoder: NSCoder) { diff --git a/WordPress/Classes/ViewRelated/AccountSettingsViewController.swift b/WordPress/Classes/ViewRelated/AccountSettingsViewController.swift index e985c6a73707..e3b69cb568a3 100644 --- a/WordPress/Classes/ViewRelated/AccountSettingsViewController.swift +++ b/WordPress/Classes/ViewRelated/AccountSettingsViewController.swift @@ -11,13 +11,10 @@ func AccountSettingsViewController(account account: WPAccount) -> ImmuTableViewC func AccountSettingsViewController(service service: AccountSettingsService) -> ImmuTableViewController { let controller = AccountSettingsController(service: service) let viewController = ImmuTableViewController(controller: controller) - assert(viewController.controller?.presenter != nil, "ImmuTableViewController should have set the presenter for AccountSettingsController") return viewController } private struct AccountSettingsController: SettingsController { - weak var presenter: ImmuTablePresenter? = nil - let title = NSLocalizedString("Account Settings", comment: "Account Settings Title"); var immuTableRows: [ImmuTableRow.Type] { @@ -38,13 +35,7 @@ private struct AccountSettingsController: SettingsController { // MARK: - Model mapping - func mapViewModel(settings: AccountSettings?) -> ImmuTable { - precondition(presenter != nil, "presenter must be set before using") - guard let presenter = presenter else { - // This shouldn't happen. If there's no presenter we can't push the - // editText controllers. - return ImmuTable.Empty - } + func mapViewModel(settings: AccountSettings?, presenter: ImmuTablePresenter) -> ImmuTable { let username = TextRow( title: NSLocalizedString("Username", comment: "Account Settings Username label"), value: settings?.username ?? "") diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 1ffac915badb..e8248170f8fd 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -10,7 +10,6 @@ func MyProfileViewController(account account: WPAccount) -> ImmuTableViewControl func MyProfileViewController(service service: AccountSettingsService) -> ImmuTableViewController { let controller = MyProfileController(service: service) let viewController = ImmuTableViewController(controller: controller) - assert(viewController.controller?.presenter != nil, "ImmuTableViewController should have set the presenter for MyProfileController") return viewController } @@ -20,8 +19,6 @@ func MyProfileViewController(service service: AccountSettingsService) -> ImmuTab private struct MyProfileController: SettingsController { // MARK: - ImmuTableController - weak var presenter: ImmuTablePresenter? = nil - let title = NSLocalizedString("My Profile", comment: "My Profile view title") var immuTableRows: [ImmuTableRow.Type] { @@ -38,13 +35,7 @@ private struct MyProfileController: SettingsController { // MARK: - Model mapping - func mapViewModel(settings: AccountSettings?) -> ImmuTable { - precondition(presenter != nil, "presenter must be set before using") - guard let presenter = presenter else { - // This shouldn't happen. If there's no presenter we can't push the - // editText controllers. - return ImmuTable.Empty - } + func mapViewModel(settings: AccountSettings?, presenter: ImmuTablePresenter) -> ImmuTable { let firstNameRow = EditableTextRow( title: NSLocalizedString("First Name", comment: "My Profile first name label"), value: settings?.firstName ?? "", diff --git a/WordPress/Classes/ViewRelated/SettingsCommon.swift b/WordPress/Classes/ViewRelated/SettingsCommon.swift index 4b2d7769a99e..9945d2be9a74 100644 --- a/WordPress/Classes/ViewRelated/SettingsCommon.swift +++ b/WordPress/Classes/ViewRelated/SettingsCommon.swift @@ -2,8 +2,7 @@ import RxSwift protocol SettingsController: ImmuTableController { var service: AccountSettingsService { get } - var presenter: ImmuTablePresenter? { get } - func mapViewModel(settings: AccountSettings?) -> ImmuTable + func mapViewModel(settings: AccountSettings?, presenter: ImmuTablePresenter) -> ImmuTable } // MARK: - Shared implementation @@ -16,20 +15,14 @@ extension SettingsController { SwitchRow.self] } - var immuTable: Observable { - precondition(presenter != nil, "presenter must be set before using") - return service.settings.map(mapViewModel) + func tableViewModelWithPresenter(presenter: ImmuTablePresenter) -> Observable { + return service.settings.map({ settings in + self.mapViewModel(settings, presenter: presenter) + }) } var errorMessage: Observable { - precondition(presenter != nil, "presenter must be set before using") - guard let presenter = presenter else { - // This shouldn't happen, but if it does, disabling the error feels - // safer than having it running when the VC is not visible. - return Observable.just(nil) - } return service.refresh - .pausable(presenter.visible) // replace errors with .Failed status .catchErrorJustReturn(.Failed) // convert status to string