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.

Sign up for GitHub

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

Jump to bottom

Add lifecycle aware Presenters #1282

Merged
merged 15 commits into from
May 26, 2024
Merged

Add lifecycle aware Presenters #1282

merged 15 commits into from
May 26, 2024

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Mar 13, 2024 •
edited

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

chrisbanes
chrisbanes commented May 23, 2024
View reviewed changes
...uit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/NavigableCircuitContent.kt Show resolved Hide resolved
circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/PauseablePresenter.kt Outdated Show resolved Hide resolved
circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/PauseablePresenter.kt Outdated Show resolved Hide resolved
circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/CircuitContent.kt Outdated Show resolved Hide resolved
...oidUnitTest/kotlin/com/slack/circuit/foundation/NavigableCircuitViewModelStateAndroidTest.kt Show resolved Hide resolved
circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/Lifecycle.kt Outdated Show resolved Hide resolved
ZacSweers
ZacSweers reviewed May 24, 2024
View reviewed changes
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

circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/Lifecycle.kt Outdated Show resolved Hide resolved
circuit-foundation/src/commonMain/kotlin/com/slack/circuit/foundation/Lifecycle.kt Outdated Show resolved Hide resolved
@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

ZacSweers
ZacSweers approved these changes May 26, 2024
View reviewed changes
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
@chrisbanes chrisbanes mentioned this pull request May 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2024
@chrisbanes
Fix saveable state being restored when using reset root navigation ev…
ae42097 
…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
Reviewers

@ZacSweers ZacSweers ZacSweers approved these changes

Assignees
No one assigned
Labels
cla:signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@chrisbanes @ZacSweers