Fixed stack animation not updating to new child component instance when active configuration is unchanged #794
Conversation
arkivanov
force-pushed
the
fix-stack-animations-2
branch
from
82aee7d to
d489220
Compare
WalkthroughThe pull request introduces several enhancements to the stack animation handling in the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (5)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (2)
30-77: Enhance test readability by adding descriptive comments.The test method
WHEN_animating_push_and_stack_popped_during_animation_THEN_animated_push_and_pop_fully()is complex and involves multiple steps and data collections. Adding comments to explain the purpose of key sections, such as the logic behind collectingvalues1andvalues2, will improve readability and maintainability.
147-154: Leverage type inference for cleaner code.In the
childandstackfunctions, the explicit return types can be omitted, and you can simplify lambda expressions for better readability.Apply this diff to simplify the functions:
-private fun child(config: Int): Child.Created<Int, Any> = +private fun child(config: Int) = Child.Created(configuration = config, instance = Any()) -private fun stack(vararg stack: Int): ChildStack<Int, Any> = +private fun stack(vararg stack: Int) = ChildStack( active = child(stack.last()), - backStack = stack.dropLast(1).map(::child), + backStack = stack.dropLast(1).map { child(it) }, )extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (3)
141-150: Ensurewhenexpression inanimateFloatis exhaustiveAlthough all enum cases of
EnterExitStateare covered in thewhenexpression, explicitly marking the expression as exhaustive can improve code safety and readability.Modify the
whenexpression to be exhaustive:when (state) { EnterExitState.PreEnter -> 0F EnterExitState.Visible -> 1F EnterExitState.PostExit -> 0F + else -> error("Unknown state: $state") }This ensures that any future additions to
EnterExitStatewill cause a compilation error if not handled.
38-42: Consider passing progress as a parameter instead of usingCompositionLocalProviderUsing
CompositionLocalProviderto passfactormay introduce complexity, especially if multiple animations are involved. Passingfactoras a parameter to the content composable could enhance clarity and reduce the reliance on composition locals.Refactor the selector to pass
factordirectly:- stackAnimator(animationSpec = tween(durationMillis = 1000)) { factor, _, content -> - CompositionLocalProvider(LocalProgress provides factor) { - content(Modifier) - } + stackAnimator(animationSpec = tween(durationMillis = 1000)) { factor, _, content -> + content(Modifier, factor)Adjust the
contentcomposable to accept the additional parameter.
26-27: Consider simplifying test function naming conventionsWhile descriptive test names are valuable, the annotation
@Suppress("TestFunctionName")indicates that the function names may be excessively long. Consider adopting a naming convention that balances descriptiveness with brevity.For example, you could use backticks to allow spaces in function names without suppressing the naming convention:
@Test fun `animating push and stack popped during animation animates push and pop fully`() { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (5 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1 hunks)
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (3 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (5 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1 hunks)
🧰 Additional context used
🔇 Additional comments (22)
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (4)
33-44: Effective handling of animation updates withnextItemsThe introduction of
nextItemsand the updated logic correctly address the issue where the stack's active configuration remains unchanged but the child instance updates. This ensures that the animation stack reflects the new child component instance, resolving the animation inconsistency described in the PR objectives.
62-69: Appropriate use ofDisposableEffectfor state managementUtilizing
DisposableEffectwithin the exit condition allows for the safe update ofitemswithnextItemsupon the disposal of the exiting item. This ensures that the transition to the new animation items occurs only after the exit animation completes, maintaining smooth and accurate animations.
75-75: Correct input handling during pending animationsThe updated condition for displaying
InputConsumingOverlaynow accounts for(nextItems != null)in addition to(items.size > 1). This effectively prevents user interactions during ongoing or pending animations, enhancing the user experience by avoiding unintended behavior during transitions.
83-86: Improved initial animation item generationThe refinement in the
getAnimationItemsfunction ensures that whenoldStackisnullor the active keys are identical, a singleAnimationItemis created withisInitial = true. This clarifies the initial state handling and avoids unnecessary animations when there are no actual changes to animate.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
139-145: Ensure exhaustive handling ofEnterExitStateinanimateFloat.Currently, all possible states of
EnterExitStateare handled. However, to future-proof the code against potential additions to the enum, consider adding anelsebranch or a default case.Run the following script to check for exhaustive
whenexpressions overEnterExitState:This script searches for
whenexpressions that handleEnterExitStatebut lack anelsebranch. If any are found, consider adding anelsecase to handle any future states.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (3)
100-102: 🛠️ Refactor suggestionUse
rememberinstead ofLaunchedEffectfor side-effect-free state updatesUsing
LaunchedEffectto incrementcounterintroduces a side effect within a composable, which may lead to unintended behavior. Instead, consider usingrememberto safely increment the counter without side effects.Refactor the code as follows to use
remember:- if (it.key == 1) { - LaunchedEffect(Unit) { - counter++ - } - } + if (it.key == 1) { + counter = remember { counter + 1 } + }Likely invalid or redundant comment.
160-162: 🛠️ Refactor suggestionAvoid using
companion objectfor mutable stateDefining
LocalProgresswithin acompanion objectmight cause unintended sharing of state across different instances of the test class, especially if tests are run in parallel.Move
LocalProgressoutside the companion object:- private companion object { - private val LocalProgress = compositionLocalOf { 0F } - } + private val LocalProgress = compositionLocalOf { 0F }This ensures that each instance of the test class has its own
LocalProgress.Likely invalid or redundant comment.
95-102: Verify thatcounteraccurately reflects recompositionsThe variable
counteris incremented withinLaunchedEffect, but it's important to ensure that it correctly counts the number of times the composable with key1is recomposed or restarted.Run the following script to check for any shadowed variables and ensure
counteris updated correctly:Review the output to confirm that
counteris declared only once.✅ Verification successful
counteraccurately reflects recompositionsThe variable
counteris declared only once and is incremented solely within the intendedLaunchedEffectblock. This ensures that it accurately counts the number of recompositions for the composable withkey == 1.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that there are no shadowed declarations of `counter` in the test file. # Search for declarations of `counter`. Expect: Only one declaration at line 95. rg --line-number --word-regexp "var counter" extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.ktLength of output: 198
Script:
#!/bin/bash # Description: Verify that `counter++` is only called within the intended LaunchedEffect blocks for key == 1. # Search for all LaunchedEffect blocks and check if `counter++` is present ast-grep --lang kotlin --pattern ' LaunchedEffect($_) { $$$ counter++ $$$ }' extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.ktLength of output: 209
Script:
#!/bin/bash # Description: Search for all instances of `counter++` in the test file to verify their contexts. rg --line-number --word-regexp "counter\+\+" extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.ktLength of output: 209
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (6)
29-29: LGTM: ImportingassertSamefor instance comparisonThe addition of
import kotlin.test.assertSameis appropriate for ensuring that the instances in tests are identical by reference.
34-34: Updated type parameter toAnyforStackAnimationChanging the
animationparameter toStackAnimation<Config, Any>?enhances flexibility by allowing any type of instance, which is useful for testing different scenarios.
92-102: Added test for new instance with the same keyThe new test
GIVEN_child_displayed_WHEN_new_child_instance_with_the_same_key_THEN_new_child_instance_displayedcorrectly verifies that a new instance replaces the old one even when the key remains the same. This ensures the stack updates appropriately with new instances.
198-205: ModifiedsetContentto accept generic instancesUpdating the
setContentmethod to acceptState<ChildStack<Config, Any>>and a content lambda withChild.Created<Config, Any>allows for greater flexibility in rendering different instance types within the tests.
226-231: OverloadedrouterStateforChild.Created<Config, Any>The new overloaded
routerStatefunction simplifies creating aChildStack<Config, Any>from a variable number ofChild.Created<Config, Any>instances, enhancing test setup versatility.
257-257: UpdatedgetParametersto useStackAnimation<Any, Any>Changing the return type of
getParameters()toList<StackAnimation<Any, Any>?>accommodates a broader range of animations, allowing tests to cover more generic cases and ensuring compatibility with the updated type parameters.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (3)
3-3: ImportAnimatedVisibilityScopeAddedThe addition of
import androidx.compose.animation.AnimatedVisibilityScopeis appropriate and necessary for the updatedcontentparameter in thesetContentfunction.
33-33: ImportassertSamefor Instance Equality CheckIncluding
import kotlin.test.assertSameis correct as it's utilized in the new test method to verify that the new instance is displayed.
97-107: New Test for Child Instance ReplacementThe test method
GIVEN_child_displayed_WHEN_new_child_instance_with_the_same_key_THEN_new_child_instance_displayedeffectively verifies that a new instance with the same key replaces the old instance. The use ofassertSamecorrectly checks for instance equality.extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (5)
52-52: LGTM: IntroducednextItemsfor managing upcoming animationsThe addition of
nextItemseffectively manages the animation items for the next stack state, enhancing the handling of transitions when the active configuration remains unchanged.
62-65: LGTM: Updated condition for item updatesThe refined condition accurately checks whether to update
itemsbased on both the active key and the instance of the active child. This ensures that animations are appropriately triggered even when the active configuration hasn't changed but the instance has.
71-76: LGTM: Proper assignment ofnewItemsThe logic deciding whether to assign
newItemstoitemsornextItemsbased on the size ofitemseffectively manages transitions during stack updates.
95-102: LGTM: Correct cleanup ofnextItemsafter exit animationsUsing
DisposableEffectto manage the disposal ofnextItemsensures thatitemsare correctly updated after an exit animation completes, preventing stale data and ensuring smooth transitions.
108-108: LGTM: Input correctly disabled during animations involvingnextItemsBy including
(nextItems != null)in the condition, you ensure that user input is appropriately disabled when there are pendingnextItems, enhancing the user experience during transitions.
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Outdated
Show resolved
Hide resolved
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Show resolved
Hide resolved
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Show resolved
Hide resolved
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Show resolved
Hide resolved
...otlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
Outdated
Show resolved
Hide resolved
...otlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
Outdated
Show resolved
Hide resolved
...otlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
Show resolved
Hide resolved
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
fix-stack-animations-2
branch
from
d489220 to
675ba35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (1)
8-11: LGTM! Consider a minor optimization for clarity.The
takeWhileIndexedfunction is well-implemented and follows functional programming principles. It effectively combineswithIndex(),takeWhile, andmapto achieve the desired result.For slightly improved readability, you could consider using destructuring in the lambda:
internal fun <T> Iterable<T>.takeWhileIndexed(predicate: (Int, T) -> Boolean): List<T> = withIndex() .takeWhile { (index, item) -> predicate(index, item) } .map { it.value }This change doesn't affect functionality but might make the code slightly more intuitive.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt (2)
10-13: LGTM! Consider adding KDoc for clarity.The
takeSortedfunction is well-implemented and efficient. It correctly uses thetakeWhileIndexedfunction to create a sorted subset of the list based on the provided comparator.Consider adding KDoc to explain the function's behavior, especially noting that it returns a prefix of the list that is already sorted according to the comparator. This would improve clarity for future maintainers.
/** * Returns the longest prefix of this list that is sorted according to the given [comparator]. * * @param comparator The comparator to determine the order of elements. * @return A new list containing the longest sorted prefix of this list. */ internal fun <T> List<T>.takeSorted(comparator: Comparator<T>): List<T> = ...
15-18: LGTM! Consider adding KDoc for clarity.The
takeWhileIndexedfunction is well-implemented, making efficient use of Kotlin's standard library functions.Consider adding KDoc to explain the function's behavior, especially noting the inclusion of the index in the predicate. This would improve clarity for future maintainers.
/** * Returns a list containing first elements satisfying the given [predicate] applied to the element's index and the element itself. * * @param predicate The predicate to be invoked on each element, which also receives the element's index. * @return A list containing the first elements satisfying the given [predicate]. */ internal fun <T> Iterable<T>.takeWhileIndexed(predicate: (Int, T) -> Boolean): List<T> = ...extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (2)
71-76: Approve the new logic for assigning items with a suggestionThe new logic for assigning items is a good improvement. It handles single-item and multi-item cases differently, which allows for more precise control over transitions. This approach should help in managing rapid changes more effectively, addressing the issues described in #792.
However, to improve code clarity, consider extracting the condition
items.size == 1into a named variable:val isSingleItem = items.size == 1 if (isSingleItem) { items = newItems } else { nextItems = newItems }This minor refactoring would make the intent of the condition more explicit and improve readability.
108-108: Approve the updated condition for disabling user input with a suggestionThe expanded condition for disabling user input during animations is a good improvement. By including a check for
nextItems, it ensures that user interactions are properly managed during complex transitions, which is crucial for preventing unexpected behavior in scenarios like those described in #792.To improve readability, consider extracting the condition into a separate function:
private fun shouldDisableInput(): Boolean = disableInputDuringAnimation && ((items.size > 1) || (nextItems != null)) // Usage if (shouldDisableInput()) { Overlay(modifier = Modifier.matchParentSize()) }This refactoring would make the code more self-documenting and easier to maintain.
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (3)
35-36: Consider using more descriptive variable names for clarityThe variables
values1andvalues2could be renamed to more descriptive names likevaluesFirstChildandvaluesSecondChildto improve code readability and maintainability.
43-46: Handle unexpected keys in thewhenexpressionTo increase the robustness of the test, consider adding an
elsebranch to thewhenexpression to handle unexpectedit.keyvalues.
116-120: Rename parameterstackto avoid confusionIn the function
private fun stack(vararg stack: Int), the parameter namestackcould be confusing since it suggests a complete stack but actually represents individual configurations. Consider renaming it toconfigsoritemsfor better clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt (1 hunks)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (2 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (5 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1 hunks)
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (3 hunks)
- extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (1 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (5 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
🧰 Additional context used
🔇 Additional comments (22)
extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (2)
3-6: LGTM! Efficient implementation oftakeSorted.The
takeSortedfunction is well-implemented and efficient. It cleverly usestakeWhileIndexedto create a new list that maintains the order specified by the comparator, stopping as soon as an out-of-order element is encountered. This approach is more efficient than sorting the entire list when only a portion of it needs to be in order.
1-11: Well-designed utility functions for collection processing.These new utility functions,
takeSortedandtakeWhileIndexed, are well-implemented and provide valuable extensions to Kotlin collections. Theirinternalvisibility is appropriate, limiting their use to within the module. Given their location in thecommonTestsource set, they appear to be designed for use in testing scenarios, which is a good practice for keeping test-specific utilities separate from production code.The functions demonstrate a good understanding of functional programming concepts and efficient collection processing. They should prove useful for various testing scenarios within the module, particularly when dealing with ordered collections or when needing to process collections with access to both index and value.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt (1)
10-18: Good additions supporting the PR objectives.The new utility functions
takeSortedandtakeWhileIndexedare well-implemented and appear to support the improvements in stack animation handling mentioned in the PR objectives. Their internal visibility appropriately limits their use to within the module.These functions likely contribute to more efficient management of animation items during transitions, which aligns with the goal of fixing the stack animation issues described in the linked issue #792.
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (1)
19-20: LGTM! Improved flexibility for animation testing.The addition of the
durationMillisparameter to theanimateFloatfunction is a positive change. It allows for more precise control over animation timing in tests, which can be crucial for debugging issues like the one described in the PR objectives. The use ofAnimationConstants.DefaultDurationMillisas the default value ensures backward compatibility.This change enhances the utility of the function without introducing any apparent issues. It's well-aligned with the goal of improving stack animation handling.
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (7)
5-5: LGTM: Import statement addition is appropriate.The addition of the
DisposableEffectimport is consistent with its usage in the new functionality introduced later in the file.
33-33: LGTM: New variablenextItemsenhances animation state management.The introduction of
nextItemsas a nullableMap<Any, AnimationItem<C, T>>usingmutableStateOfis a good addition. It allows for better handling of upcoming animation items, which aligns with the PR's objective to improve stack animation behavior.
35-44: Excellent update to the stack refresh condition.The modification to check both
stack.active.keyandstack.active.instanceaddresses the core issue described in the PR objectives. This change ensures that the stack updates correctly when a new child component instance is created, even if the active configuration remains unchanged.The subsequent logic for updating
itemsandnextItemsis also well-structured:
- If there's only one item, it directly updates
items.- Otherwise, it prepares the next set of items in
nextItems.This approach should resolve the animation issues when quickly replacing components in a nested structure.
62-69: Great addition of DisposableEffect for managing animation state transitions.The introduction of the DisposableEffect for exiting items is a crucial improvement. It ensures that:
- The
nextItemsare applied toitemswhen an exiting animation completes.- The
nextItemsstate is reset to null after being applied.This mechanism provides a clean way to handle the transition between animation states, which is essential for the smooth operation of the stack animation, especially during rapid component replacements.
75-75: Improved InputConsumingOverlay condition enhances user interaction handling.The updated condition
((items.size > 1) || (nextItems != null))for displaying the InputConsumingOverlay is a valuable improvement. It ensures that user input is properly managed during both ongoing animations (whenitems.size > 1) and pending animations (whennextItemsis not null). This change aligns well with the PR objectives and should prevent unexpected behavior during rapid component transitions.
83-83: Simplified and improved getAnimationItems condition.The modification to the condition in the
getAnimationItemsfunction is a good improvement. By checking for(oldStack == null) || (newStack.active.key == oldStack.active.key), it correctly handles both initial animations and cases where the active key remains the same but the instance changes. This change supports the PR's goal of ensuring proper animation behavior during component updates and replacements.
Line range hint
1-114: Overall, excellent improvements to stack animation handling.The changes made to
AbstractStackAnimation.kteffectively address the issues outlined in the PR objectives. Key improvements include:
- Introduction of
nextItemsfor better management of upcoming animation states.- Enhanced condition for updating
currentStack, ensuring proper handling of new child component instances.- Addition of a DisposableEffect for clean transitions between animation states.
- Improved InputConsumingOverlay condition for better user interaction management during animations.
- Simplified logic in
getAnimationItemsfor more robust animation item generation.These changes collectively provide a more robust and flexible stack animation system, particularly for scenarios involving rapid component replacements and nested structures. The modifications should resolve the reported issues with animation behavior and improve overall user experience.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (4)
52-52: Approve the addition ofnextItemsvariableThe introduction of the
nextItemsvariable is a good solution to manage upcoming animation items during stack transitions. This addresses the issue described in #792 where quick replacements led to retaining references to old component instances. By temporarily storing the next set of items, the animation system can now handle rapid transitions more effectively.
62-64: Approve the refined condition for updating itemsThe updated condition for updating items is more comprehensive and precise. By checking both the key and the instance of the active child, it ensures that the animation system correctly handles cases where the active configuration remains unchanged but the child component instance is new. This directly addresses the core issue described in the PR, where the stack animation was not updating to new child component instances in certain scenarios.
95-102: Approve the addition of DisposableEffect for managing animation item lifecycleThe introduction of the DisposableEffect is a crucial improvement in managing the lifecycle of animation items. By ensuring that
nextItemsare correctly assigned toitemsafter the exit animation completes, it addresses the issue of component instances not being refreshed correctly during quick transitions. This change is key to solving the problem described in #792, where rapid replacements led to unexpected behavior in nested component structures.The null check on
nextItemsis a good practice, preventing unnecessary assignments and potential issues. This implementation ensures a smooth transition between animation states, even in scenarios of quick component replacements.
Line range hint
1-379: Overall assessment: Effective solution to the stack animation issueThe changes implemented in this PR effectively address the core issue described in #792, where stack animations were not updating to new child component instances when the active configuration remained unchanged. The introduction of
nextItems, refined update conditions, and improved lifecycle management throughDisposableEffectcollectively provide a robust solution for handling rapid transitions in nested component structures.These improvements should resolve the strange behavior observed during quick replacements between components like
MainComponentandWelcomeComponent. The enhanced logic ensures that new instances of child components (e.g.,InnerComponent) are correctly instantiated and displayed, even during rapid transitions.The code quality is good, with only minor suggestions for improving readability. These changes align well with the PR objectives and should significantly improve the reliability and consistency of stack animations in complex UI scenarios.
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (4)
92-103: Well-Implemented Test for Child Instance ReplacementThe new test
GIVEN_child_displayed_WHEN_new_child_instance_with_the_same_key_THEN_new_child_instance_displayed()correctly verifies that a new child instance with the same key replaces the old instance. The use ofassertSameensures that the new instance is properly displayed. This test enhances the reliability of component updates when instances change but keys remain the same.
198-205: UpdatedsetContentMethod Signature and UsageThe
setContentmethod now acceptsState<ChildStack<Config, Any>>and acontentlambda. This change aligns with the updated instance type and provides greater flexibility in rendering child components. Ensure that all calls tosetContentare updated accordingly and that the defaultcontentparameter functions as expected.
226-231: Addition of OverloadedrouterStateFunctionThe new
routerStatefunction accepts a variable number ofChild.Created<Config, Any>parameters, allowing the creation of aChildStackwith diverse instance types. This addition improves test setup flexibility. Verify that this does not conflict with existingrouterStatemethods and that it is used appropriately throughout the tests.
257-257: Consistent Type Parameters ingetParametersMethodThe
getParameters()method now returns aList<StackAnimation<Any, Any>?>, which maintains consistency with the other changes to useAnyas the instance type. This update ensures that the stack animations tested can handle any instance type, increasing the robustness of the tests.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (3)
97-107: Test Function Correctly Verifies Child Instance ReplacementThe new test
GIVEN_child_displayed_WHEN_new_child_instance_with_the_same_key_THEN_new_child_instance_displayedaccurately checks that a new child instance with the same key replaces the previous instance in the UI. This ensures that the stack correctly updates to reflect the new instance, aligning with the PR's objective to fix the animation stack issue.
203-210: Enhanced Flexibility with Custom Content insetContentFunctionThe modification of the
setContentfunction to accept acontentparameter allows for more customizable UI components during testing. This enhancement improves the test suite's flexibility by enabling different content representations for child instances.
231-236: OverloadedrouterStateFunction Improves Test SetupThe addition of the overloaded
routerStatefunction that acceptsChild.Created<Config, Any>instances simplifies the creation of custom child stacks with specific instances and keys. This change enhances the ease of setting up test scenarios that require precise control over the stack's state.
...-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
fix-stack-animations-2
branch
from
675ba35 to
51a1cc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (1)
35-44: LGTM: Improved handling of stack updates and animation items.The changes effectively address the issue of the stack not updating to new child component instances when the active configuration is unchanged. The condition now checks both the key and instance of the active stack item, ensuring proper updates.
The refined handling of
itemsandnextItemsbased on the current state is a good approach to manage animations smoothly.Consider extracting the condition
stack.active.key != currentStack.active.key || stack.active.instance != currentStack.active.instanceinto a separate function for better readability and reusability. For example:private fun isStackChanged(newStack: ChildStack<C, T>, currentStack: ChildStack<C, T>): Boolean = newStack.active.key != currentStack.active.key || newStack.active.instance != currentStack.active.instanceThen use it in the condition:
if (isStackChanged(stack, currentStack)) { // ... existing code ... }extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (1)
92-102: LGTM: New test method addresses the core issue.The new test method effectively verifies that a new child instance with the same key is displayed correctly, which aligns with the PR objectives. The test is well-structured and follows the Given-When-Then pattern.
Consider adding a brief comment explaining the purpose of this test, as it directly relates to the issue being fixed in this PR.
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (3)
203-210: Approve changes tosetContentwith a suggestionThe addition of the
contentparameter enhances the flexibility of the test setup. However, consider using a generic type instead ofAnyto maintain type safety:private fun <Instance> setContent( state: State<ChildStack<Config, Instance>>, content: @Composable AnimatedVisibilityScope.(Child.Created<Config, Instance>) -> Unit = { Child(name = it.key.toString()) }, )This change would be consistent with the earlier suggestion to parameterize the class.
231-236: Approve newrouterStateoverload with a suggestionThe new overload enhances the flexibility of creating router states for testing. However, to maintain type safety, consider using a generic type:
private fun <Instance> routerState(vararg stack: Child.Created<Config, Instance>): ChildStack<Config, Instance> = ChildStack( active = stack.last(), backStack = stack.dropLast(1), )This change would be consistent with the earlier suggestions to use generics throughout the class.
Line range hint
1-300: Summary ofChildStackTest.ktchangesThe modifications to
ChildStackTest.ktenhance the flexibility of the test class by allowing for more diverse instance types. However, the consistent use ofAnyas a type parameter introduces potential type safety issues.The main recommendation is to parameterize the
ChildStackTestclass and use generics throughout instead ofAny. This approach would maintain the desired flexibility while preserving type safety. Consider applying this change consistently across the class, including the constructor, methods, and parameter types.These changes align well with the PR objectives of improving stack animation handling and ensuring correct updates when replacing components. The new test method effectively covers an important edge case in stack management.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3)
71-76: Approved: Enhanced item update logicThe new logic for updating items based on their current size is a smart approach to manage transitions more smoothly, especially for quick replacements. The use of
nextItemsfor multiple items allows for a staged update, which can prevent visual glitches during complex transitions.Consider extracting the condition
items.size == 1into a meaningful variable name to improve readability. For example:val isSingleItem = items.size == 1 if (isSingleItem) { items = newItems } else { nextItems = newItems }
95-102: Approved: DisposableEffect for managing exiting itemsThe addition of this DisposableEffect is a crucial improvement. It ensures that
nextItemsare correctly reassigned toitemsafter the exit animation completes, maintaining the proper state of animations during complex transitions.For improved clarity, consider adding a brief comment explaining the purpose of this DisposableEffect. For example:
// Ensure nextItems are assigned to items after exit animation DisposableEffect(Unit) { onDispose { nextItems?.also { items = it } nextItems = null } }
108-108: Approved: Enhanced condition for disabling user inputThe updated condition for disabling user input during animation is a good improvement. By including a check for
nextItems, it ensures that user input remains disabled during complex transitions managed bynextItems, preventing potential unexpected behavior.Consider extracting the condition into a separate function for improved readability:
private fun shouldDisableInput(): Boolean = disableInputDuringAnimation && ((items.size > 1) || (nextItems != null)) // Usage if (shouldDisableInput()) { Overlay(modifier = Modifier.matchParentSize()) }This refactoring would make the code more self-documenting and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt (1 hunks)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (2 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (6 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1 hunks)
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (3 hunks)
- extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (1 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (5 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt
- extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
🧰 Additional context used
🔇 Additional comments (16)
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (6)
5-5: LGTM: Import statement addition is appropriate.The addition of the
DisposableEffectimport is consistent with its usage in the new functionality introduced later in the file.
33-33: LGTM: New variablenextItemsenhances animation state management.The introduction of
nextItemsas a nullableMap<Any, AnimationItem<C, T>>usingmutableStateOfis a good addition. It allows for better handling of upcoming animation items, which aligns with the PR's objective to improve stack animation behavior.
62-69: LGTM: DisposableEffect ensures proper cleanup of animation items.The addition of the DisposableEffect is a good implementation to handle the cleanup of exiting items. It ensures that
itemsare updated withnextItemswhen an item is disposed, which is crucial for proper management of animation items during transitions. This change directly addresses the PR's objective to fix issues with the animation stack updating.
75-75: LGTM: Improved InputConsumingOverlay condition enhances animation behavior.The modified condition for displaying the InputConsumingOverlay now accounts for both multiple items and the presence of
nextItems. This change ensures that user input is properly managed during animations, even when transitioning to new items. It aligns well with the PR's objective to improve animation behavior during quick replacements.
83-83: LGTM: Simplified and improved getAnimationItems logic.The modification to the
getAnimationItemsfunction's condition is a good improvement. It now handles cases where the old stack is null or the active keys of the new and old stacks match, which simplifies the logic and improves the handling of initial states and unchanged configurations. This change aligns well with the PR's objective to fix issues with stack animation updating.
Line range hint
1-114: Overall assessment: Excellent implementation addressing the PR objectives.The changes in this file effectively address the issues described in the PR objectives, particularly the problem of the animation stack not updating to new child component instances when the active configuration is unchanged. The introduction of
nextItems, refinement of update conditions, and improved handling of animation items all contribute to solving the reported issues.The code quality is high, with clear logic and appropriate use of Compose concepts. The suggestions made are minor and aimed at further improving readability and maintainability.
Great job on implementing these improvements!
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (6)
29-29: LGTM: Import statement added correctly.The import of
kotlin.test.assertSameis necessary for the new test method and is correctly placed.
34-34: LGTM: Constructor parameter type updated correctly.The change from
StackAnimation<Config, Config>?toStackAnimation<Config, Any>?aligns with the PR objectives and allows for more flexibility in handling different types of child instances.
198-203: LGTM:setContentmethod signature updated correctly.The changes to the
setContentmethod signature align with the PR objectives and the constructor parameter type change. The addition of thecontentparameter with a default value increases flexibility while maintaining backward compatibility.
226-230: LGTM: NewrouterStatemethod added correctly.The new
routerStatemethod aligns with the type changes made throughout the class and provides a convenient way to create aChildStackwith the updated type parameters. The implementation correctly sets the last item as the active child and the rest as the back stack.
257-257: LGTM:getParametersmethod return type updated correctly.The change in the return type from
List<StackAnimation<Config, Config>?>toList<StackAnimation<Config, Any>?>is consistent with the type changes made throughout the class and ensures type consistency across the test class.
Line range hint
1-277: Overall assessment: Changes effectively address the PR objectives.The modifications in this file successfully address the issue of stack animation not updating to new child component instances. The changes include:
- Updating type parameters from
ConfigtoAnyfor greater flexibility.- Adding a new test method to verify correct handling of new child instances with the same key.
- Updating method signatures and return types to maintain consistency with the new type parameters.
These changes align well with the PR objectives and should resolve the reported issue while maintaining backward compatibility and increasing the flexibility of the
ChildrenTestclass.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
97-107: Excellent addition of edge case testThis new test method effectively verifies that the stack updates correctly when a new child instance with the same key is pushed. It aligns well with the PR objectives and helps ensure proper component replacement behavior.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3)
52-52: LGTM: Introduction ofnextItemsvariableThe addition of the
nextItemsvariable is a good approach to manage upcoming animation items during stack transitions. This aligns well with the PR objective of fixing the stack animation update issue, particularly when quickly replacing components in a nested structure.
62-64: Crucial fix: Improved condition for updating itemsThis change is critical to addressing the core issue described in the PR. By checking not only the active key but also the instance of the active child, the code now ensures that the animation updates correctly even when only the instance changes. This directly fixes the problem where InnerComponent wasn't refreshing properly after quick transitions between MainComponent and WelcomeComponent.
Line range hint
1-380: Overall assessment: Excellent improvements to stack animation handlingThe changes in this file effectively address the reported issue of stack animation not updating for new child component instances when the active configuration remains unchanged. The introduction of
nextItems, refined update logic, and improved handling of transitions and user input collectively contribute to a more robust and reliable animation system.The modifications align well with the PR objectives and should resolve the strange behavior observed during quick component replacements. The code quality is good, with only minor suggestions for improvement in terms of readability and organization.
Great job on implementing these crucial fixes and enhancements!
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Show resolved
Hide resolved
...vanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
Outdated
Show resolved
Hide resolved
arkivanov
force-pushed
the
fix-stack-animations-2
branch
from
51a1cc3 to
92472d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt (1 hunks)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (2 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (6 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1 hunks)
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (3 hunks)
- extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (1 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (5 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/Utils.kt
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt
- extensions-compose/src/commonTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
🧰 Additional context used
🔇 Additional comments (19)
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (6)
5-5: LGTM: Import statement addition is appropriate.The addition of the
DisposableEffectimport is consistent with its usage in the new functionality introduced later in the file.
33-33: LGTM: Addition ofnextItemsvariable enhances animation handling.The introduction of the
nextItemsvariable is a key component in addressing the animation stack issue described in the PR objectives. It provides a mechanism to queue up the next set of animation items, allowing for smoother transitions between component states.
35-44: Excellent: Updated logic addresses core PR objective.The changes in this section directly address the main issue described in the PR objectives:
The condition
stack.active.key != currentStack.active.key || stack.active.instance != currentStack.active.instanceensures that the stack is updated when either the key or the instance changes. This fixes the problem of components not refreshing correctly when only the instance changes.The new logic for handling
itemsandnextItemsprovides a more robust way of managing the animation queue, allowing for smoother transitions between component states.These changes are crucial in fixing the animation stack issue and ensuring that new child component instances are correctly updated and displayed.
62-69: LGTM: DisposableEffect addition improves animation item lifecycle management.The introduction of the DisposableEffect is a crucial enhancement:
- It ensures proper cleanup of exiting animation items.
- It manages the transition to the next set of items by assigning
nextItemstoitemsupon disposal.- This mechanism contributes to smoother animations and more predictable component lifecycle management.
This addition aligns well with the PR's objective of improving the animation stack behavior during quick component replacements.
75-75: LGTM: Improved InputConsumingOverlay condition enhances animation robustness.The updated condition
(items.size > 1) || (nextItems != null)for displaying the InputConsumingOverlay is a valuable improvement:
- It ensures that user input is properly managed during animations, including scenarios with pending next items.
- This change prevents unintended interactions during component transitions, contributing to a more stable and predictable user experience.
- The modification aligns well with the PR's objective of enhancing animation behavior during quick component replacements.
This improvement adds an extra layer of reliability to the animation process, particularly in scenarios involving rapid state changes.
Line range hint
1-114: Summary: Changes effectively address PR objectives and enhance animation handling.The modifications in this file collectively solve the core issues described in the PR objectives and linked issue #792:
- The new
nextItemsvariable and associated logic ensure proper handling of quick component replacements.- The updated condition for refreshing the stack addresses the problem of components not updating when only the instance changes.
- The DisposableEffect addition improves the lifecycle management of animation items.
- The enhanced InputConsumingOverlay condition prevents unintended user interactions during animations.
These changes significantly improve the robustness and predictability of the animation system, particularly in scenarios involving rapid state changes and nested component structures. The implementation aligns well with the goal of fixing the strange behavior observed during quick replacements between MainComponent and WelcomeComponent.
Great job on addressing these complex animation issues!
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (2)
97-107: Well-structured test for child instance updateThis new test method effectively verifies that when a new child instance with the same key is pushed onto the stack, the new instance is correctly displayed. The test follows good practices:
- Clear naming using the Given-When-Then pattern
- Proper setup of the initial state
- Correct simulation of updating the child instance
- Appropriate use of
assertSamefor instance equality checkGood job on adding this test to cover an important edge case in the stack animation behavior.
Line range hint
1-307: Summary: Consider using generics for improved type safetyThe changes in this file introduce more flexibility by using
Anyas the instance type in various methods and parameters. While this allows for diverse test scenarios, it may lead to potential type safety issues. The main recommendation is to consider using a generic type parameter throughout theChildStackTestclass and its methods. This approach would maintain the desired flexibility while ensuring type safety.The new test method
GIVEN_child_displayed_WHEN_new_child_instance_with_the_same_key_THEN_new_child_instance_displayedis a valuable addition, effectively covering an important edge case in stack animation behavior.By implementing the suggested generic approach, you can achieve a good balance between flexibility and type safety in your test suite.
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (5)
25-72: Well-structured test for animation behaviorThe test method
WHEN_animating_push_and_stack_popped_during_animation_THEN_animated_push_and_pop_fullyis comprehensive and effectively verifies the push and pop animations when the stack is manipulated during an ongoing animation. The use of value tracking and assertions ensures the animation states are correctly validated.
74-104: Effective test for child restart upon stack manipulationThe test method
WHEN_animating_push_and_stack_popped_during_animation_THEN_first_child_restartedaccurately checks that the first child is restarted when the stack is popped during an animation. The incrementing counter within theLaunchedEffectand the final assertion provide a clear validation of the expected behavior.
106-111: Efficient time advancement utilityThe helper function
advanceFramesByprovides an efficient way to simulate the passage of time within the tests. This utility enhances readability and reusability within the test suite.
113-114: Clear child creation helper functionThe
childfunction simplifies the creation ofChild.Createdinstances for testing purposes. UsingAny()as the instance is appropriate in this context and keeps the tests focused on the stack behavior.
116-120: Concise stack construction functionThe
stackfunction provides a clear and concise way to constructChildStackinstances from a variable number of configurations. This enhances the flexibility and readability of the test setups.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (6)
29-29: ImportingassertSamefor Instance ComparisonThe
import kotlin.test.assertSamestatement is appropriate and necessary for asserting instance equality in the new test.
34-34: Consistent Update of Type Parameters forStackAnimationThe type parameter has been correctly updated to
StackAnimation<Config, Any>?, ensuring consistency across the class and accommodating any child instance types.
92-102: Addition of Test for New Child Instance with Same KeyThe new test method effectively verifies that a new child instance with the same key replaces the previous instance as expected. The use of
assertSameconfirms that the displayed instance is updated correctly.
198-205: Enhanced Flexibility insetContentMethodUpdating the
setContentmethod to acceptState<ChildStack<Config, Any>>and a content lambda withChild.Created<Config, Any>improves the flexibility and reusability of the test setup.
226-231: OverloadedrouterStateFunction for Generic Child InstancesThe new
routerStatefunction overload facilitates the creation ofChildStack<Config, Any>with generic child instances, enhancing the versatility of the test configurations.
257-257: Updated Type Parameters ingetParametersMethodThe return type of
getParametershas been updated toList<StackAnimation<Config, Any>?>, ensuring alignment with the updated type parameters and maintaining type safety.
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
...mTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt
Show resolved
Hide resolved
…en active configuration is unchanged
arkivanov
force-pushed
the
fix-stack-animations-2
branch
from
92472d3 to
7dd61ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (3)
3-6: LGTM! Consider adding KDoc for better documentation.The
takeSortedfunction is well-implemented and efficiently returns a new list with elements in non-decreasing order according to the provided comparator. The logic is correct and the function name accurately describes its behavior.Consider adding KDoc to describe the function's purpose and parameters for better documentation:
/** * Returns a new list containing elements from the original list, * starting from the first element and continuing as long as the elements * are in non-decreasing order according to the provided comparator. * * @param comparator The comparator used to determine the order of elements. * @return A new list with elements in non-decreasing order. */ internal fun <T> List<T>.takeSorted(comparator: Comparator<T>): List<T> = ...
8-11: LGTM! Consider adding KDoc for better documentation.The
takeWhileIndexedfunction is well-implemented and efficiently returns a list of elements that satisfy the given predicate, which has access to both the index and the item. The logic is correct and the function name accurately describes its behavior.Consider adding KDoc to describe the function's purpose and parameters for better documentation:
/** * Returns a list containing elements from the original iterable * as long as the given predicate returns true for each element and its index. * * @param predicate A function that takes the index and item as parameters and returns a boolean. * @return A list containing elements that satisfy the predicate. */ internal fun <T> Iterable<T>.takeWhileIndexed(predicate: (Int, T) -> Boolean): List<T> = ...
1-11: Well-implemented utility functions for testing purposes.This file introduces two well-designed and efficient utility functions,
takeSortedandtakeWhileIndexed, which are likely to be useful in various testing scenarios. The functions are appropriately marked as internal and placed in the test directory, indicating good separation of test utilities from the main library code.Consider creating a dedicated
TestUtils.ktfile if more test-specific utility functions are added in the future, to keep all test utilities organized in one place.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (3)
19-26: LGTM! Consider adding a comment for the default value.The changes to
animateFloatfunction look good. The addition of thedurationMillisparameter provides more flexibility for testing different animation durations while maintaining backward compatibility with the default value.Consider adding a brief comment explaining the significance of
AnimationConstants.DefaultDurationMillisfor better code documentation:internal fun Transition<EnterExitState>.animateFloat( // Default duration as defined in AnimationConstants durationMillis: Int = AnimationConstants.DefaultDurationMillis ): State<Float> = // ... rest of the function
28-31: LGTM! Consider adding a brief function comment.The
takeSortedfunction is a useful addition that efficiently creates a sorted subset of a list. The implementation is concise and correct.Consider adding a brief function comment to explain its behavior:
/** * Returns a list containing the first elements satisfying the given [comparator] in ascending order. * Stops taking elements when the order defined by the comparator is broken. */ internal fun <T> List<T>.takeSorted(comparator: Comparator<T>): List<T> = // ... rest of the function
33-36: LGTM! Consider adding a brief function comment.The
takeWhileIndexedfunction is a well-implemented utility that combines indexing and filtering operations efficiently. It's good to see it being used in thetakeSortedfunction, demonstrating effective code reuse.Consider adding a brief function comment to explain its behavior:
/** * Returns a list containing first elements satisfying the given [predicate] applied to the element and its index. * The operation is intermediate and stateless. */ internal fun <T> Iterable<T>.takeWhileIndexed(predicate: (Int, T) -> Boolean): List<T> = // ... rest of the functionextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (2)
33-33: LGTM: New variablenextItemsaddedThe addition of the
nextItemsvariable is a good approach to manage upcoming animation items. This change appears to address the issue with stack animation not updating to new child component instances.Consider adding a brief comment explaining the purpose of
nextItemsto improve code readability.
35-44: LGTM: Improved logic for handling stack updates and animationsThe changes in this section effectively address the issue of not updating when only the instance changes. The new logic for handling
itemsandnextItemsprovides a smoother transition between animation states.Consider extracting the condition
stack.active.key != currentStack.active.key || stack.active.instance != currentStack.active.instanceinto a separate function for better readability, e.g.,hasStackChanged(stack, currentStack).extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
203-210: LGTM: Improved flexibility insetContentmethodThe updated
setContentmethod signature with the newcontentparameter enhances the flexibility of content rendering in tests. This change aligns well with the constructor parameter update and allows for more diverse testing scenarios.However, consider adding a comment explaining the purpose of the
contentparameter to improve code readability:/** * @param content A composable function to render the child content. * Defaults to rendering a basic text with the child's key. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (3 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (2 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (6 hunks)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1 hunks)
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (3 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/Utils.kt (1 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (5 hunks)
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
🧰 Additional context used
🔇 Additional comments (16)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/TestUtils.kt (1)
Line range hint
1-37: Overall, excellent changes that enhance testing capabilities.The modifications to
TestUtils.ktare well-implemented and align with the PR objectives. The updatedanimateFloatfunction provides more flexibility for animation testing, while the newtakeSortedandtakeWhileIndexedfunctions offer useful utilities for handling collections in tests. These changes should contribute to more robust and flexible testing of stack animations.extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/AbstractStackAnimation.kt (4)
5-5: LGTM: Import statement added for DisposableEffectThe addition of the import statement for
DisposableEffectis appropriate and consistent with its usage in the file.
62-69: LGTM: DisposableEffect added for proper handling of nextItemsThe addition of the DisposableEffect is a good approach to ensure that
nextItemsare properly handled when an item exits the composition. This change contributes to the smooth transition between animation states and is consistent with Compose best practices for side effects.
75-75: LGTM: Improved condition for InputConsumingOverlayThe modification to the condition for displaying the InputConsumingOverlay is appropriate. It now correctly accounts for the presence of
nextItems, ensuring that input is consumed during animations when either there are multiple items or when there are upcoming items to be animated.
Line range hint
1-114: Overall assessment: Effective solution for stack animation issuesThe changes introduced in this file successfully address the issue described in the PR objectives. The new approach with
nextItems, improved condition checking, and the addition of the DisposableEffect provide a more robust way to manage animations during quick replacements of child components.Key improvements:
- The condition for updating
currentStacknow checks both the key and instance, addressing cases where only the instance changes.- The introduction of
nextItemsallows for queuing up the next animation while the current one is still in progress.- The DisposableEffect ensures proper handling of
nextItemswhen items exit the composition.- The modified condition for InputConsumingOverlay accounts for both current and upcoming animation items.
These changes collectively contribute to resolving the strange behavior observed during rapid component replacements, ensuring that new instances of child components are correctly instantiated and displayed.
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (5)
3-3: LGTM: Import and constructor parameter updatesThe addition of the
assertSameimport and the change in the constructor parameter type toStackAnimation<Config, Any>?improve the flexibility of the test class. These changes align well with the PR objectives by allowing for more diverse instance types in the animation, which is crucial for testing the stack animation updates.Also applies to: 33-33, 39-39
97-107: Excellent addition: New test for child instance updatesThis new test method directly addresses the core issue described in the PR objectives. It verifies that when a new child instance with the same key is pushed onto the stack, the new instance is correctly displayed. This is crucial for ensuring that the stack animation properly updates to reflect new child component instances, even when the active configuration remains unchanged.
231-236: LGTM: NewrouterStateoverload for flexible instance typesThe addition of this new
routerStateoverload is a good improvement. It allows for creating router states with more flexible instance types, which is consistent with the other changes in the class. This addition enhances the testing capabilities of theChildStackTestclass and supports the PR objectives by allowing for more diverse testing scenarios.
261-261: LGTM: UpdatedgetParametersreturn typeThe change in the return type of
getParameterstoList<StackAnimation<Config, Any>?>is consistent with the other updates in the class. This modification ensures that the test parameters align with the new flexible instance type approach, supporting more comprehensive testing scenarios.
Line range hint
1-301: Overall: Excellent improvements toChildStackTestThe changes made to
ChildStackTesteffectively address the PR objectives and enhance the class's testing capabilities. Key improvements include:
- More flexible instance types throughout the class, allowing for diverse testing scenarios.
- A new test method that directly verifies the correct updating of child component instances.
- Enhanced flexibility in content rendering for tests.
- Consistent updates across related methods to support the new approach.
These modifications significantly improve the class's ability to test stack animations, particularly in scenarios where child component instances change while configurations remain the same. The changes are well-structured, maintain consistency, and align perfectly with the PR objectives.
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (6)
52-52: LGTM: Introduction ofnextItemsvariableThe addition of the
nextItemsvariable is a good approach to manage upcoming animation items during stack transitions. This aligns well with the PR objective of fixing the stack animation update issue, particularly when quickly replacing components in a nested structure.
56-76: LGTM: Improved logic for updating animation itemsThe changes in this segment significantly improve the handling of stack transitions, particularly addressing the issue where the animation stack wasn't updating to new child component instances when the active configuration remained unchanged. The new logic ensures that items are updated not just when keys change, but also when instances change, which is crucial for resolving the reported issue.
The differentiation between single and multiple items in the update logic adds necessary complexity to handle various scenarios correctly, including the case of quickly replacing components in a nested structure.
94-102: LGTM: Addition of DisposableEffect for managing nextItemsThe introduction of this DisposableEffect is a crucial addition that ensures proper management of
nextItemsafter the exit animation completes. This change directly addresses the issue of components not refreshing correctly after quick transitions, as described in the PR objectives.By reassigning
nextItemstoitemsand clearingnextItemson disposal, this effect helps maintain the correct state of the animation stack, even in scenarios involving rapid component replacements.
108-108: LGTM: Enhanced condition for disabling user input during animationsThe modification to include a check for
nextItemsin the condition for disabling user input during animations is a valuable improvement. This change ensures that user interactions are properly managed not only when multiple items are animating but also when there are pending items (nextItems).This enhancement contributes to a more robust handling of user input during quick transitions, aligning well with the overall goal of improving the animation behavior in such scenarios.
150-150: LGTM: Refined condition for creating animation itemsThe modification to the condition in the
getAnimationItemsfunction is a key improvement. By checking if the new stack's active key is the same as the old stack's active key, this change ensures that new animation items are created in the appropriate scenarios.This refinement directly addresses the core issue of the animation stack not updating when the active configuration remains unchanged but the instance changes. It contributes significantly to solving the problem of components not refreshing correctly after quick transitions, as outlined in the PR objectives.
Line range hint
1-385: Overall assessment: Effective solution to the stack animation issueThe changes implemented in this file collectively provide an effective solution to the issue described in the PR objectives. The modifications address the core problem of the animation stack not updating to new child component instances when the active configuration remains unchanged, particularly during quick transitions between components.
Key improvements include:
- Introduction of
nextItemsfor better management of upcoming animation items.- Refined logic for updating items during transitions.
- Addition of a DisposableEffect to ensure proper handling of
nextItemsafter exit animations.- Enhanced control over user input during animations.
- Improved condition for creating animation items.
These changes significantly enhance the robustness and correctness of the stack animation behavior, effectively resolving the strange behavior reported in the linked issue #792.
Fixes #792
Summary by CodeRabbit
New Features
Bug Fixes
Tests