Refactor out ArticleViewController into extensions #4942

l-olson1214 · 2024-08-26T13:50:30Z

Phabricator: N/A

Notes

Refactor ArticleViewController to have extension files where needed, clean up warnings of unused variables

Test Steps

Screenshots/Videos

tonisevener

@l-olson1214 Thanks for taking this on! I'm totally on board to move related methods out into extensions to more closely align with the original way ArticleViewController was built.

I commented on some methods in your extensions that I think should be shuffled around, and maybe some new extensions could be made - let me know what you think. One other thing:

For any new file that lives in the Wikipedia target, we should also check the Staging and Experimental targets in this screenshot. Otherwise the Experimental and Staging schemes won't compile. Thanks!

tonisevener · 2024-08-26T19:35:34Z

Wikipedia.xcodeproj/project.pbxproj

@@ -385,6 +385,16 @@
 		0EF8634E1C19E02700006D2D /* WMFEmptyView.xib in Resources */ = {isa = PBXBuildFile; fileRef = 0EF8634D1C19E02700006D2D /* WMFEmptyView.xib */; };
 		0EF863511C19E4F100006D2D /* WMFEmptyView.m in Sources */ = {isa = PBXBuildFile; fileRef = 0EF863501C19E4F100006D2D /* WMFEmptyView.m */; };
 		19A175F095F5197BA20EA8BA /* NSUserActivity+WMFExtensionsTest.m in Sources */ = {isa = PBXBuildFile; fileRef = 19A172FA6AE61E76FCEF4259 /* NSUserActivity+WMFExtensionsTest.m */; };
+		375B0AC92C7AC0DE00BDE00A /* ArticleViewController + AltTextExperiment.swift in Sources */ = {isa = PBXBuildFile; fileRef = 375B0AC82C7AC0DE00BDE00A /* ArticleViewController + AltTextExperiment.swift */; };


A space in the filename slipped in here.

tonisevener · 2024-08-26T19:39:31Z

Wikipedia/Code/ArticleViewController + AltTextExperiment.swift

+            self.navigationItem.titleView = nil
+            self.title = altTextExperimentViewModel.localizedStrings.articleNavigationBarTitle
+
+            let rightBarButtonItem =
+                UIBarButtonItem(
+                    image: WMFSFSymbolIcon.for(symbol: .ellipsisCircle),
+                    primaryAction: nil,
+                    menu: overflowMenu
+                )
+            navigationItem.rightBarButtonItem = rightBarButtonItem
+            rightBarButtonItem.tintColor = theme.colors.link
+
+            self.navigationBar.updateNavigationItems()


This one is a little tricky in that only the code in this conditional applies to Alt Text. Can you rework so that only this code lives in this extension, but the rest (else code, setupWebView, etc.) is back in ArticleViewController.swift?

tonisevener · 2024-08-26T19:40:33Z

Wikipedia/Code/ArticleViewController+ArticleAsLivingDoc.swift

+extension ArticleViewController: UISheetPresentationControllerDelegate {
+    func sheetPresentationControllerDidChangeSelectedDetentIdentifier(_ sheetPresentationController: UISheetPresentationController) {
+
+        guard altTextExperimentViewModel != nil else {
+            return
+        }
+
+        let oldContentInset = webView.scrollView.contentInset
+
+        if let selectedDetentIdentifier = sheetPresentationController.selectedDetentIdentifier {
+            switch selectedDetentIdentifier {
+            case .medium, .large:
+                webView.scrollView.contentInset = UIEdgeInsets(top: oldContentInset.top, left: oldContentInset.left, bottom: view.bounds.height * 0.65, right: oldContentInset.right)
+            default:
+                logMinimized()
+                webView.scrollView.contentInset = UIEdgeInsets(top: oldContentInset.top, left: oldContentInset.left, bottom: 75, right: oldContentInset.right)
+            }
+        }
+    }
+
+    private func logMinimized() {
+        if let project = project {
+            EditInteractionFunnel.shared.logAltTextInputDidMinimize(project: project)
+        }
+    }
+}
+
+extension ArticleViewController: WMFAltTextExperimentModalSheetLoggingDelegate {
+
+    func didTriggerCharacterWarning() {
+        if let project = project {
+            EditInteractionFunnel.shared.logAltTextInputDidTriggerWarning(project: project)
+        }
+    }
+
+    func didTapFileName() {
+        if let project = project {
+            EditInteractionFunnel.shared.logAltTextInputDidTapFileName(project: project)
+        }
+    }
+
+    func didAppear() {
+        if let project = project {
+            EditInteractionFunnel.shared.logAltTextInputDidAppear(project: project)
+        }
+    }
+
+    func didFocusTextView() {
+        if let project = project {
+            EditInteractionFunnel.shared.logAltTextInputDidFocus(project: project)
+        }
+    }
+}
+


This chunk of code is actually Alt Text related, can you move it to the Alt Text extension file?

tonisevener · 2024-08-26T19:42:51Z

Wikipedia/Code/ArticleViewController+ArticleLoad.swift

+    private func setupForAltTextExperiment() {
+
+        guard let altTextExperimentViewModel,
+         altTextBottomSheetViewModel != nil else {
+            return
+        }
+
+        let oldContentInset = webView.scrollView.contentInset
+        webView.scrollView.contentInset = UIEdgeInsets(top: oldContentInset.top, left: oldContentInset.left, bottom: view.bounds.height * 0.65, right: oldContentInset.right)
+        messagingController.hideEditPencils()
+        messagingController.scrollToNewImage(filename: altTextExperimentViewModel.filename)
+
+        presentAltTextModalSheet()
+    }
+
+    func presentAltTextModalSheet() {
+
+        guard altTextExperimentViewModel != nil,
+         let altTextBottomSheetViewModel else {
+            return
+        }
+
+        let bottomSheetViewController = WMFAltTextExperimentModalSheetViewController(viewModel: altTextBottomSheetViewModel, delegate: self, loggingDelegate: self)
+
+        if #available(iOS 16.0, *) {
+            if let sheet = bottomSheetViewController.sheetPresentationController {
+                sheet.delegate = self
+                let customSmallId = UISheetPresentationController.Detent.Identifier("customSmall")
+                let customSmallDetent = UISheetPresentationController.Detent.custom(identifier: customSmallId) { context in
+                    return 44
+                }
+                sheet.detents = [customSmallDetent, .medium(), .large()]
+                sheet.selectedDetentIdentifier = .medium
+                sheet.largestUndimmedDetentIdentifier = .medium
+                sheet.prefersGrabberVisible = true
+            }
+            bottomSheetViewController.isModalInPresentation = true
+            self.altTextBottomSheetViewController = bottomSheetViewController
+
+            present(bottomSheetViewController, animated: true) { [weak self] in
+                self?.presentAltTextTooltipsIfNecessary(force: false)
+            }
+        }
+    }
+
+    internal func presentAltTextTooltipsIfNecessary(force: Bool = false) {
+        guard let altTextExperimentViewModel,
+              let bottomSheetViewController = altTextBottomSheetViewController,
+              let tooltip1SourceView = view,
+              let tooltip2SourceView = bottomSheetViewController.tooltip2SourceView,
+              let tooltip2SourceRect = bottomSheetViewController.tooltip2SourceRect,
+              let tooltip3SourceView = bottomSheetViewController.tooltip3SourceView,
+              let tooltip3SourceRect = bottomSheetViewController.tooltip3SourceRect,
+        let dataController = WMFAltTextDataController.shared else {
+            return
+        }
+
+        if !force && dataController.hasPresentedOnboardingTooltips {
+            return
+        }
+
+        let tooltip1SourceRect = CGRect(x: 30, y: navigationBar.frame.height + 30, width: 0, height: 0)
+
+        let viewModel1 = WMFTooltipViewModel(localizedStrings: altTextExperimentViewModel.firstTooltipLocalizedStrings, buttonNeedsDisclosure: true, sourceView: tooltip1SourceView, sourceRect: tooltip1SourceRect, permittedArrowDirections: .up) { [weak self] in
+
+            if let siteURL = self?.articleURL.wmf_site,
+               let project = WikimediaProject(siteURL: siteURL) {
+                EditInteractionFunnel.shared.logAltTextOnboardingDidTapNextOnFirstTooltip(project: project)
+            }
+        }
+
+
+        let viewModel2 = WMFTooltipViewModel(localizedStrings: altTextExperimentViewModel.secondTooltipLocalizedStrings, buttonNeedsDisclosure: true, sourceView: tooltip2SourceView, sourceRect: tooltip2SourceRect, permittedArrowDirections: .down)
+
+        let viewModel3 = WMFTooltipViewModel(localizedStrings: altTextExperimentViewModel.thirdTooltipLocalizedStrings, buttonNeedsDisclosure: false, sourceView: tooltip3SourceView, sourceRect: tooltip3SourceRect, permittedArrowDirections: .down) { [weak self] in
+
+            if let siteURL = self?.articleURL.wmf_site,
+               let project = WikimediaProject(siteURL: siteURL) {
+                EditInteractionFunnel.shared.logAltTextOnboardingDidTapDoneOnLastTooltip(project: project)
+            }
+
+        }
+
+        bottomSheetViewController.displayTooltips(tooltipViewModels: [viewModel1, viewModel2, viewModel3])
+
+        if !force {
+            dataController.hasPresentedOnboardingTooltips = true
+        }
+    }


These 3 methods are also alt text related, can you move them to the alt text extension?

setupForAltTextExperiment presentAltTextTooltipsIfNecessary presentAltTextTooltipsIfNecessary

tonisevener · 2024-08-26T19:46:45Z

Wikipedia/Code/ArticleViewController+ArticleLoadErrors.swift

+    }
+}
+
+extension ArticleViewController: WKNavigationDelegate {


I would prefer putting this conformance code in it's own ArticleViewController+WKNavigationDelegate extension file actually. I think there are pieces to these methods that aren't dealing with failures, so living in a errors extension might be misleading.

tonisevener · 2024-08-26T19:53:18Z

Wikipedia/Code/ArticleViewController+Loading.swift

+import WMFData
+
+// MARK: - ArticleViewController + Loading
+extension ArticleViewController {


I think we should scrap this one and move these back into ArticleViewController.swift. I myself usually expect these lifecycle methods (viewDidLoad, viewWillAppear, etc.) to be one of the first things I see when looking at a view controller definition file, after the properties and the init methods.

tonisevener · 2024-08-26T19:56:12Z

Wikipedia/Code/ArticleViewController+Notifications.swift

+        surveyTimerController?.didBecomeActive(withState: state)
+    }
+
+    func setupSearchButton() {


I think everything from this point down should move elsewhere, as to me they look unrelated to Notifications.

tonisevener · 2024-08-26T19:57:08Z

Wikipedia/Code/ArticleViewController+Notifications.swift

+    }
+
+    /// Adds the lead image view to the web view's scroll view and configures the associated constraints
+    func setupLeadImageView() {


This one would make more sense in your +LeadImage extension.

tonisevener · 2024-08-26T20:00:50Z

Wikipedia/Code/ArticleViewController+Notifications.swift

+    func setupPageContentServiceJavaScriptInterface(with completion: @escaping () -> Void) {
+        guard let siteURL = articleURL.wmf_site else {
+            DDLogError("Missing site for \(articleURL)")
+            showGenericError()
+            return
+        }
+
+        // Need user groups to let the Page Content Service know if the page is editable for this user
+        authManager.getLoggedInUser(for: siteURL) { (result) in
+            assert(Thread.isMainThread)
+            switch result {
+            case .success(let user):
+                self.setupPageContentServiceJavaScriptInterface(with: user?.groups ?? [])
+            case .failure:
+                DDLogError("Error getting userinfo for \(siteURL)")
+                self.setupPageContentServiceJavaScriptInterface(with: [])
+            }
+            completion()
+        }
+    }
+
+    func setupPageContentServiceJavaScriptInterface(with userGroups: [String]) {
+        let areTablesInitiallyExpanded = altTextExperimentViewModel != nil ? true : UserDefaults.standard.wmf_isAutomaticTableOpeningEnabled
+
+        messagingController.shouldAttemptToShowArticleAsLivingDoc = articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc
+
+        messagingController.setup(with: webView, languageCode: articleLanguageCode, theme: theme, layoutMargins: articleMargins, leadImageHeight: leadImageHeight, areTablesInitiallyExpanded: areTablesInitiallyExpanded, userGroups: userGroups)
+    }
+
+    func setupToolbar() {
+        enableToolbar()
+        toolbarController.apply(theme: theme)
+        toolbarController.setSavedState(isSaved: article.isAnyVariantSaved)
+        setToolbarHidden(false, animated: false)
+    }


There are several setup-y method names, here and elsewhere. Maybe they could live in their own +Setup extensions file?

tonisevener · 2024-08-26T20:10:50Z

Wikipedia/Code/ArticleViewController+ArticleStateRestoration.swift

@@ -0,0 +1,67 @@
+//~~~**DELETE THIS HEADER**~~~


Need this header deleted. At the time this default was made, Xcode didn't support the ability to have an empty header when a new file was created. It's possible that has since changed.

Right now it adds DELETE THIS HEADER, and there's a git commit hook that checks if this exists in any file before allowing the commit.

ankushkushwaha · 2024-10-04T11:50:10Z

Wikipedia/Code/ArticleViewController+ArticleAsLivingDoc.swift

+    }
+
+    var shouldAttemptToShowArticleAsLivingDoc: Bool {
+        return articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc


Suggested change

return articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc

articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc

ankushkushwaha · 2024-10-04T11:50:28Z

Wikipedia/Code/ArticleViewController+ArticleAsLivingDoc.swift

+
+

Suggested change

ankushkushwaha · 2024-10-30T05:17:28Z

Wikipedia/Code/ArticleViewController+ArticleAsLivingDoc.swift

+    }
+
+    var shouldShowArticleAsLivingDoc: Bool {
+        return articleAsLivingDocController.shouldShowArticleAsLivingDoc


Suggested change

return articleAsLivingDocController.shouldShowArticleAsLivingDoc

articleAsLivingDocController.shouldShowArticleAsLivingDoc

ankushkushwaha · 2024-10-30T05:17:37Z

Wikipedia/Code/ArticleViewController+ArticleAsLivingDoc.swift

+    }
+
+    var livingDocSurveyLinkState: ArticleAsLivingDocSurveyLinkState {
+        return articleAsLivingDocController.surveyLinkState


Suggested change

return articleAsLivingDocController.surveyLinkState

articleAsLivingDocController.surveyLinkState

ankushkushwaha · 2024-10-30T05:21:05Z

Wikipedia/Code/ArticleViewController+ArticleLoad.swift

+        }
+    }
+
+    internal func presentAltTextTooltipsIfNecessary(force: Bool = false) {


Do we need to specify internal explicitly? As swift has all method as internal by default.

Refactor out ArticleViewController into extensions

458ff72

l-olson1214 requested review from tonisevener and mazevedofs and removed request for tonisevener August 26, 2024 14:40

tonisevener requested changes Aug 26, 2024

View reviewed changes

ankushkushwaha reviewed Oct 4, 2024

View reviewed changes

ankushkushwaha reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor out ArticleViewController into extensions #4942

Refactor out ArticleViewController into extensions #4942

l-olson1214 commented Aug 26, 2024

tonisevener left a comment

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

tonisevener Aug 26, 2024

ankushkushwaha Oct 4, 2024

ankushkushwaha Oct 4, 2024

ankushkushwaha Oct 30, 2024

ankushkushwaha Oct 30, 2024

ankushkushwaha Oct 30, 2024

	return articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc
	articleAsLivingDocController.shouldAttemptToShowArticleAsLivingDoc

	return articleAsLivingDocController.shouldShowArticleAsLivingDoc
	articleAsLivingDocController.shouldShowArticleAsLivingDoc

	return articleAsLivingDocController.surveyLinkState
	articleAsLivingDocController.surveyLinkState

Refactor out ArticleViewController into extensions #4942

Are you sure you want to change the base?

Refactor out ArticleViewController into extensions #4942

Conversation

l-olson1214 commented Aug 26, 2024

Notes

Test Steps

Screenshots/Videos

tonisevener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment