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.

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

Dispose Screens only when transition ends #436

Merged
merged 10 commits into from
Jun 4, 2024
Merged

Conversation

DevSrSouza
Copy link
Collaborator

ISSUE SOLVED: #106

To use this new behavior you have to disable disposeBehavior.disposeSteps and use the ScreenTransition.disposeScreenAfterTransitionEnd = true

Navigator(
    ....
    disposeBehavior = NavigatorDisposeBehavior(disposeSteps = false)
) {
    ScreenTransition(
       navigator = it,
       ...,
       disposeScreenAfterTransitionEnd = true
    )
}

) {
// This can be costly because is checking every single item in the list
// we should re evaluate how validation works, maybe validating screen keys or ===
val previousItems = rememberPrevious(navigator.items)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should collect all navigator items changes.

Case:

We have navigator with transitions, it's stack: A -> B -> C.
User presses "back" 2 times quickly, so the stack now: A.

IMPORTANT NOTE: the transition was not completed during the entire stack change.

After all these actions, screens B, C should be disposed. But as i understand from your solution, only screen B will be disposed

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@shpasha shpasha Jun 4, 2024

Choose a reason for hiding this comment

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

So my idea is simple:

Collect all screens created during transitions (because during slow transition crazy user can navigate to and from different screens). After completion of transition compare collected screens with idled stack. Then dispose screens that are no longer in the navigation stack

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeh, I have being testing exactly that previously and I was able to reproduce it! I will try your suggestion.

@DevSrSouza
Copy link
Collaborator Author

DevSrSouza commented Jun 4, 2024

@shpasha I have bring your solution instead of previousItems, I have tested here and it seems pretty good. Can you test your use cases as well?

This way we can let as experimental and rethink if a Channel with Local Composition is a better solution in the future, for now, I think let this responsibility to the actual Transition component is safer and makes more sense and also, does not do any changes on Navigator Core

onDispose {
val newScreenKeys = navigator.items.map { it.key }
screenCandidatesToDispose.value += currentScreens.filter { it.key !in newScreenKeys }
.map { ScreenData(it.key, it) }
Copy link
Contributor

@shpasha shpasha Jun 4, 2024

Choose a reason for hiding this comment

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

Actually, i don't understand why I wrote this line 😄

I think we can get rid of ScreenData without any problems. Just save Screen only

@@ -104,8 +167,34 @@ public fun ScreenTransition(
},
modifier = modifier
) { screen ->
if (this.transition.targetState == this.transition.currentState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest changing the condition a little:

if (this.transition.targetState == this.transition.currentState && disposeScreenAfterTransitionEnd) {
 ...
}

and get rid of disposeScreenAfterTransitionEnd check in LaunchedEffect


val currentScreens = navigator.items

DisposableEffect(currentScreens) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to wrap 142-148 lines in if condition if (disposeScreenAfterTransitionEnd) { ... }

@DevSrSouza DevSrSouza merged commit db387ca into main Jun 4, 2024
1 check passed
@DevSrSouza DevSrSouza deleted the dispose-transition branch June 4, 2024 23:06
@shpasha
Copy link
Contributor

shpasha commented Jun 5, 2024

@DevSrSouza thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants