Merge pull request #122 from tidal-music/alberto/always-initialize-of…

…flineengine

[Player] Make OfflineEngine non-optional

Sources/Player/Common/FeatureFlags/FeatureFlagProvider+Standard.swift

@@ -7,7 +7,6 @@ public extension FeatureFlagProvider {
 		shouldSendEventsInDeinit: { true },
 		shouldUseImprovedCaching: { false },
 		shouldPauseAndPlayAroundSeek: { false },
-		shouldNotPerformActionAtItemEnd: { false },
-		isOfflineEngineEnabled: { false }
+		shouldNotPerformActionAtItemEnd: { false }
 	)
 }

Sources/Player/Common/FeatureFlags/FeatureFlagProvider.swift

@@ -5,23 +5,20 @@ public struct FeatureFlagProvider {
 	public var shouldUseImprovedCaching: () -> Bool
 	public var shouldPauseAndPlayAroundSeek: () -> Bool
 	public var shouldNotPerformActionAtItemEnd: () -> Bool
-	public var isOfflineEngineEnabled: () -> Bool
 
 	public init(
 		shouldUseEventProducer: @escaping () -> Bool,
 		isContentCachingEnabled: @escaping () -> Bool,
 		shouldSendEventsInDeinit: @escaping () -> Bool,
 		shouldUseImprovedCaching: @escaping () -> Bool,
 		shouldPauseAndPlayAroundSeek: @escaping () -> Bool,
-		shouldNotPerformActionAtItemEnd: @escaping () -> Bool,
-		isOfflineEngineEnabled: @escaping () -> Bool
+		shouldNotPerformActionAtItemEnd: @escaping () -> Bool
 	) {
 		self.shouldUseEventProducer = shouldUseEventProducer
 		self.isContentCachingEnabled = isContentCachingEnabled
 		self.shouldSendEventsInDeinit = shouldSendEventsInDeinit
 		self.shouldUseImprovedCaching = shouldUseImprovedCaching
 		self.shouldPauseAndPlayAroundSeek = shouldPauseAndPlayAroundSeek
 		self.shouldNotPerformActionAtItemEnd = shouldNotPerformActionAtItemEnd
-		self.isOfflineEngineEnabled = isOfflineEngineEnabled
 	}
 }

Sources/Player/Mocks/Common/FeatureFlags/FeatureFlagProvider+Mock.swift

@@ -7,7 +7,6 @@ public extension FeatureFlagProvider {
 		shouldSendEventsInDeinit: { true },
 		shouldUseImprovedCaching: { false },
 		shouldPauseAndPlayAroundSeek: { false },
-		shouldNotPerformActionAtItemEnd: { false },
-		isOfflineEngineEnabled: { false }
+		shouldNotPerformActionAtItemEnd: { false }
 	)
 }

Sources/Player/Player.swift

@@ -21,10 +21,10 @@ public final class Player {
 	private(set) var playerEngine: PlayerEngine
 
 	@Atomic
-	private(set) var offlineStorage: OfflineStorage?
+	private(set) var offlineStorage: OfflineStorage
 
 	@Atomic
-	private(set) var offlineEngine: OfflineEngine?
+	private(set) var offlineEngine: OfflineEngine
 
 	// MARK: - Properties
 
@@ -61,15 +61,15 @@ public final class Player {
 		queue: OperationQueue,
 		urlSession: URLSession,
 		configuration: Configuration,
-		offlineStorage: OfflineStorage?,
+		offlineStorage: OfflineStorage,
 		djProducer: DJProducer,
 		playerEventSender: PlayerEventSender,
 		fairplayLicenseFetcher: FairPlayLicenseFetcher,
 		streamingPrivilegesHandler: StreamingPrivilegesHandler,
 		networkMonitor: NetworkMonitor,
 		notificationsHandler: NotificationsHandler,
 		playerEngine: PlayerEngine,
-		offlineEngine: OfflineEngine?,
+		offlineEngine: OfflineEngine,
 		featureFlagProvider: FeatureFlagProvider,
 		externalPlayers: [GenericMediaPlayer.Type],
 		credentialsProvider: CredentialsProvider
@@ -122,6 +122,10 @@ public extension Player {
 			return nil
 		}
 
+		guard let offlineStorage = Player.initializedOfflineStorage() else {
+			return nil
+		}
+
 		Time.initialise()
 
 		// When we have logging enabled, we create a logger to be used inside Player module and set it in PlayerWorld.
@@ -175,25 +179,16 @@ public extension Player {
 			queue: listenerQueue
 		)
 
-		// For now, OfflineStorage and OfflineEngine can be optional.
-		// Once the functionality is finalized, Player should not work with a missing OfflineEngine.
-		var offlineStorage: OfflineStorage?
-		var offlineEngine: OfflineEngine?
-		if featureFlagProvider.isOfflineEngineEnabled() {
-			offlineStorage = Player.initializedOfflineStorage()
-			if let offlineStorage {
-				offlineEngine = Player.newOfflineEngine(
-					offlineStorage,
-					configuration,
-					playerEventSender,
-					networkMonitor,
-					fairplayLicenseFetcher,
-					featureFlagProvider,
-					credentialsProvider,
-					notificationsHandler
-				)
-			}
-		}
+		let offlineEngine = Player.newOfflineEngine(
+			offlineStorage,
+			configuration,
+			playerEventSender,
+			networkMonitor,
+			fairplayLicenseFetcher,
+			featureFlagProvider,
+			credentialsProvider,
+			notificationsHandler
+		)
 
 		let playerEngine = Player.newPlayerEngine(
 			sharedPlayerURLSession,
@@ -353,8 +348,7 @@ public extension Player {
 	///   - mediaProduct: The media product to offline
 	/// - Returns: True if an offline job is created, false otherwise.
 	func offline(mediaProduct: MediaProduct) -> Bool {
-		initializeOfflineEngineIfNeeded()
-		return offlineEngine?.offline(mediaProduct: mediaProduct) ?? false
+		offlineEngine.offline(mediaProduct: mediaProduct)
 	}
 
 	/// Deletes an offlined media product. No difference is made between queued, executing or done offlines. Everything is removed
@@ -364,16 +358,14 @@ public extension Player {
 	///   - mediaProduct: Media product to delete offline for
 	/// - Returns: True if a delete job is created, False otherwise.
 	func deleteOffline(mediaProduct: MediaProduct) -> Bool {
-		initializeOfflineEngineIfNeeded()
-		return offlineEngine?.deleteOffline(mediaProduct: mediaProduct) ?? false
+		offlineEngine.deleteOffline(mediaProduct: mediaProduct)
 	}
 
 	/// All offlined media products will be deleted. All queued, executing and done offlines will be deleted.
 	/// Async and thread safe. If returns true, progress can be tracked via AllOfflinesDeletedMessage.
 	/// - Returns: True if a delete all offlines job is created, False otherwise.
 	func deleteAllOfflines() -> Bool {
-		initializeOfflineEngineIfNeeded()
-		return offlineEngine?.deleteAllOfflinedMediaProducts() ?? false
+		offlineEngine.deleteAllOfflinedMediaProducts()
 	}
 
 	/// Returns offline state of a media product.
@@ -382,8 +374,7 @@ public extension Player {
 	///   - mediaProduct: Media product to gett offline state for.
 	/// - Returns: The state mediaProduct is in.
 	func getOfflineState(mediaProduct: MediaProduct) -> OfflineState {
-		initializeOfflineEngineIfNeeded()
-		return offlineEngine?.getOfflineState(mediaProduct: mediaProduct) ?? .NOT_OFFLINED
+		offlineEngine.getOfflineState(mediaProduct: mediaProduct)
 	}
 
 	func startDjSession(title: String) {
@@ -405,44 +396,11 @@ public extension Player {
 }
 
 private extension Player {
-	/// We need this for now in case the feature flag is enabled after the bootstrap process
-	func initializeOfflineEngineIfNeeded() {
-		if featureFlagProvider.isOfflineEngineEnabled(), offlineEngine == nil {
-			if offlineStorage == nil {
-				offlineStorage = Player.initializedOfflineStorage()
-			}
-			if let offlineStorage {
-				offlineEngine = Player.newOfflineEngine(
-					offlineStorage,
-					configuration,
-					playerEventSender,
-					networkMonitor,
-					fairplayLicenseFetcher,
-					featureFlagProvider,
-					credentialsProvider,
-					notificationsHandler
-				)
-			}
-		}
-	}
-
 	func instantiatedPlayerEngine(_ notificationsHandler: NotificationsHandler?) -> PlayerEngine {
-		var controlledOfflineStorage: OfflineStorage?
-		if offlineStorage != nil, !featureFlagProvider.isOfflineEngineEnabled() {
-			// We use this to control the case where the feature flag is disabled after bootstraping the player
-			controlledOfflineStorage = nil
-		} else if offlineStorage == nil, featureFlagProvider.isOfflineEngineEnabled() {
-			// We use this to control the case where the feature flag is enabled after bootstraping the player
-			offlineStorage = Player.initializedOfflineStorage()
-			controlledOfflineStorage = offlineStorage
-		} else {
-			controlledOfflineStorage = offlineStorage
-		}
-
-		return Player.newPlayerEngine(
+		Player.newPlayerEngine(
 			playerURLSession,
 			configuration,
-			controlledOfflineStorage,
+			offlineStorage,
 			djProducer,
 			fairplayLicenseFetcher,
 			networkMonitor,

Tests/PlayerTests/FeatureFlagProviderTests.swift

@@ -5,56 +5,13 @@ import XCTest
 
 final class FeatureFlagProviderTests: XCTestCase {
 	private var featureFlagProvider: FeatureFlagProvider!
-	private var shouldUseOfflineEngine: Bool = false
 
 	override func setUp() {
 		Player.shared = nil
-
 		featureFlagProvider = FeatureFlagProvider.mock
-		featureFlagProvider.isOfflineEngineEnabled = {
-			self.shouldUseOfflineEngine
-		}
-	}
-
-	func testOffliningIsNotInitialized() throws {
-		shouldUseOfflineEngine = false
-
-		let playerInstance = Player.bootstrap(
-			playerListener: PlayerListenerMock(),
-			offlineEngineListener: OfflineEngineListenerMock(),
-			listenerQueue: DispatchQueue(label: "com.tidal.queue.for.testing"),
-			featureFlagProvider: featureFlagProvider,
-			credentialsProvider: CredentialsProviderMock(),
-			eventSender: EventSenderMock()
-		)
-
-		let player = try XCTUnwrap(playerInstance)
-		XCTAssertNotNil(player.playerEngine)
-		XCTAssertNil(player.offlineStorage)
-		XCTAssertNil(player.offlineEngine)
 	}
 
 	func testOffliningIsInitialized() throws {
-		shouldUseOfflineEngine = true
-
-		let playerInstance = Player.bootstrap(
-			playerListener: PlayerListenerMock(),
-			offlineEngineListener: OfflineEngineListenerMock(),
-			listenerQueue: DispatchQueue(label: "com.tidal.queue.for.testing"),
-			featureFlagProvider: featureFlagProvider,
-			credentialsProvider: CredentialsProviderMock(),
-			eventSender: EventSenderMock()
-		)
-
-		let player = try XCTUnwrap(playerInstance)
-		XCTAssertNotNil(player.playerEngine)
-		XCTAssertNotNil(player.offlineStorage)
-		XCTAssertNotNil(player.offlineEngine)
-	}
-
-	func testOffliningIsInitializedAfterBootstrap() throws {
-		shouldUseOfflineEngine = false
-
 		let playerInstance = Player.bootstrap(
 			playerListener: PlayerListenerMock(),
 			offlineEngineListener: OfflineEngineListenerMock(),
@@ -65,10 +22,6 @@ final class FeatureFlagProviderTests: XCTestCase {
 		)
 
 		let player = try XCTUnwrap(playerInstance)
-
-		shouldUseOfflineEngine = true
-		_ = player.offline(mediaProduct: .mock())
-
 		XCTAssertNotNil(player.playerEngine)
 		XCTAssertNotNil(player.offlineStorage)
 		XCTAssertNotNil(player.offlineEngine)

Tests/PlayerTests/Mocks/OfflineEngine/Internal/Downloader+Mock.swift

@@ -0,0 +1,16 @@
+import Foundation
+@testable import Player
+
+extension Downloader {
+	static func mock(
+		playbackInfoFetcher: PlaybackInfoFetcher = .mock(),
+		fairPlayLicenseFetcher: FairPlayLicenseFetcher = .mock(),
+		networkMonitor: NetworkMonitor = NetworkMonitorMock()
+	) -> Downloader {
+		Downloader(
+			playbackInfoFetcher: playbackInfoFetcher,
+			fairPlayLicenseFetcher: fairPlayLicenseFetcher,
+			networkMonitor: networkMonitor
+		)
+	}
+}

Tests/PlayerTests/Mocks/OfflineEngine/OfflineEngine+Mock.swift

@@ -0,0 +1,18 @@
+import Foundation
+@testable import Player
+
+extension OfflineEngine {
+	static func mock(
+		downloader: Downloader = .mock(),
+		storage: OfflineStorage,
+		playerEventSender: PlayerEventSender = PlayerEventSenderMock(),
+		notificationsHandler: NotificationsHandler = .mock()
+	) -> OfflineEngine {
+		OfflineEngine(
+			downloader: downloader,
+			offlineStorage: storage,
+			playerEventSender: playerEventSender,
+			notificationsHandler: notificationsHandler
+		)
+	}
+}

Tests/PlayerTests/Player/PlayerTests.swift

@@ -1,6 +1,6 @@
 import Auth
-@testable import Player
 import GRDB
+@testable import Player
 import XCTest
 
 // MARK: - PlayerTests
@@ -9,7 +9,7 @@ final class PlayerTests: XCTestCase {
 	private var player: Player!
 	private var playerEventSender: PlayerEventSenderMock!
 	private var dbQueue: DatabaseQueue!
-	
+
 	override func setUpWithError() throws {
 		PlayerWorld = PlayerWorldClient.mock(developmentFeatureFlagProvider: DevelopmentFeatureFlagProvider.mock)
 
@@ -50,6 +50,11 @@ final class PlayerTests: XCTestCase {
 		)
 
 		let notificationsHandler = NotificationsHandler.mock()
+		let offlineEngine = OfflineEngine.mock(
+			storage: storage,
+			playerEventSender: playerEventSender,
+			notificationsHandler: notificationsHandler
+		)
 		let playerEngine = PlayerEngine.mock(httpClient: httpClient)
 
 		player = Player(
@@ -64,7 +69,7 @@ final class PlayerTests: XCTestCase {
 			networkMonitor: networkMonitor,
 			notificationsHandler: notificationsHandler,
 			playerEngine: playerEngine,
-			offlineEngine: nil,
+			offlineEngine: offlineEngine,
 			featureFlagProvider: .mock,
 			externalPlayers: [],
 			credentialsProvider: CredentialsProviderMock()