Closes #3567: BrowserStore/SessionManager: Make result of "nearby" tab selection match.

components/browser/session/src/main/java/mozilla/components/browser/session/LegacySessionManager.kt

@@ -12,6 +12,7 @@ import mozilla.components.concept.engine.EngineSessionState
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
 import kotlin.math.max
+import kotlin.math.min
 
 /**
  * This class provides access to a centralized registry of all active sessions.
@@ -398,20 +399,49 @@ class LegacySessionManager(
         return findNearbySession(index) { !it.isCustomTabSession() }
     }
 
+    /**
+     * Tries to find a [Session] that is near [index] and satisfies the [predicate].
+     *
+     * Its implementation is synchronized with the behavior in `TabListReducer` of the `browser-state` component.
+     */
+    @Suppress("LongMethod") // Yes, this method is pretty long. I hope we can delete it soon.
     private fun findNearbySession(index: Int, predicate: (Session) -> Boolean): Int {
-        val maxSteps = max(values.lastIndex - index, index)
+        // Okay, this is a bit stupid and complex. This implementation intends to implement the same behavior we use in
+        // BrowserStore - which is operating on a list without custom tabs. Since LegacySessionManager uses a list with
+        // custom tabs mixed in, we need to shift the index around a bit.
+
+        // First we need to determine the number of custom tabs that are *before* the index we removed from. This is
+        // later needed to calculate the index we would have removed from in a list without custom tabs.
+        var numberOfCustomTabsBeforeRemovedIndex = 0
+        for (i in 0..min(index - 1, values.lastIndex)) {
+            if (values[i].isCustomTabSession()) {
+                numberOfCustomTabsBeforeRemovedIndex++
+            }
+        }
 
-        if (maxSteps <= 0) {
-            return NO_SELECTION
+        // Now calculate the index and list without custom tabs included.
+        val indexWithoutCustomTabs = index - numberOfCustomTabsBeforeRemovedIndex
+        val sessionsWithoutCustomTabs = sessions
+
+        // Code run before this one would have re-used the selected index if it is still a valid index that satisfies
+        // the predicate - however that list contained custom tabs. Now without custom tabs we may be able to re-use
+        // the exiting index. Let's check if we can:
+        if (indexWithoutCustomTabs >= 0 &&
+            indexWithoutCustomTabs <= sessionsWithoutCustomTabs.lastIndex &&
+            predicate(sessionsWithoutCustomTabs[indexWithoutCustomTabs])) {
+            return values.indexOf(sessionsWithoutCustomTabs[indexWithoutCustomTabs])
         }
 
-        // Try sessions oscillating near the index.
-        for (steps in 1..maxSteps) {
-            listOf(index - steps, index + steps).forEach { current ->
+        // Finally try sessions (without custom tabs) oscillating near the index.
+        val range = 1..max(sessionsWithoutCustomTabs.lastIndex - indexWithoutCustomTabs, indexWithoutCustomTabs)
+        for (steps in range) {
+            listOf(indexWithoutCustomTabs - steps, indexWithoutCustomTabs + steps).forEach { current ->
                 if (current >= 0 &&
-                    current <= values.lastIndex &&
-                    predicate(values[current])) {
-                    return current
+                    current <= sessionsWithoutCustomTabs.lastIndex &&
+                    predicate(sessionsWithoutCustomTabs[current])) {
+                    // Now that we have a match we need to re-calculate the index in the list that includes custom tabs.
+                    val session = sessionsWithoutCustomTabs[current]
+                    return values.indexOf(session)
                 }
             }
         }

components/browser/session/src/test/java/mozilla/components/browser/session/SessionManagerMigrationTest.kt

@@ -339,4 +339,113 @@ class SessionManagerMigrationTest {
         assertEquals("https://www.firefox.org", manager.sessions[4].url)
         assertEquals("https://www.firefox.org", store.state.tabs[4].content.url)
     }
+
+    @Test
+    fun `Remove session will not select custom tab`() {
+        val session1 = Session(id = "session1", initialUrl = "https://www.firefox.com").apply {
+            customTabConfig = mock() }
+        val session2 = Session(id = "session2", initialUrl = "https://developer.mozilla.org/en-US/")
+        val session3 = Session(id = "session3", initialUrl = "https://www.mozilla.org/en-US/internet-health/").apply {
+            customTabConfig = mock() }
+        val session4 = Session(id = "session4", initialUrl = "https://www.mozilla.org/en-US/technology/")
+        val session5 = Session(id = "session5", initialUrl = "https://example.org/555").apply {
+            customTabConfig = mock() }
+        val session6 = Session(id = "session6", initialUrl = "https://example.org/Hello").apply {
+            customTabConfig = mock() }
+        val session7 = Session(id = "session7", initialUrl = "https://example.org/World").apply {
+            customTabConfig = mock() }
+        val session8 = Session(id = "session8", initialUrl = "https://example.org/JustTestingThings").apply {
+            customTabConfig = mock() }
+        val session9 = Session(id = "session9", initialUrl = "https://example.org/NoCustomTab")
+
+        val store = BrowserStore()
+
+        val manager = SessionManager(engine = mock(), store = store)
+        manager.add(session1)
+        manager.add(session2)
+        manager.add(session3)
+        manager.add(session4)
+        manager.add(session5)
+        manager.add(session6)
+        manager.add(session7)
+        manager.add(session8)
+        manager.add(session9)
+
+        manager.select(session4)
+        assertEquals(session4, manager.selectedSession)
+        assertEquals("session4", store.state.selectedTabId)
+
+        manager.remove(session4)
+        assertEquals(session9, manager.selectedSession)
+        assertEquals("session9", store.state.selectedTabId)
+
+        manager.remove(session9)
+        assertEquals(session2, manager.selectedSession)
+        assertEquals("session2", store.state.selectedTabId)
+
+        manager.remove(session2)
+        assertNull(manager.selectedSession)
+        assertNull(store.state.selectedTabId)
+    }
+
+    /**
+     * This test case:
+     *
+     * - Runs 10000 times
+     * - Generates a random permutation of tabs and custom tabs
+     * - Selects a random tab
+     * - Randomly removes tabs
+     * - Verifies that the selected tabs in BrowserStore and SessionManager match
+     *
+     * The `println` statements have been commented out to not spam the CI log. However they are helpful in case someone
+     * needs to actually debug a problem here.
+     */
+    @Test
+    fun `Remove session from randomized set will keep store and manager in sync`() {
+        repeat(10000) {
+            val numberOfSessions = (3..20).random()
+
+            // println("Round $round ($numberOfSessions sessions)")
+
+            val store = BrowserStore()
+            val manager = SessionManager(engine = mock(), store = store)
+
+            repeat(numberOfSessions) { number ->
+                val isCustomTab = listOf(true, false).random()
+
+                val session = Session("https://example.org/$number").apply {
+                    if (isCustomTab) {
+                        customTabConfig = mock()
+                    }
+                }
+
+                manager.add(session)
+
+                // println("- Session $number (ct = $isCustomTab, id = ${session.id})" )
+            }
+
+            if (manager.sessions.isNotEmpty()) {
+                val select = manager.sessions.random()
+                // println("Select initial session: $select")
+                manager.select(select)
+            }
+
+            assertEquals(store.state.tabs.size, manager.sessions.size)
+            assertEquals(store.state.tabs.size + store.state.customTabs.size, manager.all.size)
+
+            assertEquals(store.state.selectedTabId, manager.selectedSession?.id)
+
+            (3..3).random()
+
+            repeat((3..numberOfSessions).random()) {
+                val sessionToRemove = manager.all.random()
+
+                // println("X Removing session ${sessionToRemove.id} (ct = ${sessionToRemove.isCustomTabSession()})")
+
+                manager.remove(sessionToRemove)
+
+                assertEquals(store.state.selectedTabId, manager.selectedSession?.id)
+            }
+        }
+    }
 }

components/browser/session/src/test/java/mozilla/components/browser/session/SessionManagerTest.kt

@@ -978,8 +978,8 @@ class SessionManagerTest {
         manager.select(child)
         manager.remove(child, selectParentIfExists = true)
 
-        assertEquals(session2, manager.selectedSession)
-        assertEquals("https://getpocket.com", manager.selectedSessionOrThrow.url)
+        assertEquals(session1, manager.selectedSession)
+        assertEquals("https://www.firefox.com", manager.selectedSessionOrThrow.url)
     }
 
     @Test
@@ -1009,12 +1009,12 @@ class SessionManagerTest {
         assertEquals(session4, manager.selectedSession)
 
         manager.remove(session4)
-        assertEquals(session2, manager.selectedSession)
-
-        manager.remove(session2)
         assertEquals(session9, manager.selectedSession)
 
         manager.remove(session9)
+        assertEquals(session2, manager.selectedSession)
+
+        manager.remove(session2)
         assertNull(manager.selectedSession)
     }