Merge #4739

4739: Closes #3720: Incorrect selection notification when session removed r=pocmo,rocketsroger a=csadilek

This fixes two issues: 

- The issue described in #3720 where removing the selected session does not cause a `onSessionSelected` notification

- Sending an unnecessary `onSessionSelected` when removing a session to the left of the selected session. In that case the index changed but it's still the same session that's selected and observers should not need to get notified that a session was selected
Co-authored-by: Christian Sadilek <christian.sadilek@gmail.com>

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

@@ -309,15 +309,17 @@ class LegacySessionManager(
             return
         }
 
+        val selectedBeforeRemove = selectedSession
+
         values.removeAt(indexToRemove)
 
         unlink(session)
 
-        val selectionUpdated = recalculateSelectionIndex(
-                indexToRemove,
-                selectParentIfExists,
-                session.private,
-                session.parentId
+        recalculateSelectionIndex(
+            indexToRemove,
+            selectParentIfExists,
+            session.private,
+            session.parentId
         )
 
         values.filter { it.parentId == session.id }
@@ -325,7 +327,7 @@ class LegacySessionManager(
 
         notifyObservers { onSessionRemoved(session) }
 
-        if (selectionUpdated && selectedIndex != NO_SELECTION) {
+        if (selectedBeforeRemove != selectedSession && selectedIndex != NO_SELECTION) {
             notifyObservers { onSessionSelected(selectedSessionOrThrow) }
         }
     }
@@ -340,7 +342,7 @@ class LegacySessionManager(
         selectParentIfExists: Boolean,
         private: Boolean,
         parentId: String?
-    ): Boolean {
+    ) {
         // Recalculate selection
         var newSelectedIndex = when {
             // All items have been removed
@@ -368,13 +370,7 @@ class LegacySessionManager(
                 if (selectParentIfExists) indexToRemove else newSelectedIndex)
         }
 
-        val selectionUpdated = newSelectedIndex != selectedIndex
-
-        if (selectionUpdated) {
-            selectedIndex = newSelectedIndex
-        }
-
-        return selectionUpdated
+        selectedIndex = newSelectedIndex
     }
 
     private fun newSelection(

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

@@ -9,6 +9,7 @@ import mozilla.components.browser.state.state.CustomTabConfig
 import mozilla.components.concept.engine.Engine
 import mozilla.components.concept.engine.EngineSession
 import mozilla.components.concept.engine.EngineSessionState
+import mozilla.components.support.test.any
 import mozilla.components.support.test.mock
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
@@ -151,16 +152,32 @@ class SessionManagerTest {
         val manager = SessionManager(mock())
         val session1 = Session("https://www.mozilla.org")
         val session2 = Session("https://www.firefox.com")
+        val session3 = Session("https://getpocket.com")
+        val session4 = Session("https://github.com/mozilla-mobile/android-components")
 
         manager.add(session1)
-        manager.add(session2)
+        manager.add(session2, selected = true)
+        manager.add(session3)
+        manager.add(session4)
 
         val observer: SessionManager.Observer = mock()
         manager.register(observer)
 
-        manager.remove(session1)
+        manager.remove(session3)
+        verify(observer).onSessionRemoved(session3)
+        verify(observer, never()).onSessionSelected(any())
 
+        manager.remove(session1)
         verify(observer).onSessionRemoved(session1)
+        verify(observer, never()).onSessionSelected(any())
+
+        assertEquals(session2, manager.selectedSession)
+        manager.remove(session2)
+        verify(observer).onSessionRemoved(session2)
+        // Only removing the selected session should cause
+        // a new one to be selected.
+        verify(observer).onSessionSelected(session4)
+
         verifyNoMoreInteractions(observer)
     }
 
@@ -718,14 +735,16 @@ class SessionManagerTest {
         val child = Session("https://www.mozilla.org/en-US/internet-health/")
 
         val manager = SessionManager(mock())
+        val observer: SessionManager.Observer = mock()
         manager.add(parent)
         manager.add(session1)
         manager.add(session2)
         manager.add(child, parent = parent)
 
         manager.select(child)
+        manager.register(observer)
         manager.remove(child, selectParentIfExists = true)
-
+        verify(observer).onSessionSelected(parent)
         assertEquals(parent, manager.selectedSession)
         assertEquals("https://www.mozilla.org", manager.selectedSessionOrThrow.url)
     }
@@ -740,6 +759,8 @@ class SessionManagerTest {
         val child2 = Session("https://www.mozilla.org/en-US/technology/")
 
         val manager = SessionManager(mock())
+        val observer: SessionManager.Observer = mock()
+        manager.register(observer)
         manager.add(parent)
         manager.add(session1)
         manager.add(session2)
@@ -748,7 +769,7 @@ class SessionManagerTest {
 
         manager.select(child1)
         manager.remove(child1, selectParentIfExists = false)
-
+        verify(observer).onSessionSelected(session1)
         assertEquals(session1, manager.selectedSession)
         assertEquals("https://www.firefox.com", manager.selectedSessionOrThrow.url)
     }
@@ -760,13 +781,15 @@ class SessionManagerTest {
         val session3 = Session("https://www.mozilla.org/en-US/internet-health/")
 
         val manager = SessionManager(mock())
+        val observer: SessionManager.Observer = mock()
+        manager.register(observer)
         manager.add(session1)
         manager.add(session2)
         manager.add(session3)
 
         manager.select(session3)
         manager.remove(session3, selectParentIfExists = true)
-
+        verify(observer).onSessionSelected(session2)
         assertEquals(session2, manager.selectedSession)
         assertEquals("https://getpocket.com", manager.selectedSessionOrThrow.url)
     }