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

State Retain and padding problem on Android 14 #985

Closed
Fed93 opened this issue Nov 3, 2023 · 3 comments · Fixed by #1089
Closed

State Retain and padding problem on Android 14 #985

Fed93 opened this issue Nov 3, 2023 · 3 comments · Fixed by #1089
Labels
bug Something isn't working

Comments

@Fed93
Copy link

Fed93 commented Nov 3, 2023

Hello,
I'm using Circuit for a Compose Multiplatform project and i've found two strange problems that only occur on Android 14

The first problem is about state retain using collectAsRetainedState extension or rememberRetained function.
When using state retain on the first start of the app and the user navigate to a detail page then come back to the previous page the state is not retained while, on the second attempt, the retention works perfectly.
This happens only on Android 14, i've also tested on Android 13 both on emulator and physical device.
I've also noticed a lag opening a detail page with a lot of component and logic in presenter on the first attempt, while in the second everything is really smooth.

The second problem is about padding of pages shown using navigator.
It seems that on Android 14 is not applying any padding while on Android 13 is centering the page for some kind of reason.

You can verify each problem with the demo project attached.

Using:
Circuit: v0.15.0, v0.16.0
Compose Multiplatform: v1.5.3, v1.5.10
Devices: Google Pixel 6 Pro (Android 14), Emulator (Android 13 and Android 14)

DemoRetainBug.zip

@kalinjul
Copy link

kalinjul commented Nov 3, 2023

I have a similar bug on Android 14.
When using GestureNavigationDecoration, goTo(DetailScreen) will create new instance of the previous presenter (which does not have the retained state)
Without GestureNavigationDecoration, this is not the case.

@Fed93
Copy link
Author

Fed93 commented Nov 3, 2023

Hi @kalinjul thanks for your tip.
I can confirm that removing GestureNavigationDecoration from NavigableCircuitContent let the app run smoothly and without issues. Also in demo app, removing the decorator results in same page visualisation of page on Android 13 (with components centered in screen)

@chrisbanes
Copy link
Contributor

Looked a bit into this this morning. The reason that this is happening is because we optional display ScreenA after navigating to ScreenB, so that the swipe back gesture works (i.e. you can see ScreenA behind). This is leading to a race condition with retained state (and saveable AFAICT) saving its value. So once the ScreenA -> B transition has completed, ScreenA is moved from the top of the stack to the 'optional' previous slot. This recomposition happens before the retained state in ScreenA has saved itself, therefore the state is recreated.

A few things to look more into:

  • Why is ScreenA being recreated? We use moveableContentOf so the composition should be re-used
  • Can we make saveIfRetainable happen faster?

@ZacSweers ZacSweers added the bug Something isn't working label Dec 29, 2023
ZacSweers pushed a commit that referenced this issue Jan 1, 2024
…tures (#1089)

The issue is caused by both the iOS and Android implementations
displaying multiple back records at the same time, for mostly the same
reason (so that swipes display the previous record). Both
implementations use a `Transition` to enable one-shot back events
(button presses, etc) to display an appropriate animation, which means
that we have 2 places where the previous record could be attached to
composition.

This works mostly fine, but does come with an issue with how we handle
state, which is through `movableContentOf`. Movable content can only be
attached to composition once, whilst still moving all of the state
around. If the content is attached twice, the second time will create a
new copy of the content (like a normal composable lambda), which is why
the state was 'being dropped'. It wasn't actually being dropped in the
state registries, the content just didn't get access to them.

Back to our use case. In the typical 'user swipes for back' scenario, we
manually display the previous record content so that the user sees it as
appropriate. However, when transition is run the `AnimatedContent` is
responsible for composing both sides of the transition, and thus both
the current and previous content will be attached to composition.
Hopefully you can see here the issue: we were attaching the previous
content twice, once in the `AnimatedContent` and again by our manual
call.

The fix is pretty simple, only attach the previous content once by
keeping the transition and manual composition calls exclusive.

Fixes #985.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants