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

WIP RetainedStateHolder #1168

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

WIP RetainedStateHolder #1168

wants to merge 12 commits into from

Conversation

ZacSweers
Copy link
Collaborator

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

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!!,
)
}
Copy link
Contributor

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.

Copy link
Contributor

@chrisbanes chrisbanes Feb 1, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor

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

@ZacSweers
Copy link
Collaborator Author

Sadly this did not Just Work when I converted to retained, gonna keep digging

Copy link
Contributor

@chrisbanes chrisbanes left a 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?

@ZacSweers
Copy link
Collaborator Author

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 (HomeScreen)

It actually does work, I just forgot the UI state that we see here is mostly all rememberSaveable from compose UI.

@ZacSweers ZacSweers mentioned this pull request May 26, 2024
4 tasks
@ZacSweers ZacSweers changed the title WIP persist tab state WIP RetainedStateHolder Jul 20, 2024
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.

Sample app does not restore state when changing tabs
3 participants