-
Notifications
You must be signed in to change notification settings - Fork 76
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
WIP RetainedStateHolder #1168
base: main
Are you sure you want to change the base?
WIP RetainedStateHolder #1168
Conversation
This is just some tinkering to resolve #116 and after talking with @bryanstern. Wanna iterate on this more and see if we can make this simpler, and also possibly explore implementing an analogous `RetainedStateHolder`. Another thing this highlights is the fact that we always initially show a progress indicator even when restoring from a loaded state, wonder what we can do to improve that.
presenter = presenter!!, | ||
ui = ui!!, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: loading. I think the solution here is to do something like having a map of nav items to their presenter state that is instantiated and rememberRetained above this section.
And then have a "presenter facade" which switches state to the presenter state that is currently being shown.
This is effectively what I'm doing in my project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because we don't retain state for root back stack changes, only back stack additions (effectively).
This has been at the back of my mind for a while (and mildly annoys me every time I use Tivi).
So this change stores the tab index across app starts, but the state of that tab won't be retained if you switch between them.
There is a comment in the code somewhere from when I wrote the retain backstack stuff, let me find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find it now, but I think this might actually be as simple as making buildCircuitContentProviders
a bit smarter, and manually retaining any dropped root records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on this in #1173
Sadly this did not Just Work when I converted to retained, gonna keep digging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but all of this is already handled if you put the root screens into the SaveableBackStack
, right?
The back stack will automatically save/restore itself, so everything is persisted. I do exactly this in Tivi and it just works™️
Just thinking out loud, as there's a lot of code in this PR. Maybe I'm missing the use case though?
It doesn't work in this PR's case because content is all swapped inside the tab pager. As far as the backstack's concerned, they're all just one screen ( It actually does work, I just forgot the UI state that we see here is mostly all rememberSaveable from compose UI. |
This is just some tinkering to resolve #116 and after talking with @bryanstern. Wanna iterate on this more and see if we can make this simpler, and also possibly explore implementing an analogous
RetainedStateHolder
.Another thing this highlights is the fact that we always initially show a progress indicator even when restoring from a loaded state, wonder what we can do to improve that.
Screen_recording_20240131_182803.mp4