fix double clearing on autoclear (#2666)

Task/Issue URL: https://app.asana.com/0/414709148257752/1206731143449260/f Tech Design URL: CC: Description: Refactoring to inject tabs model into the main view controller Prevents double clearing when using auto-clear + timer Also fixes [Bug: Unable to launch URL properly when auto clear enabled](https://app.asana.com/0/414235014887631/1206454582708636/f) Steps to test this PR: Test standard fire button and fire proofing usage Open some tabs, set cookies (e.g. login to a site), use fire button ensure data is cleared Open some tabs, set cookies (e.g. login to a site), add to fire proofing, ensure data except fireproofed is cleared Open some tabs, set cookies (e.g. login to a site), terminate the app and reload, ensure tab loads as expected Autoclear with Terminate Update settings to enable auto clear on terminate Open some tabs, set cookies (e.g. login to a site) Terminate the app Launch the app, ensure tabs and data are cleared Autoclear with Terminate and Delay Update settings to enable auto clear on terminate with a delay Hack the code to return true for the selected delay in the shouldClearData function Background the app Use another app (e.g news) to launch a URL in DDG (either through default browser or by share extension) Ensure tabs and data are cleared Autoclear with tabs only Repeat above tests but with tabs only selected in auto clear settings Ensure tabs are closed but data is retained
duckduckgo · Apr 4, 2024 · cebfbf5 · cebfbf5
1 parent 3251097
commit cebfbf5
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 61 deletions.
diff --git a/DuckDuckGo/AppDelegate.swift b/DuckDuckGo/AppDelegate.swift
@@ -266,7 +266,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
                 })
             }
 
+        let previewsSource = TabPreviewsSource()
         let historyManager = makeHistoryManager()
+        let tabsModel = prepareTabsModel(previewsSource: previewsSource)
 
 #if APP_TRACKING_PROTECTION
         let main = MainViewController(bookmarksDatabase: bookmarksDatabase,
@@ -275,14 +277,18 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
                                       historyManager: historyManager,
                                       syncService: syncService,
                                       syncDataProviders: syncDataProviders,
-                                      appSettings: AppDependencyProvider.shared.appSettings)
+                                      appSettings: AppDependencyProvider.shared.appSettings,
+                                      previewsSource: previewsSource,
+                                      tabsModel: tabsModel)
 #else
         let main = MainViewController(bookmarksDatabase: bookmarksDatabase,
                                       bookmarksDatabaseCleaner: syncDataProviders.bookmarksAdapter.databaseCleaner,
                                       historyManager: historyManager,
                                       syncService: syncService,
                                       syncDataProviders: syncDataProviders,
-                                      appSettings: AppDependencyProvider.shared.appSettings)
+                                      appSettings: AppDependencyProvider.shared.appSettings,
+                                      previewsSource: previewsSource,
+                                      tabsModel: tabsModel)
 #endif
 
         main.loadViewIfNeeded()
@@ -345,6 +351,27 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         return true
     }
 
+    private func prepareTabsModel(previewsSource: TabPreviewsSource = TabPreviewsSource(),
+                                  appSettings: AppSettings = AppDependencyProvider.shared.appSettings,
+                                  isDesktop: Bool = UIDevice.current.userInterfaceIdiom == .pad) -> TabsModel {
+        let isPadDevice = UIDevice.current.userInterfaceIdiom == .pad
+        let tabsModel: TabsModel
+        if AutoClearSettingsModel(settings: appSettings) != nil {
+            tabsModel = TabsModel(desktop: isPadDevice)
+            tabsModel.save()
+            previewsSource.removeAllPreviews()
+        } else {
+            if let storedModel = TabsModel.get() {
+                // Save new model in case of migration
+                storedModel.save()
+                tabsModel = storedModel
+            } else {
+                tabsModel = TabsModel(desktop: isPadDevice)
+            }
+        }
+        return tabsModel
+    }
+
     private func makeHistoryManager() -> HistoryManager {
         let historyManager = HistoryManager(privacyConfigManager: ContentBlocking.shared.privacyConfigurationManager,
                               variantManager: DefaultVariantManager(),
@@ -733,7 +760,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         }
 
         Task { @MainActor in
-            await autoClear?.applicationWillMoveToForeground()
+            // Autoclear should have happened by now
             showKeyboardIfSettingOn = false
 
             if !handleAppDeepLink(app, mainViewController, url) {

diff --git a/DuckDuckGo/AutoClear.swift b/DuckDuckGo/AutoClear.swift
@@ -33,14 +33,15 @@ class AutoClear {
     private let worker: AutoClearWorker
     private var timestamp: TimeInterval?
 
-    private lazy var appSettings = AppDependencyProvider.shared.appSettings
-    
+    private let appSettings: AppSettings
+
     var isClearingEnabled: Bool {
         return AutoClearSettingsModel(settings: appSettings) != nil
     }
 
-    init(worker: AutoClearWorker) {
+    init(worker: AutoClearWorker, appSettings: AppSettings = AppDependencyProvider.shared.appSettings) {
         self.worker = worker
+        self.appSettings = appSettings
     }
 
     @MainActor
@@ -86,8 +87,8 @@ class AutoClear {
             let timestamp = timestamp,
             shouldClearData(elapsedTime: Date().timeIntervalSince1970 - timestamp) else { return }
 
+        self.timestamp = nil
         worker.clearNavigationStack()
         await clearData()
-        self.timestamp = nil
     }
 }
diff --git a/DuckDuckGo/MainViewController.swift b/DuckDuckGo/MainViewController.swift
@@ -84,11 +84,13 @@ class MainViewController: UIViewController {
     var tabsBarController: TabsBarViewController?
     var suggestionTrayController: SuggestionTrayViewController?
 
-    var tabManager: TabManager!
-    let previewsSource = TabPreviewsSource()
+    let tabManager: TabManager
+    let previewsSource: TabPreviewsSource
     let appSettings: AppSettings
     private var launchTabObserver: LaunchTabNotification.Observer?
 
+    var doRefreshAfterClear = true
+
 #if APP_TRACKING_PROTECTION
     private let appTrackingProtectionDatabase: CoreDataDatabase
 #endif
@@ -139,7 +141,7 @@ class MainViewController: UIViewController {
 
     lazy var tabSwitcherTransition = TabSwitcherTransitionDelegate()
     var currentTab: TabViewController? {
-        return tabManager?.current(createIfNeeded: false)
+        return tabManager.current(createIfNeeded: false)
     }
 
     var searchBarRect: CGRect {
@@ -174,7 +176,9 @@ class MainViewController: UIViewController {
         historyManager: HistoryManager,
         syncService: DDGSyncing,
         syncDataProviders: SyncDataProviders,
-        appSettings: AppSettings = AppUserDefaults()
+        appSettings: AppSettings = AppUserDefaults(),
+        previewsSource: TabPreviewsSource,
+        tabsModel: TabsModel
     ) {
         self.appTrackingProtectionDatabase = appTrackingProtectionDatabase
         self.bookmarksDatabase = bookmarksDatabase
@@ -185,9 +189,17 @@ class MainViewController: UIViewController {
         self.favoritesViewModel = FavoritesListViewModel(bookmarksDatabase: bookmarksDatabase, favoritesDisplayMode: appSettings.favoritesDisplayMode)
         self.bookmarksCachingSearch = BookmarksCachingSearch(bookmarksStore: CoreDataBookmarksSearchStore(bookmarksStore: bookmarksDatabase))
         self.appSettings = appSettings
-
+        self.previewsSource = previewsSource
+
+        self.tabManager = TabManager(model: tabsModel,
+                                     previewsSource: previewsSource,
+                                     bookmarksDatabase: bookmarksDatabase,
+                                     historyManager: historyManager,
+                                     syncService: syncService)
+
         super.init(nibName: nil, bundle: nil)
-
+
+        tabManager.delegate = self
         bindFavoritesDisplayMode()
         bindSyncService()
     }
@@ -198,7 +210,9 @@ class MainViewController: UIViewController {
         historyManager: HistoryManager,
         syncService: DDGSyncing,
         syncDataProviders: SyncDataProviders,
-        appSettings: AppSettings
+        appSettings: AppSettings,
+        previewsSource: TabPreviewsSource,
+        tabsModel: TabsModel
     ) {
         self.bookmarksDatabase = bookmarksDatabase
         self.bookmarksDatabaseCleaner = bookmarksDatabaseCleaner
@@ -208,9 +222,18 @@ class MainViewController: UIViewController {
         self.favoritesViewModel = FavoritesListViewModel(bookmarksDatabase: bookmarksDatabase, favoritesDisplayMode: appSettings.favoritesDisplayMode)
         self.bookmarksCachingSearch = BookmarksCachingSearch(bookmarksStore: CoreDataBookmarksSearchStore(bookmarksStore: bookmarksDatabase))
         self.appSettings = appSettings
-
+        self.previewsSource = previewsSource
+
+        self.tabManager = TabManager(model: tabsModel,
+                                     previewsSource: previewsSource,
+                                     bookmarksDatabase: bookmarksDatabase,
+                                     historyManager: historyManager,
+                                     syncService: syncService)
+
+
         super.init(nibName: nil, bundle: nil)
 
+        tabManager.delegate = self
         bindSyncService()
     }
 #endif
@@ -264,7 +287,6 @@ class MainViewController: UIViewController {
         initTabButton()
         initMenuButton()
         initBookmarksButton()
-        configureTabManager()
         loadInitialView()
         previewsSource.prepare()
         addLaunchTabNotificationObserver()
@@ -699,40 +721,6 @@ class MainViewController: UIViewController {
         dismissOmniBar()
     }
 
-    private func configureTabManager() {
-
-        let isPadDevice = UIDevice.current.userInterfaceIdiom == .pad
-
-        let tabsModel: TabsModel
-        if let settings = AutoClearSettingsModel(settings: appSettings) {
-            // This needs to be refactored so that tabs model is injected and cleared before view did load,
-            //  but for now, ensure this happens in the right order by clearing data here too, if needed.
-            tabsModel = TabsModel(desktop: isPadDevice)
-            tabsModel.save()
-            previewsSource.removeAllPreviews()
-
-            if settings.action.contains(.clearData) {
-                Task { @MainActor in
-                    await forgetData()
-                }
-            }
-        } else {
-            if let storedModel = TabsModel.get() {
-                // Save new model in case of migration
-                storedModel.save()
-                tabsModel = storedModel
-            } else {
-                tabsModel = TabsModel(desktop: isPadDevice)
-            }
-        }
-        tabManager = TabManager(model: tabsModel,
-                                previewsSource: previewsSource,
-                                bookmarksDatabase: bookmarksDatabase,
-                                historyManager: historyManager,
-                                syncService: syncService,
-                                delegate: self)
-    }
-
     private func addLaunchTabNotificationObserver() {
         launchTabObserver = LaunchTabNotification.addObserver(handler: { [weak self] urlString in
             guard let self = self else { return }
@@ -890,6 +878,7 @@ class MainViewController: UIViewController {
                 selectTab(existing)
                 return
             } else if reuseExisting, let existing = tabManager.firstHomeTab() {
+                doRefreshAfterClear = false
                 tabManager.selectTab(existing)
                 loadUrl(url, fromExternalLink: fromExternalLink)
             } else {
@@ -2020,7 +2009,7 @@ extension MainViewController: TabDelegate {
         if currentTab == tab {
             refreshControls()
         }
-        tabManager?.save()
+        tabManager.save()
         tabsBarController?.refresh(tabsModel: tabManager.model)
         // note: model in swipeTabsCoordinator doesn't need to be updated here
         // https://app.asana.com/0/414235014887631/1206847376910045/f
@@ -2267,7 +2256,7 @@ extension MainViewController: TabSwitcherButtonDelegate {
     }
 
     func showTabSwitcher() {
-        guard let currentTab = currentTab ?? tabManager?.current(createIfNeeded: true) else {
+        guard let currentTab = currentTab ?? tabManager.current(createIfNeeded: true) else {
             fatalError("Unable to get current tab")
         }
 
@@ -2324,6 +2313,10 @@ extension MainViewController: AutoClearWorker {
     }
 
     func refreshUIAfterClear() {
+        guard doRefreshAfterClear else {
+            doRefreshAfterClear = true
+            return
+        }
         showBars()
         attachHomeScreen()
         tabsBarController?.refresh(tabsModel: tabManager.model)
@@ -2336,6 +2329,7 @@ extension MainViewController: AutoClearWorker {
         refreshUIAfterClear()
     }
 
+    @MainActor
     func forgetData() async {
         guard !clearInProgress else {
             assertionFailure("Shouldn't get called multiple times")

diff --git a/DuckDuckGo/TabManager.swift b/DuckDuckGo/TabManager.swift
@@ -35,6 +35,7 @@ class TabManager {
     private let historyManager: HistoryManager
     private let syncService: DDGSyncing
     private var previewsSource: TabPreviewsSource
+
     weak var delegate: TabDelegate?
 
     @UserDefaultsWrapper(key: .faviconTabsCacheNeedsCleanup, defaultValue: true)
@@ -45,14 +46,12 @@ class TabManager {
          previewsSource: TabPreviewsSource,
          bookmarksDatabase: CoreDataDatabase,
          historyManager: HistoryManager,
-         syncService: DDGSyncing,
-         delegate: TabDelegate) {
+         syncService: DDGSyncing) {
         self.model = model
         self.previewsSource = previewsSource
         self.bookmarksDatabase = bookmarksDatabase
         self.historyManager = historyManager
         self.syncService = syncService
-        self.delegate = delegate
         let index = model.currentIndex
         let tab = model.tabs[index]
         if tab.link != nil {

diff --git a/DuckDuckGoTests/AutoClearTests.swift b/DuckDuckGoTests/AutoClearTests.swift
@@ -49,19 +49,20 @@ class AutoClearTests: XCTestCase {
 
     private var worker: MockWorker!
     private var logic: AutoClear!
+    private var appSettings: AppSettingsMock!
+
+    override func setUp() async throws {
+        try await super.setUp()
 
-    override func setUp() {
-        super.setUp()
-
         worker = MockWorker()
-        logic = AutoClear(worker: worker)
+        appSettings = AppSettingsMock()
+        logic = AutoClear(worker: worker, appSettings: appSettings)
     }
 
     // Note: applicationDidLaunch based clearing has moved to "configureTabManager" function of
     //  MainViewController to ensure that tabs are removed before the data is cleared.
 
     func testWhenTimingIsSetToTerminationThenOnlyRestartClearsData() async {
-        let appSettings = AppUserDefaults()
         appSettings.autoClearAction = .clearData
         appSettings.autoClearTiming = .termination
 
@@ -70,10 +71,14 @@ class AutoClearTests: XCTestCase {
 
         XCTAssertEqual(worker.clearNavigationStackInvocationCount, 0)
         XCTAssertEqual(worker.forgetDataInvocationCount, 0)
+
+        await logic.applicationWillMoveToForeground()
+
+        XCTAssertEqual(worker.clearNavigationStackInvocationCount, 0)
+        XCTAssertEqual(worker.forgetDataInvocationCount, 0)
     }
 
     func testWhenDesiredTimingIsSetThenDataIsClearedOnceTimeHasElapsed() async {
-        let appSettings = AppUserDefaults()
         appSettings.autoClearAction = .clearData
 
         let cases: [AutoClearSettingsModel.Timing: TimeInterval] = [.delay5min: 5 * 60,