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

Adds support for result passing between screens #128

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kashif-E
Copy link
Contributor

Closes #110
Adds support to pass results between screen for Navigator and BottomSheetNavigator.

using Navigator.

Passing the result

    //you can also pass your own key or key of the screen will be the default result key
     navigator.popWithResult("This is result")

     navigator.popUntilWithResult(
     { screen -> screen is ProductDetailsScreen },
     data)

Getting the result

 destinationsNavigator
        .getResult<Boolean>(LocalNavigator.currentOrThrow.lastItem.key)
        .value
        ?.let { result -> onAddedToCart(result) }

using BottomSheetNavigator:

Passing the result

 bsNavigator.hideWithResult(Pair(item.id, item.data), "my-key")

Getting the result:

   bottomSheetNavigator.getResult<Pair>(screenKey = "my-key").value?.let {result->
        //do something with the result here
    }

@Kashif-E
Copy link
Contributor Author

@adrielcafe @DevSrSouza

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Apr 24, 2023

@Kashif-E HI, we will have a look on this soon (testing and trying on our samples), don't know if it will be for 1.0 that will be soon released. Btw thanks for the contribution!

I will let you know soon!

@Kashif-E Kashif-E closed this Jun 15, 2023
@Kashif-E Kashif-E force-pushed the main branch 2 times, most recently from 97053dc to 2792058 Compare June 15, 2023 16:39
# Conflicts:
#	voyager-navigator/src/commonMain/kotlin/cafe/adriel/voyager/navigator/Navigator.kt
@Kashif-E Kashif-E reopened this Jun 15, 2023
@Kashif-E Kashif-E requested a review from jakoss June 16, 2023 18:33
@Kashif-E
Copy link
Contributor Author

@DevSrSouza @adrielcafe

@FunkyMuse
Copy link

Hey guys, any updates on this?

@Kashif-E
Copy link
Contributor Author

@DevSrSouza any update ?

@Kashif-E
Copy link
Contributor Author

@DevSrSouza @adrielcafe this is something we really need can you please help here?

@Kashif-E Kashif-E closed this Aug 12, 2023
@jakoss
Copy link

jakoss commented Aug 13, 2023

image

@Kashif-E Have you given up on this? :(

@Kashif-E
Copy link
Contributor Author

@jakoss i hate to admit this but yes, but, i am thinking about making my fork a separate library.

@jakoss
Copy link

jakoss commented Aug 13, 2023

@jakoss i hate to admit this but yes, but, i am thinking about making my fork a separate library.

More fragmentation for navigation libraries for compose, but i understand the reason. This thread is too ignored at this point

@DevNatan DevNatan reopened this Oct 4, 2023
@DevNatan
Copy link
Collaborator

DevNatan commented Oct 4, 2023

@Kashif-E Can we get back here? Forgive us for inactivity, this feature is very important and we want it on Voyager

Please let me know if you can continue here, I will take care of reviewing it and ensuring it is merged, if not possible I will continue it myself. Thank you in advance for your time and contribution!! 🙏🏽

@DevNatan DevNatan added the enhancement New feature or request label Oct 4, 2023
@Syer10
Copy link
Contributor

Syer10 commented Oct 4, 2023

I don't feel like this implementation is ideal for Voyager, I would prefer something like the one displayed here would be better.
https://www.droidcon.com/2022/08/02/painless-typesafe-jetpack-compose-navigation-with-voyager/

I feel it clutters the navigator when it could just be a module added on-top of it

@Syer10
Copy link
Contributor

Syer10 commented Oct 4, 2023

1 issue I find with this is that it uses String keys, I find that with Kotlin I would prefer to have a TypeSafe key to Value object that allows managing types inherently. Something like this

interface TypedKey<T>

object DataId : TypedKey<Long>

private val typedKeyMap: Map<TypedKey<*>, Any>

fun <T> putInTypedKeyMap(typedKey: TypedKey<T>, value: T): T {
    typedKeyMap[typedKey] = value
}

fun <T> getFromTypedKeyMap(typedKey: TypedKey<T>): T {
    return typedKeyMap[typedKey] as T
}

fun inputMyData() {
    putInTypedKeyMap(DataId, 100L)
}

fun getMyData() {
    val myDataId: Long = getFromTypedKeyMap(DataId)
}

@DevNatan
Copy link
Collaborator

DevNatan commented Oct 4, 2023

@Syer10 Great! What we want here is the "possibility of passing results between screens" the way, api, imp in which this will be done has not yet been defined. Also agree that such a feature should be something "more" and not tied to the navigator. What you said will be considered, thanks!

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Oct 5, 2023

@Syer10 what you said is perfectly right. However, there are a few things that should be considered here

  1. Strings are chosen over type-safe keys because screen keys are inherently strings, eliminating the need for custom keys for each result.
  2. Binding results to navigators enhances developer ergonomics, simplifying result access and aligning with screen-based mental models.

@DevNatan
Copy link
Collaborator

DevNatan commented Oct 6, 2023

Both are valid and reasonable, do you guys @Kashif-E @Syer10 have any example code (using Voyager) so that we can reach a consensus on what will be best for everyone? I'm also considering how this will work for Web platform and Multi-module feature (ScreenProvider)

@@ -114,33 +116,41 @@ public class Navigator @InternalVoyagerApi constructor(
public val parent: Navigator? = null
) : Stack<Screen> by screens.toMutableStateStack(minSize = 1) {

public val level: Int =
parent?.level?.inc() ?: 0
public var isNavigating: Boolean by mutableStateOf(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently, this property is never used

Comment on lines +140 to 144
@OptIn(ExperimentalVoyagerApi::class, InternalVoyagerApi::class)
@Composable
public fun saveableState(
key: String,
screen: Screen = lastItem,
content: @Composable () -> Unit
key: String, screen: Screen = lastItem, content: @Composable () -> Unit
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing did change here right? can you revert this?

Comment on lines +146 to +151
if (!(screen.key.contains("onboarding", ignoreCase = true) && stateKey.contains(
"onboarding", ignoreCase = true
))
) {
stateKeys += stateKey
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this "onboarding" validation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was our own implementation we can ignore this

Comment on lines -158 to +168
MultipleProvideBeforeScreenContent(
screenLifecycleContentProviders = composed,
MultipleProvideBeforeScreenContent(screenLifecycleContentProviders = composed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be reverted

Comment on lines -185 to +229
stateKeys
.asSequence()
.filter { it.startsWith(screen.key) }
.forEach { key ->
stateHolder.removeState(key)
stateKeys -= key
}
stateKeys.asSequence().filter { it.startsWith(screen.key) }.forEach { key ->
stateHolder.removeState(key)
stateKeys -= key
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can reverted as well

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Oct 14, 2023

I have baked your API without requiring to change any internal voyager APIs, could you try it out? If it works for you, we could provide this a result API examples.

val Navigator.navigationResult: VoyagerResultExtension
    @Composable get() = remember {
        NavigatorLifecycleStore.register(this) {
            VoyagerResultExtension(this)
        }
    }

// OR

@Composable
public fun rememberNavigationResultExtension(): VoyagerResultExtension {
    val navigator = LocalNavigator.currentOrThrow

    return remember {
        NavigatorLifecycleStore.get(navigator) {
            VoyagerResultExtension(navigator)
        }
    }
}

class VoyagerResultExtension(
    private val navigator: Navigator
) : NavigatorDisposable {
    private val results = mutableStateMapOf<String, Any?>()

    override fun onDispose(navigator: Navigator) {
        // not used
    }

    public fun setResult(screenKey: String, result: Any?) {
        results[screenKey] = result
    }

    public fun popWithResult(result: Any? = null) {
        val currentScreen = navigator.lastItem
        results[currentScreen.key] = result
        navigator.pop()
    }

    public fun clearResults() {
        results.clear()
    }


    public fun popUntilWithResult(predicate: (Screen) -> Boolean, result: Any? = null) {
        val currentScreen = navigator.lastItem
        results[currentScreen.key] = result
        navigator.popUntil(predicate)
    }

    @Composable
    public fun <T> getResult(screenKey: String): State<T?> {
        val result = results[screenKey] as? T
        val resultState = remember(screenKey, result) {
            derivedStateOf {
                results.remove(screenKey)
                result
            }
        }
        return resultState
    }
}

@Syer10
Copy link
Contributor

Syer10 commented Oct 14, 2023

+1 for @DevSrSouza's implementation

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Oct 16, 2023

this looks great @DevSrSouza. ill test this in our app and will let you know how this works

@osrl
Copy link
Contributor

osrl commented Oct 23, 2023

This could get into the documentation IMHO @DevSrSouza

@DevSrSouza
Copy link
Collaborator

Did it work for you @Kashif-E ?

@osrl we are planning to create a documentation page with "community extension and third party libraries"

@osrl
Copy link
Contributor

osrl commented Nov 6, 2023

Is it possible to use this snippet with a BottomSheetNavigator? @DevSrSouza

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Nov 7, 2023

@DevSrSouza it works, 🎆🎆 @osrl both mine and what @DevSrSouza proposed can be used with bottomsheet navigator

@osrl
Copy link
Contributor

osrl commented Nov 7, 2023

NavigatorLifecycleStore.register takes only Navigator so I couldn't find a way to use @DevSrSouza's code.

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Nov 9, 2023

@osrl you can use these extensions, remove the actual keyword




/**
 * Navigator
 */
private val results = mutableStateMapOf<String, Any?>()


actual fun Navigator.popWithResult(key: String, result: Any?) {
    results[key] = result
    pop()
}
actual fun Navigator.pushX(screen: Screen) {
    val existingScreen = items.firstOrNull { it.key == screen.key }
    if (existingScreen == null) {
        push(screen)
    }
}

actual fun Navigator.replaceAllX(screen: Screen) {
    if (items.last().key != screen.key && items.last().uniqueScreenKey != screen.uniqueScreenKey) {
        replaceAll(screen)
    }
}

actual fun Navigator.popUntilWithResult(predicate: (Screen) -> Boolean, result: Any?) {
    val currentScreen = lastItem
    results[currentScreen.key] = result
    popUntil(predicate)
}

actual fun Navigator.clearResults() {
    results.clear()
}

@Composable
actual fun <T> Navigator.getResult(screenKey: String): State<T?> {
    val result = results[screenKey] as? T
    val resultState = remember(screenKey, result) {
        derivedStateOf {
            results.remove(screenKey)
            result
        }
    }
    return resultState
}

/**
 * bottom sheet navigator
 */


@OptIn(ExperimentalMaterialApi::class)
actual fun BottomSheetNavigator.hideWithResult(result: Any?, key: String) {
    results[key] = result
    this.hide()

}

@Composable
actual fun <T> BottomSheetNavigator.getResult(screenKey: String): State<T?> {
    val result = results[screenKey] as? T
    val resultState = remember(screenKey, result) {
        derivedStateOf {
            results -= screenKey
            result
        }
    }
    return resultState
}

@AlexanderKatonaGetNetEurope

Hello, is it going to be supported by the library, or you prefer to keep it as extension function and everybody just copy paste it?

@0xZhangKe
Copy link

@DevSrSouza This works, it's great, but it seems a little cumbersome to use, maybe I'm using it the wrong way?
If you want to get the result of the second page on the first page, you need to get the Key of the second page.

However, due to life cycle issues, the first page can only get the current value by directly using Navigator.lastItem.key. The Key of the page, not the Key of the second page,so you need to use the following method to obtain the result on the first page:

var lastItemKey: String? by rememberSaveable {
    mutableStateOf(null)
}
Box(modifier = Modifier.clickable {
    navigator.push(SecondPageScreen())
    lastItemKey = navigator.lastItem.key
})
if (lastItemKey != null) {
    val result by navigator.navigationResult
        .getResult<String>(lastItemKey!!)
}

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Jan 9, 2024

@0xZhangKe it does not matter what you use as key for example you use "papi chulo" as key when passing back result and use it on first screen

1 similar comment
@Kashif-E
Copy link
Contributor Author

Kashif-E commented Jan 9, 2024

@0xZhangKe it does not matter what you use as key for example you use "papi chulo" as key when passing back result and use it on first screen

@0xZhangKe
Copy link

@Kashif-E This means that the first page must clearly know the Key of the second page. If the two Screens are in different modules and cannot reference each other, then this will become a bit troublesome.
Generally speaking, this situation is It doesn't exist, but I'm currently encountering this problem.
My two pages belong to two modules. The first page discovers the second page through routing, so I can only do this.

@Kashif-E
Copy link
Contributor Author

Kashif-E commented Jan 10, 2024

@0xZhangKe try this

navigator.popWithResult( key="gender", result=true  ) //you can use any key here 

// use the result like this
 navigator.getResult<Boolean>("gender").value?.let {booli->
       navigator.popWithResult("phone", booli)
   }

@programadorthi
Copy link
Collaborator

I think that there is a confusion here. Voyager is Screen based navigation and if you needs the result it should be handled by the previous screen and not returned to the navigation stack action call.
So, doing something like behaviors below should be accepted:

interface PopResult<in T> {
    fun onResult(result: T)
}

class MyScreen : Screen, PopResult<T> {
    fun onResult(result: T) {
        // handle the popped result
    }
}

// Inside screen Content() that will be popped
val navigator = LocalNavigator.currentOrThrow
navigator.pop() // It is a sync call and will not trigger recomposition immediatelly
val nextScreen = navigator.lastItemOrNull as? PopResult<T>
nextScreen?.onResult(...)

@fabriciovergara
Copy link

fabriciovergara commented Feb 9, 2024

meanwhile there is no official support, I'm currently using this if anyone needs, it enforces some type safety by sending "request" to the next screen. It's almost the same as DevSrSouza proposal, but nav library agnostic.

Ofc, if it's not implemented correctly an value can be held in memory until the app is closed by the user and even potentially exceed parcelable size limit

// Root
ProvideNavResultStateHolder() {
   Navigator(Screen1())
}


// Screen1
val contract = rememberNavResultContract<Int>(key)
val response = contract.response.value
val isCurrentScreen = navigator.isCurrentScreen(key)

LaunchedEffect(response, isCurrentScreen) {
  if(!isCurrentScreen) return@LaunchedEffect
  if(response !is NavResult.Value) return@LaunchedEffect
  // Do something
  contract.consume()
}

Button(
  onClick = { 
      navigator.push(Screen2(contract.request))
  }
) {
   Text("Push")
}

// Screen2
val contract = rememberNavResultContract<Int>(request)

Button(
  onClick = { 
      contract.send(0)
      navigator.pop()
  }
) {
      Text("Pop")
}

@Vaorhd
Copy link

Vaorhd commented Feb 16, 2024

We all are still waiting for this PR.
@DevNatan

@SnowVolf
Copy link

Any updates?

@PavlikPetr
Copy link

We all are still waiting

@wiryadev
Copy link
Contributor

@Composable
public fun rememberNavigationResultExtension(): VoyagerResultExtension {
    val navigator = LocalNavigator.currentOrThrow

    return remember {
        NavigatorLifecycleStore.register(navigator) {
            VoyagerResultExtension(navigator)
        }
    }
}

Where is this register function located?

@Syer10
Copy link
Contributor

Syer10 commented May 13, 2024

Where is this register function located?

It was renamed to get in one of the recent releases. It is basically a getOrCreate

@wiryadev
Copy link
Contributor

Where is this register function located?

It was renamed to get in one of the recent releases. It is basically a getOrCreate

thanks, did look into it but not quite sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Add ability to "set result" like the library Guia has.