Commit 9e7e932d authored by Sebastian Kaspari's avatar Sebastian Kaspari
Browse files

Issue #3432: Add AddMultipleTabsAction to add multiple tabs at once to BrowserStore.

parent a8983a8d
......@@ -140,14 +140,15 @@ class LegacySessionManager(
addInternal(session, selected, engineSession, parent = parent, viaRestore = false)
}
@Suppress("LongParameterList", "ComplexMethod")
@Suppress("LongParameterList", "ComplexMethod", "LongMethod")
private fun addInternal(
session: Session,
selected: Boolean = false,
engineSession: EngineSession? = null,
engineSessionState: EngineSessionState? = null,
parent: Session? = null,
viaRestore: Boolean = false
viaRestore: Boolean = false,
notifyObservers: Boolean = true
) = synchronized(values) {
if (values.find { it.id == session.id } != null) {
throw IllegalArgumentException("Session with same ID already exists")
......@@ -184,7 +185,7 @@ class LegacySessionManager(
// If session is being added via restore, skip notification and auto-selection.
// Restore will handle these actions as appropriate.
if (viaRestore) {
if (viaRestore || !notifyObservers) {
return@synchronized
}
......@@ -209,16 +210,18 @@ class LegacySessionManager(
}
sessions.forEach {
addInternal(
session = it,
viaRestore = true
)
addInternal(session = it, notifyObservers = false)
}
if (selectedIndex == NO_SELECTION) {
// If there is no selection yet then select first non-private session
sessions.find { !it.private }?.let { select(it) }
}
// This is a performance hack to not call `onSessionAdded` for every added session. Normally
// we'd add a new observer method for that. But since we are trying to migrate away from
// browser-session, we should not add a new one and force the apps to use it.
notifyObservers { onSessionsRestored() }
}
/**
......
......@@ -126,7 +126,7 @@ class SessionManager(
}
sessions.find { it.parentId != null }?.let {
throw IllegalArgumentException("Bulk adding of ")
throw IllegalArgumentException("Bulk adding of sessions with a parent is not supported. ")
}
// Add store to each Session so that it can dispatch actions whenever it changes.
......
......@@ -1225,4 +1225,23 @@ class SessionManagerTest {
assertEquals(4, manager.sessions.size)
assertEquals("https://www.mozilla.org", manager.selectedSessionOrThrow.url)
}
@Test
fun `WHEN adding multiple sessions THEN observer is notified`() {
val manager = SessionManager(engine = mock())
val observer: SessionManager.Observer = mock()
manager.register(observer)
val sessions = listOf(
Session("htttps://getpocket.com"),
Session("https://www.firefox.com", private = true)
)
verify(observer, never()).onSessionsRestored()
manager.add(sessions)
verify(observer).onSessionsRestored()
}
}
......@@ -34,13 +34,18 @@ sealed class SystemAction : BrowserAction() {
*/
sealed class TabListAction : BrowserAction() {
/**
* Adds a new [TabSessionState] to the list.
* Adds a new [TabSessionState] to the [BrowserState.tabs] list.
*
* @property tab the [TabSessionState] to add
* @property select whether or not to the tab should be selected.
*/
data class AddTabAction(val tab: TabSessionState, val select: Boolean = false) : TabListAction()
/**
* Adds multiple [TabSessionState] objects to the [BrowserState.tabs] list.
*/
data class AddMultipleTabsAction(val tabs: List<TabSessionState>) : TabListAction()
/**
* Marks the [TabSessionState] with the given [tabId] as selected tab.
*
......
......@@ -44,6 +44,23 @@ internal object TabListReducer {
)
}
is TabListAction.AddMultipleTabsAction -> {
action.tabs.forEach { requireUniqueTab(state, it) }
action.tabs.find { tab -> tab.parentId != null }?.let {
throw IllegalArgumentException("Adding multiple tabs with a parent id is not supported")
}
state.copy(
tabs = state.tabs + action.tabs,
selectedTabId = if (state.selectedTabId == null) {
action.tabs.find { tab -> !tab.content.private }?.id
} else {
state.selectedTabId
}
)
}
is TabListAction.SelectTabAction -> {
state.copy(selectedTabId = action.tabId)
}
......
......@@ -11,6 +11,7 @@ import mozilla.components.browser.state.state.createTab
import mozilla.components.browser.state.store.BrowserStore
import mozilla.components.support.test.ext.joinBlocking
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
......@@ -648,4 +649,96 @@ class TabListActionTest {
assertEquals(1, store.state.customTabs.size)
assertEquals("a1", store.state.customTabs.last().id)
}
@Test
fun `AddMultipleTabsAction - Adds multiple tabs and updates selection`() {
val store = BrowserStore()
assertEquals(0, store.state.tabs.size)
assertNull(store.state.selectedTabId)
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(
createTab(id = "a", url = "https://www.mozilla.org", private = false),
createTab(id = "b", url = "https://www.firefox.com", private = true)
))
).joinBlocking()
assertEquals(2, store.state.tabs.size)
assertEquals("https://www.mozilla.org", store.state.tabs[0].content.url)
assertEquals("https://www.firefox.com", store.state.tabs[1].content.url)
assertNotNull(store.state.selectedTabId)
assertEquals("a", store.state.selectedTabId)
}
@Test
fun `AddMultipleTabsAction - Adds multiple tabs and does not update selection if one exists already`() {
val store = BrowserStore(BrowserState(
tabs = listOf(createTab(id = "z", url = "https://getpocket.com")),
selectedTabId = "z"
))
assertEquals(1, store.state.tabs.size)
assertEquals("z", store.state.selectedTabId)
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(
createTab(id = "a", url = "https://www.mozilla.org", private = false),
createTab(id = "b", url = "https://www.firefox.com", private = true)
))
).joinBlocking()
assertEquals(3, store.state.tabs.size)
assertEquals("https://getpocket.com", store.state.tabs[0].content.url)
assertEquals("https://www.mozilla.org", store.state.tabs[1].content.url)
assertEquals("https://www.firefox.com", store.state.tabs[2].content.url)
assertEquals("z", store.state.selectedTabId)
}
@Test
fun `AddMultipleTabsAction - Non private tab will be selected`() {
val store = BrowserStore()
assertEquals(0, store.state.tabs.size)
assertNull(store.state.selectedTabId)
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(
createTab(id = "a", url = "https://www.mozilla.org", private = true),
createTab(id = "b", url = "https://www.example.org", private = true),
createTab(id = "c", url = "https://www.firefox.com", private = false),
createTab(id = "d", url = "https://getpocket.com", private = true)
))
).joinBlocking()
assertEquals(4, store.state.tabs.size)
assertEquals("https://www.mozilla.org", store.state.tabs[0].content.url)
assertEquals("https://www.example.org", store.state.tabs[1].content.url)
assertEquals("https://www.firefox.com", store.state.tabs[2].content.url)
assertEquals("https://getpocket.com", store.state.tabs[3].content.url)
assertNotNull(store.state.selectedTabId)
assertEquals("c", store.state.selectedTabId)
}
@Test
fun `AddMultipleTabsAction - No tab will be selected if only private tabs are added`() {
val store = BrowserStore()
assertEquals(0, store.state.tabs.size)
assertNull(store.state.selectedTabId)
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(
createTab(id = "a", url = "https://www.mozilla.org", private = true),
createTab(id = "b", url = "https://www.example.org", private = true),
createTab(id = "c", url = "https://getpocket.com", private = true)
))
).joinBlocking()
assertEquals(3, store.state.tabs.size)
assertEquals("https://www.mozilla.org", store.state.tabs[0].content.url)
assertEquals("https://www.example.org", store.state.tabs[1].content.url)
assertEquals("https://getpocket.com", store.state.tabs[2].content.url)
assertNull(store.state.selectedTabId)
}
}
......@@ -6,12 +6,15 @@ package mozilla.components.browser.state.store
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.browser.state.action.TabListAction
import mozilla.components.browser.state.state.BrowserState
import mozilla.components.browser.state.state.createTab
import mozilla.components.lib.state.StoreException
import mozilla.components.support.test.ext.joinBlocking
import org.junit.Assert.fail
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.shadows.ShadowLooper
import java.lang.IllegalArgumentException
// These tests are in a separate class because they needs to run with
// Robolectric (different runner, slower) while all other tests only
......@@ -51,17 +54,61 @@ class BrowserStoreExceptionTest {
}
}
@Test(expected = IllegalArgumentException::class)
fun `AddMultipleTabsAction - Exception is thrown in tab with id already exists`() {
unwrapStoreExceptionAndRethrow {
val store = BrowserStore(BrowserState(
tabs = listOf(
createTab(id = "a", url = "https://www.mozilla.org")
)
))
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(
createTab(id = "a", url = "https://www.example.org")
)
)).joinBlocking()
}
}
@Test(expected = IllegalArgumentException::class)
fun `AddMultipleTabsAction - Exception is thrown if parent id is set`() {
unwrapStoreExceptionAndRethrow {
val store = BrowserStore()
val tab1 = createTab(
id = "a",
url = "https://www.mozilla.org")
val tab2 = createTab(
id = "b",
url = "https://www.firefox.com",
private = true,
parent = tab1)
store.dispatch(TabListAction.AddMultipleTabsAction(
tabs = listOf(tab1, tab2))
).joinBlocking()
}
}
private fun unwrapStoreExceptionAndRethrow(block: () -> Unit) {
try {
block()
// Wait for the main looper to process the re-thrown exception.
ShadowLooper.idleMainLooper()
fail("Did not throw StoreException")
} catch (e: StoreException) {
val cause = e.cause
if (cause != null) {
throw cause
}
} catch (e: Throwable) {
fail("Did throw a different exception $e")
}
fail("Did not throw StoreException with wrapped exception")
}
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment