Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Issue #21642: Remove in-progress media tab from homescreen #21670

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Issue #21642: Remove in-progress media tab from homescreen #21670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions app/src/main/java/org/mozilla/fenix/ext/BrowserState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,9 @@ import kotlin.math.max
fun BrowserState.asRecentTabs(): List<RecentTab> {
return mutableListOf<RecentTab>().apply {
val lastOpenedNormalTab = lastOpenedNormalTab
val inProgressMediaTab = inProgressMediaTab
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also wrap this behind a feature flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure to what extent we should be "removing it". So I opted for this solution so we can land it before the beta cut.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason I forgot about, is that we get intermittent test failures when we have feature flags here. See #21557.

Copy link
Contributor

@csadilek csadilek Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land as is. Thought about this last week and it's a trade-off. We don't want feature flags to be too fine-grained, so arguably this is a change in behaviour of an existing feature, as opposed to a feature itself. We can revert this to enable again when we want it (have time to add media controls).


lastOpenedNormalTab?.let { add(RecentTab.Tab(it)) }

if (inProgressMediaTab == lastOpenedNormalTab) {
secondToLastOpenedNormalTab?.let { add(RecentTab.Tab(it)) }
} else {
inProgressMediaTab?.let { add(RecentTab.Tab(it)) }
}

lastSearchGroup?.let { add(it) }
}
}
Expand Down
37 changes: 22 additions & 15 deletions app/src/test/java/org/mozilla/fenix/ext/BrowserStateTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,12 @@ class BrowserStateTest {

val result = browserState.asRecentTabs()

assertEquals(2, result.size)
assertEquals(1, result.size)
assertEquals(selectedTab, (result[0] as RecentTab.Tab).state)
assertEquals(mediaTab, (result[1] as RecentTab.Tab).state)
}

@Test
fun `GIVEN the selected tab is a private tab and another media tab exists WHEN asRecentTabs is called THEN return a list of the last normal tab and the media tab`() {
fun `GIVEN the selected tab is a private tab and another tab exists WHEN asRecentTabs is called THEN return a list of the last normal tab`() {
val lastAccessedNormalTab = createTab(url = "url2", id = "2", lastAccess = 2)
val selectedPrivateTab = createTab(url = "url", id = "1", lastAccess = 1, private = true)
val mediaTab = createTab(
Expand All @@ -110,13 +109,12 @@ class BrowserStateTest {

val result = browserState.asRecentTabs()

assertEquals(2, result.size)
assertEquals(1, result.size)
assertEquals(lastAccessedNormalTab, (result[0] as RecentTab.Tab).state)
assertEquals(mediaTab, (result[1] as RecentTab.Tab).state)
}

@Test
fun `GIVEN the selected tab is a private tab and the media tab is the last accessed normal tab WHEN asRecentTabs is called THEN return a list of the media tab and the second-to-last normal tab`() {
fun `GIVEN the selected tab is a private tab and the media tab is the last accessed normal tab WHEN asRecentTabs is called THEN return a list of the second-to-last normal tab`() {
val selectedPrivateTab = createTab(url = "url", id = "1", lastAccess = 1, private = true)
val normalTab = createTab(url = "url2", id = "2", lastAccess = 2)
val mediaTab = createTab(
Expand All @@ -130,7 +128,7 @@ class BrowserStateTest {

val result = browserState.asRecentTabs()

assertEquals(2, result.size)
assertEquals(1, result.size)
assertEquals(mediaTab, (result[0] as RecentTab.Tab).state)
}

Expand Down Expand Up @@ -196,8 +194,17 @@ class BrowserStateTest {
referrerUrl = "https://www.mozilla.org"
)
)
val searchGroupTab2 = createTab(
url = "https://www.mozilla.org",
id = "5",
historyMetadata = HistoryMetadataKey(
url = "https://www.firefox.com",
searchTerm = "Test",
referrerUrl = "https://www.mozilla.org"
)
)
val browserState = BrowserState(
tabs = listOf(mockk(relaxed = true), selectedTab, searchGroupTab),
tabs = listOf(mockk(relaxed = true), selectedTab, searchGroupTab, searchGroupTab2),
selectedTabId = selectedTab.id
)

Expand Down Expand Up @@ -226,14 +233,14 @@ class BrowserStateTest {

val result = browserState.asRecentTabs()

assertEquals(3, result.size)
assertEquals(2, result.size)
assertEquals(selectedTab, (result[0] as RecentTab.Tab).state)
assert(result[2] is RecentTab.SearchGroup)
assertEquals(searchGroupTab.historyMetadata?.searchTerm, (result[2] as RecentTab.SearchGroup).searchTerm)
assertEquals(searchGroupTab.id, (result[2] as RecentTab.SearchGroup).tabId)
assertEquals(searchGroupTab.content.url, (result[2] as RecentTab.SearchGroup).url)
assertEquals(searchGroupTab.content.thumbnail, (result[2] as RecentTab.SearchGroup).thumbnail)
assertEquals(2, (result[2] as RecentTab.SearchGroup).count)
assert(result[1] is RecentTab.SearchGroup)
assertEquals(searchGroupTab.historyMetadata?.searchTerm, (result[1] as RecentTab.SearchGroup).searchTerm)
assertEquals(searchGroupTab.id, (result[1] as RecentTab.SearchGroup).tabId)
assertEquals(searchGroupTab.content.url, (result[1] as RecentTab.SearchGroup).url)
assertEquals(searchGroupTab.content.thumbnail, (result[1] as RecentTab.SearchGroup).thumbnail)
assertEquals(2, (result[1] as RecentTab.SearchGroup).count)
}

@Test
Expand Down
10 changes: 6 additions & 4 deletions app/src/test/java/org/mozilla/fenix/home/RecentTabsListFeatureTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Ignore
import org.junit.Rule
import org.junit.Test
import org.mozilla.fenix.home.HomeFragmentAction.RecentTabsChange
Expand Down Expand Up @@ -118,6 +119,7 @@ class RecentTabsListFeatureTest {
assertEquals(1, homeStore.state.recentTabs.size)
}

@Ignore("Disabled until we want to enable this feature. See #21670.")
@Test
fun `GIVEN a valid inProgressMediaTabId and another selected tab exists WHEN the feature starts THEN dispatch both as as a recent tabs list`() {
val mediaTab = createTab(
Expand Down Expand Up @@ -146,6 +148,7 @@ class RecentTabsListFeatureTest {
assertEquals(mediaTab, (homeStore.state.recentTabs[1] as RecentTab.Tab).state)
}

@Ignore("Disabled until we want to enable this feature. See #21670.")
@Test
fun `GIVEN a valid inProgressMediaTabId exists and that is the selected tab WHEN the feature starts THEN dispatch just one tab as the recent tabs list`() {
val selectedMediaTab = createTab(
Expand Down Expand Up @@ -210,6 +213,7 @@ class RecentTabsListFeatureTest {
assertEquals(tab2, (homeStore.state.recentTabs[0] as RecentTab.Tab).state)
}

@Ignore("Disabled until we want to enable this feature. See #21670.")
@Test
fun `WHEN the browser state has an in progress media tab THEN dispatch the new recent tab list`() {
val initialMediaTab = createTab(
Expand Down Expand Up @@ -602,12 +606,10 @@ class RecentTabsListFeatureTest {
feature.start()
homeStore.waitUntilIdle()

assertEquals(3, homeStore.state.recentTabs.size)
assertEquals(2, homeStore.state.recentTabs.size)
assertTrue(homeStore.state.recentTabs[0] is RecentTab.Tab)
assertEquals(selectedTab, (homeStore.state.recentTabs[0] as RecentTab.Tab).state)
assertTrue(homeStore.state.recentTabs[1] is RecentTab.Tab)
assertEquals(mediaTab, (homeStore.state.recentTabs[1] as RecentTab.Tab).state)
val searchGroup = (homeStore.state.recentTabs[2] as RecentTab.SearchGroup)
val searchGroup = (homeStore.state.recentTabs[1] as RecentTab.SearchGroup)
assertEquals(searchGroup.searchTerm, "Test search term")
assertEquals(searchGroup.tabId, "44")
assertEquals(searchGroup.url, "https://www.mozilla.org")
Expand Down