diff --git a/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift b/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift index ba83a4fa445b..6f074070a882 100644 --- a/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift +++ b/Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift @@ -19,16 +19,8 @@ protocol TopSitesDataAdaptor { /// In other words, the number of rows shown depends on the actual data and the user preference. var numberOfRows: Int { get } - /// Get top sites data, already calculated and ready to be shown to the user + /// Get top sites data func getTopSitesData() -> [TopSite] - - /// Calculate top site data - /// This calculation is dependent on the number of tiles per row that is shown in the user interface. - /// Top sites are composed of pinned sites, history, Contiles and Google top site. - /// Google top site is always first, then comes the contiles, pinned sites and history top sites. - /// We only add Google top site or Contiles if number of pins doesn't exceeds the available number shown of tiles. - /// - Parameter numberOfTilesPerRow: The number of tiles per row shown to the user - func recalculateTopSiteData(for numberOfTilesPerRow: Int) } class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable { @@ -40,6 +32,7 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable { private var historySites: [Site] = [] private var contiles: [Contile] = [] + private let maxTopSites = 4 * 14 // Max rows * max tiles on the largest screen plus some padding var notificationCenter: NotificationProtocol weak var delegate: TopSitesManagerDelegate? private let topSiteHistoryManager: TopSiteHistoryManager @@ -82,12 +75,18 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable { } func getTopSitesData() -> [TopSite] { + recalculateTopSiteData() return topSites } - func recalculateTopSiteData(for numberOfTilesPerRow: Int) { + /// Calculate top site data + /// This calculation is dependent on the number of tiles per row that is shown in the user interface. + /// Top sites are composed of pinned sites, history, Contiles and Google top site. + /// Google top site is always first, then comes the contiles, pinned sites and history top sites. + /// We only add Google top site or Contiles if number of pins doesn't exceeds the available number shown of tiles. + private func recalculateTopSiteData() { var sites = historySites - let availableSpaceCount = getAvailableSpaceCount(numberOfTilesPerRow: numberOfTilesPerRow) + let availableSpaceCount = getAvailableSpaceCount(maxTopSites: maxTopSites) let shouldAddGoogle = shouldAddGoogle(availableSpaceCount: availableSpaceCount) // Add Sponsored tile @@ -115,7 +114,7 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable { loadTopSites() dispatchGroup.notify(queue: dataQueue) { [weak self] in - self?.recalculateTopSiteData(for: TopSitesDataAdaptorImplementation.defaultTopSitesRowCount) + self?.recalculateTopSiteData() self?.delegate?.didLoadNewData() dataLoadingCompletion?() } @@ -151,10 +150,9 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable { /// Get available space count for the sponsored tiles and Google tiles /// - Parameter numberOfTilesPerRow: Comes from top sites view model and accounts for different layout (landscape, portrait, iPhone, iPad, etc). /// - Returns: The available space count for the rest of the calculation - private func getAvailableSpaceCount(numberOfTilesPerRow: Int) -> Int { + private func getAvailableSpaceCount(maxTopSites: Int) -> Int { let pinnedSiteCount = countPinnedSites(sites: historySites) - let totalNumberOfShownTiles = numberOfTilesPerRow * numberOfRows - return totalNumberOfShownTiles - pinnedSiteCount + return maxTopSites - pinnedSiteCount } // The number of rows the user wants. diff --git a/Client/Frontend/Home/TopSites/TopSitesViewModel.swift b/Client/Frontend/Home/TopSites/TopSitesViewModel.swift index 90ca28b0f964..d2a2f5fe9c0c 100644 --- a/Client/Frontend/Home/TopSites/TopSitesViewModel.swift +++ b/Client/Frontend/Home/TopSites/TopSitesViewModel.swift @@ -22,9 +22,11 @@ class TopSitesViewModel { private let profile: Profile private var sentImpressionTelemetry = [String: Bool]() + private var unfilteredTopSites: [TopSite] = [] private var topSites: [TopSite] = [] private let dimensionManager: TopSitesDimension private var numberOfItems: Int = 0 + private var numberOfRows: Int = 0 private let topSitesDataAdaptor: TopSitesDataAdaptor private let topSiteHistoryManager: TopSiteHistoryManager @@ -165,7 +167,7 @@ extension TopSitesViewModel: HomepageViewModelProtocol, FeatureFlaggable { let interface = TopSitesUIInterface(trait: traitCollection, availableWidth: size.width) let sectionDimension = dimensionManager.getSectionDimension(for: topSites, - numberOfRows: topSitesDataAdaptor.numberOfRows, + numberOfRows: numberOfRows, interface: interface) let group = NSCollectionLayoutGroup.horizontal(layoutSize: groupSize, subitem: item, @@ -194,11 +196,14 @@ extension TopSitesViewModel: HomepageViewModelProtocol, FeatureFlaggable { let interface = TopSitesUIInterface(trait: traitCollection, availableWidth: size.width) let sectionDimension = dimensionManager.getSectionDimension(for: topSites, - numberOfRows: topSitesDataAdaptor.numberOfRows, + numberOfRows: numberOfRows, interface: interface) - topSitesDataAdaptor.recalculateTopSiteData(for: sectionDimension.numberOfTilesPerRow) - topSites = topSitesDataAdaptor.getTopSitesData() numberOfItems = sectionDimension.numberOfRows * sectionDimension.numberOfTilesPerRow + topSites = unfilteredTopSites + if numberOfItems < unfilteredTopSites.count { + let range = numberOfItems.. Frequency func testDuplicates_SponsoredTileHasPrecedenceOnFrequencyTiles() { let subject = createSubject(expectedContileResult: ContileResult.success([ContileProviderMock.duplicateTile])) - - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertTrue(data[0].isGoogleURL) XCTAssertEqual(data[1].title, ContileProviderMock.duplicateTile.name) XCTAssertTrue(data[1].isSponsoredTile) @@ -320,9 +302,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { let subject = createSubject(addPinnedSiteCount: 1, expectedContileResult: ContileResult.success([ContileProviderMock.pinnedDuplicateTile])) - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertTrue(data[0].isGoogleURL) XCTAssertFalse(data[1].isSponsoredTile) XCTAssertTrue(data[1].isPinned) @@ -334,10 +315,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { func testDuplicates_PinnedTilesHasPrecedenceOnFrequencyTiles() { let expectedPinnedURL = String(format: ContileProviderMock.url, "0") let subject = createSubject(addPinnedSiteCount: 1, siteCount: 3, duplicatePinnedSiteURL: true) - - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertEqual(data.count, 4, "Should have 3 sites and 1 pinned") XCTAssertTrue(data[0].isGoogleURL) @@ -361,9 +340,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { func testDuplicates_PinnedTilesOfSameDomainIsntDeduped() { let subject = createSubject(addPinnedSiteCount: 2, siteCount: 0) - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertEqual(data.count, 3, "Should have google site and 2 pinned sites") XCTAssertTrue(data[0].isGoogleURL) @@ -388,8 +366,6 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { isCustomEngine: false) add(searchEngine: googleSearchEngine) let subject = createSubject() - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() XCTAssertTrue(data[0].isGoogleURL) @@ -405,9 +381,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { isCustomEngine: false) add(searchEngine: pinnedTileSearchEngine) let subject = createSubject(addPinnedSiteCount: 3) - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertEqual(data.count, 14) XCTAssertTrue(data[0].isPinned) } @@ -421,8 +396,6 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { isCustomEngine: false) add(searchEngine: historyTileSearchEngine) let subject = createSubject() - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() XCTAssertEqual(data[1].title, "A title 0") @@ -438,10 +411,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable { add(searchEngine: sponsoredTileSearchEngine) let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 1) let subject = createSubject(expectedContileResult: ContileResult.success(expectedContileResult)) - - subject.recalculateTopSiteData(for: 6) - let data = subject.getTopSitesData() + XCTAssertTrue(data[0].isGoogleURL) XCTAssertFalse(data[1].isSponsoredTile) }