Screen can get the service from wrong scope during back animation

[matejdro]

With the following version of the example-simple: https://github.com/matejdro/simple-stack-compose-integration/tree/service-reuse-issue

1. Open the sample
2. Note that the background is red and screen has its own CommonSharedService (note displayed hashcode)
3. Tap on the button to go to the second screen
4. Note that the second screen also has its own CommonSharedService (displayed hashcode is different from the first)
5. Press turn into green button
6. Go back
7. Note that the first screen is now also green, even though its own service's color have not changed - first screen now got second screen's service even though that screen is in the process of being removed.

I feel like this behavior might not be intended? If each screen has its own scope, then it should receive service from their own scope first, right? I realize that simple stack always takes the latest available scope first, but this might not be the best in all cases.

Not sure if this is a bug, just want to start a discussion regarding this issue. What is the best way to workaround this?

[Zhuinden]

Curiously, this is the one thing I've been wondering about ever since creating backstack.lookupService<T>(), I specifically remember thinking about this exact scenario about 4 years ago, but it somehow just never came up.

What I do know is that this is also why I added lookupFromScope() in case something goes wrong.

In fact, to my surprise, this is what the changelog says:

-Simple Stack 1.13.1 (2018-11-25)

...

ADDED: lookupFromScope() and canFindFromScope() methods that begin the lookup from the specified scope, instead of the active-most one. This is to allow safer look-ups when the same service tag is used in different scopes.

So apparently I knew about this 4.5 years ago, but somehow it never came up in my usecases.

Knowing that, the following works:

            val backstack = LocalBackstack.current
            val key = LocalComposeKey.current as FirstKey

            val eventHandler = rememberService<ActionHandler>()
            val commonSharedService = remember {
                backstack.lookupFromScope<CommonSharedService>(
                    key.scopeTag,
                    CommonSharedService::class.java.name
                )
            }

Not exactly optimal, there's clearly no reified helper function for it in simple-stack-extensions at this time (hence the manual ::class.java.name) because we never ended up running into this problem, but this is the theoreticaly fix for this, as you do look up the nearest service by that name, and scopes are only evicted when the state changes fully execute.

I'm not sure what to do about it as this is happening according to lookupService's design, but the solution really is to either use lookupFromScope or use a specific service tag rather than T::class.java.name if there are multiple instances of the same class. 🤔

[matejdro]

So, something like this would be a solution?

https://github.com/matejdro/simple-stack-compose-integration/commit/fffbc52f444dbbddab18ec8187ae52a83e5240f1

Maybe documentation should emphasize lookupFromScope over lookupService, since there is this gotcha with lookupService?

[Zhuinden]

I've been using by lazy { backstack.lookup<T>() } for so long, I'll need to think of a good plan for this one

[Zhuinden]

I added a rememberServiceFrom() although I feel like I will still need to add documentation and think a bit more about this.

This generally hasn't come up as I usually have 1 specific service type for screen so that you can utilize lookup across screens via implicit parents.

[Zhuinden]

I think this is "resolved" by https://github.com/Zhuinden/simple-stack-extensions/commit/9c7d00f5b1958b5bb97ff80e54f8b3b1f4a5b799 and https://github.com/Zhuinden/simple-stack-extensions/commit/5471b83997fe6fe6acc447c10bc1b6f7c1de8562 + reusing service name is not technically "disallowed", disallowing it would be a breaking change.

It could be hypothetically possible to add a "strict mode" where this case throws but not sure if that really helps.

[matejdro]

Yes, this has been resolved. Sorry, I forgot to close it.