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

Add lifecycle aware Presenters #1282

Merged
merged 15 commits into from
May 26, 2024
Merged

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Mar 13, 2024

This PR adds a new Lifecycle interface, which presenters and UI can observe to know when they are 'paused'. The API is rudimentary at the moment and will change before this is ready to land.

A bundled PauseablePresenter class can be extended, enabling clients to automatically add pausing ability to their presenters. This impl will simply return the last emitted UiState when the presenter is paused. Again, name TBD.

Other things:

  • GestureNavigationRetainedStateTest and GestureNavigationSaveableStateTest have been combined into a parameterized GestureNavigationStateTest. This new test also now tests CupertinoGestureNavigationDecoration so we get extra coverage.
  • Changed rememberRetained's key param to Any to be consistent with all of the other remember functions.

TODO:

  • Saveable is currently broken by these changes. This needs to be fixed before landing.
  • Remove all of the logging code.
  • Add comments
  • Fix the last remaining failing test

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Can you unpack the change from key: String? -> key: Any? for me? I didn't quite follow from the PR diff alone why that was necessary. Otherwise a couple nits around naming. Any thoughts on the CI failure? If not I can look into it

@chrisbanes
Copy link
Contributor Author

Can you unpack the change from key: String? -> key: Any? for me? I didn't quite follow from the PR diff alone why that was necessary.

So I thought it would be good to add a key param to pausableState, but the problem is that it wraps both retained and saveable state. Saveable state (like all of the other remembers) uses a Any key, but retained uses a String key for some reason. There's no real benefit to using String AFAICT, so I've migrated it to Any to be consistent with everything else.

Any thoughts on the CI failure? If not I can look into it

It's one last failing test which I need to fix. Going to look at it asap.

@chrisbanes chrisbanes marked this pull request as ready for review May 26, 2024 13:31
@chrisbanes chrisbanes changed the title [WIP] Try out pausing presenters Add lifecycle aware Presenters May 26, 2024
@ZacSweers
Copy link
Collaborator

Saveable state (like all of the other remembers) uses a Any key, but retained uses a String key for some reason.

are you possibly mixing up inputs and keys? rememberSaveable also uses String for the key param (source).

image image

@chrisbanes
Copy link
Contributor Author

SaveableStateHolder uses Any keys: https://developer.android.com/reference/kotlin/androidx/compose/runtime/saveable/SaveableStateHolder

Why there's a difference, I don't really know. Happy to revert back to a String, I don't feel strongly here.

@ZacSweers
Copy link
Collaborator

yeah let's keep them consistent for the rememberRetained functions for now 👍

@chrisbanes
Copy link
Contributor Author

Side note: I think adding a RetainedStateHolder analogy to SaveableStateHolder would be great. I think we could get rid of a bunch of the nested RetainedStateRegistrys. That can wait though.

@ZacSweers
Copy link
Collaborator

Yes definitely. I started tinkering in #1168 but never picked it back up

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Last bit before I hit merge - do you mind adding a blurb to the Unreleased section of the CHANGELOG.md? I can add one post-merge too if you're busy

@ZacSweers ZacSweers enabled auto-merge May 26, 2024 19:03
@ZacSweers ZacSweers added this pull request to the merge queue May 26, 2024
Merged via the queue into slackhq:main with commit bb09273 May 26, 2024
5 checks passed
@ZacSweers ZacSweers deleted the cb/pause-presenter branch May 26, 2024 19:15
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2024
…ents (#1354)

This PR migrates `NavigableCircuitContent` to use `SaveableStateHolder`,
rather than our hand-rolled
`SaveableStateRegistryBackStackRecordLocalProvider`.

I don't know the history behind
`SaveableStateRegistryBackStackRecordLocalProvider`, but
`SaveableStateHolder` is the first party solution for this. It is used
by AndroidX Navigation, so we can assume it is well tested.

This PR relies on a bunch of `movableContent` fixes added in #1282 (I
had to add a similar one in this PR for
`CupertinoGestureNavigationDecoration`).

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

Successfully merging this pull request may close these issues.

2 participants