Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support removing navigables from LazySetNavigator #304

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Support removing navigables from LazySetNavigator #304

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ad37ae
support removing navigables from LazySetNavigator
gerin98 Oct 23, 2024
2123832
safely remove navigables
gerin98 Oct 24, 2024
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
33 changes: 33 additions & 0 deletions magellan-library/src/main/java/com/wealthfront/magellan/navigation/LazySetNavigator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ public open class LazySetNavigator(
private val navigationPropagator: NavigationPropagator = NavigationPropagator
private var ongoingTransition: MagellanTransition? = null

@VisibleForTesting
internal var existingNavigables: MutableSet<NavigableCompat> = mutableSetOf()

@VisibleForTesting
internal var containerView: ScreenContainer? = null
private var currentNavigable: NavigableCompat? = null
Expand All @@ -36,9 +39,39 @@ public open class LazySetNavigator(
}

public fun addNavigable(navigable: NavigableCompat) {
existingNavigables.add(navigable)
lifecycleRegistry.attachToLifecycleWithMaxState(navigable, LifecycleLimit.CREATED)
}

public fun removeNavigables(navigables: Set<NavigableCompat>) {
for (navigable in navigables) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a risk of ConcurrentModificationException if one of the navigables tries to add / remove a navigable in its onDestroy ? I don't know enough about mutableSetOf to know if it allows concurrent modification

removeNavigable(navigable)
}
}

public fun removeNavigable(navigable: NavigableCompat) {
existingNavigables.remove(navigable)
lifecycleRegistry.removeFromLifecycle(navigable)
}

public fun safeAddNavigable(navigable: NavigableCompat) {
if (!existingNavigables.contains(navigable)) {
addNavigable(navigable)
}
}

public fun updateNavigables(navigables: Set<NavigableCompat>, handleCurrentTabRemoval: () -> Unit) {
val navigablesToRemove = existingNavigables subtract navigables
val navigablesToAdd = navigables subtract existingNavigables

if (navigablesToRemove.contains(currentNavigable)) {
handleCurrentTabRemoval()
}
Comment on lines +74 to +76
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an edge case to be aware of when removing a Navigable that is currently visible. We need to switch to another navigable before we can remove it because once you call removeFromLifecycle on a navigable you can't call replace on it anymore. I wrote a test updateMultipleNavigables_andRemoveCurrentNavigable describing this case.


removeNavigables(navigablesToRemove)
addNavigables(navigablesToAdd)
}

override fun onShow(context: Context) {
containerView = container()
currentNavigable?.let { currentNavigable ->
Expand Down
125 changes: 125 additions & 0 deletions magellan-library/src/test/java/com/wealthfront/magellan/navigation/LazySetNavigatorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.wealthfront.magellan.lifecycle.LifecycleState
import com.wealthfront.magellan.lifecycle.transitionToState
import com.wealthfront.magellan.transitions.CrossfadeTransition
import io.mockk.MockKAnnotations.init
import io.mockk.clearAllMocks
import io.mockk.clearMocks
import io.mockk.every
import io.mockk.impl.annotations.MockK
Expand All @@ -28,6 +29,8 @@ class LazySetNavigatorTest {
private lateinit var navigator: LazySetNavigator
private lateinit var step1: DummyStep
private lateinit var step2: DummyStep
private lateinit var step3: DummyStep
private lateinit var step4: DummyStep
@MockK private lateinit var navigableListener: NavigationListener

@Before
Expand All @@ -38,13 +41,16 @@ class LazySetNavigatorTest {
navigator = LazySetNavigator { ScreenContainer(activityController.get()) }
step1 = DummyStep()
step2 = DummyStep()
step3 = DummyStep()
step4 = DummyStep()

NavigationPropagator.addNavigableListener(navigableListener)
}

@After
fun tearDown() {
NavigationPropagator.removeNavigableListener(navigableListener)
clearAllMocks()
}

private fun initMocks() {
Expand Down Expand Up @@ -100,6 +106,7 @@ class LazySetNavigatorTest {
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step2.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Shown::class.java)
assertThat(step2.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step2)

verify { navigableListener.beforeNavigation() }
verify { navigableListener.onNavigatedFrom(step1) }
Expand Down Expand Up @@ -136,5 +143,123 @@ class LazySetNavigatorTest {
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step2.currentState).isInstanceOf(LifecycleState.Shown::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step2)
}

@Test
fun removeNavigables() {
navigator.addNavigables(setOf(step1, step2, step3, step4))
navigator.transitionToState(LifecycleState.Resumed(activityController.get()))

navigator.replace(step1, CrossfadeTransition())
step1.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step2.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(step3.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(step4.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step2, step3, step4)

verify { navigableListener.beforeNavigation() }
verify(exactly = 0) { navigableListener.onNavigatedFrom(any()) }
verify { navigableListener.onNavigatedTo(step1) }
verify { navigableListener.afterNavigation() }
clearMocks(navigableListener)
initMocks()

navigator.removeNavigables(setOf(step2, step4))

step1.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step3.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step3)

verify(exactly = 0) { navigableListener.onNavigatedTo(any()) }
}

@Test
fun updateMultipleNavigables() {
navigator.updateNavigables(setOf(step1, step2)) {}
navigator.transitionToState(LifecycleState.Resumed(activityController.get()))

navigator.replace(step1, CrossfadeTransition())
step1.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step2.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step2)

verify { navigableListener.beforeNavigation() }
verify(exactly = 0) { navigableListener.onNavigatedFrom(any()) }
verify { navigableListener.onNavigatedTo(step1) }
verify { navigableListener.afterNavigation() }
clearMocks(navigableListener)
initMocks()

navigator.updateNavigables(setOf(step1, step3, step4)) {
navigator.replace(step1, CrossfadeTransition())
}
navigator.replace(step3, CrossfadeTransition())

step3.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step3.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Shown::class.java)
assertThat(step3.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step4.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step3, step4)

verify { navigableListener.beforeNavigation() }
verify { navigableListener.onNavigatedFrom(step1) }
verify { navigableListener.onNavigatedTo(step3) }
verify { navigableListener.afterNavigation() }
}

@Test
fun updateMultipleNavigables_andRemoveCurrentNavigable() {
navigator.updateNavigables(setOf(step1, step2)) {}
navigator.transitionToState(LifecycleState.Resumed(activityController.get()))

navigator.replace(step2, CrossfadeTransition())
step2.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step2.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(step2.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step2)

verify { navigableListener.beforeNavigation() }
verify(exactly = 0) { navigableListener.onNavigatedFrom(any()) }
verify { navigableListener.onNavigatedTo(step2) }
verify { navigableListener.afterNavigation() }
clearMocks(navigableListener)
initMocks()

navigator.updateNavigables(setOf(step1, step3, step4)) {
navigator.replace(step1, CrossfadeTransition())
}

step1.view!!.viewTreeObserver.dispatchOnPreDraw()
shadowOf(Looper.getMainLooper()).idle()
assertThat(navigator.containerView!!.childCount).isEqualTo(1)
assertThat(navigator.containerView!!.getChildAt(0)).isEqualTo(step1.view)
assertThat(step1.currentState).isInstanceOf(LifecycleState.Resumed::class.java)
assertThat(step3.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(step4.currentState).isInstanceOf(LifecycleState.Created::class.java)
assertThat(navigator.existingNavigables).containsExactly(step1, step3, step4)

verify { navigableListener.beforeNavigation() }
verify { navigableListener.onNavigatedFrom(step2) }
verify { navigableListener.onNavigatedTo(step1) }
verify { navigableListener.afterNavigation() }
}
}
Loading