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 ability to using miscellaneous Navigator #326

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

Conversation

Velord
Copy link

@Velord Velord commented Feb 6, 2024

ISSUES SOLVED: #190
POSSIBLE SOLVED: #265

This is further discussion of #302
We ended with the following statements:

  1. Internal stable API can not be changed. Such as Stack API.
  2. Everything should be added as Extensions on top of the Core APIs
  3. Backward compatibility is a must

I added possibility to write and change custom Navigator with simple ONE line of a code.

public typealias NavigatorCreator = (
    screens: List<Screen>,
    key: String,
    stateHolder: SaveableStateHolder,
    disposeBehavior: NavigatorDisposeBehavior,
    parent: Navigator?
) -> Navigator

// Choose your Navigator or leave default
public fun summonNavigatorCreator(): NavigatorCreator = DefaultNavigatorCreator()

Navigator now is abstract class that I leave as it is.
Introduced 2 New navigators:

  1. DefaultNavigator. It is just wrapper around Navigator
  2. ExtendedNavigator. Resolves the issue of screen and action based transition.
    Marked as @ExperimentalVoyagerApi.
    Implements idea of Add new Stack that requires the caller #302 (comment)

Pros:

  1. Added possibility to replace Navigator. Navigator now is abstraction that everyone can work with.
  2. Added Screen and StackAction based transition simultaneously by ExtendedNavigator.
  3. Added demo "Transition" that shows what that approach could bring.

Cons:

  1. Screen and StackAction based transition limited to the LAST SCREEN. You can't take any screen.
    More examples when and where it wiill come in handy check this: Add new Stack that requires the caller #302 (comment)

Please @DevSrSouza make a glance.

@Velord Velord force-pushed the feature/NavigatorExtended branch 2 times, most recently from 3b3ca42 to 43020f5 Compare February 6, 2024 16:40
@Velord Velord force-pushed the feature/NavigatorExtended branch from 43020f5 to 2fb4f15 Compare February 6, 2024 16:41
Copy link
Collaborator

@DevSrSouza DevSrSouza left a comment

Choose a reason for hiding this comment

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

I still don't know why this API is required, there is couple examples on the project creating Custom Navigator with Navigator it self as base, for example BottomSheetNavigator.

Also the transitions problem is not a problem, I have tested here and I was able to reproduce all your transitions with targetState and initialState. (#302 (comment))

Comment on lines 55 to 66
val isPush = navigator.lastAction.isPush()
val isPop = navigator.lastAction.isPop()
// Define any Screen you want transition must be from
val isInvokerTransitionScreen = navigator.lastAction?.invoker == TransitionScreen
val isInvokerFadeScreen = navigator.lastAction?.invoker == FadeScreen
val isInvokerShrinkScreen = navigator.lastAction?.invoker == ShrinkScreen
val isInvokerScaleScreen = navigator.lastAction?.invoker == ScaleScreen
// Define any Screen you want transition must be to
val isTargetTransitionScreen = navigator.lastItem == TransitionScreen
val isTargetFadeScreen = navigator.lastItem == FadeScreen
val isTargetShrinkScreen = navigator.lastItem == ShrinkScreen
val isTargetScaleScreen = navigator.lastItem == ScaleScreen
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val isPush = navigator.lastAction.isPush()
val isPop = navigator.lastAction.isPop()
// Define any Screen you want transition must be from
val isInvokerTransitionScreen = navigator.lastAction?.invoker == TransitionScreen
val isInvokerFadeScreen = navigator.lastAction?.invoker == FadeScreen
val isInvokerShrinkScreen = navigator.lastAction?.invoker == ShrinkScreen
val isInvokerScaleScreen = navigator.lastAction?.invoker == ScaleScreen
// Define any Screen you want transition must be to
val isTargetTransitionScreen = navigator.lastItem == TransitionScreen
val isTargetFadeScreen = navigator.lastItem == FadeScreen
val isTargetShrinkScreen = navigator.lastItem == ShrinkScreen
val isTargetScaleScreen = navigator.lastItem == ScaleScreen
val invoker = this.initialState
val target = this.targetState
// Define any StackEvent you want transition to be
val isPush = navigator.lastEvent == StackEvent.Push
val isPop = navigator.lastEvent == StackEvent.Pop
// Define any Screen you want transition must be from
val isInvokerTransitionScreen = invoker == TransitionScreen
val isInvokerFadeScreen = invoker == FadeScreen
val isInvokerShrinkScreen = invoker == ShrinkScreen
val isInvokerScaleScreen = invoker == ScaleScreen
// Define any Screen you want transition must be to
val isTargetTransitionScreen = target == TransitionScreen
val isTargetFadeScreen = target == FadeScreen
val isTargetShrinkScreen = target == ShrinkScreen
val isTargetScaleScreen = target == ScaleScreen

Copy link
Author

Choose a reason for hiding this comment

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

I did not know about that API. I have checked, works as expected. Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like if you can open a PR with this transitions examples using the initialState and targetState API to we have on our oficial Voyager samples.
Could be also the sample base that we could use to create a new Transition API for Screens and test on it.

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Feb 6, 2024

The API currently, for creating a custom Navigator is flexible enough, example:

public val LocalMyCustomNavigator: ProvidableCompositionLocal<MyCustomNavigator?> =
    staticCompositionLocalOf { null }

interface MyCustomScreenType : Screen {
    val header: @Composable () -> Header
}

class MyCustomNavigator(
    val voyagerNavigator: Navigator
) : Stack<MyCustomScreenType> by navigator as Stack<MyCustomScreenType> {

   public val lastItem: MyCustomScreenType by derivedStateOf {
        lastItemOrNull ?: error("MyCustomNavigator has no screen")
    }

   fun yourOwnNavigatorFunctions() {}
}

@Composable
fun MyCustomNavigator(initialScreen: MyCustomScreenType) {
   Navigator(initialScreen, ....) { navigator ->
       val myCustomNavigator = remember(navigator) { MyCustomNavigator(navigator) }
       CompositionLocalProvider(LocalMyCustomNavigator provides myCustomNavigator) {
            Scaffold(
               topBar = { myCustomNavigator.lastItem.header() }
               content = { CurrentScreen() }
            )
       }
   }
}

@Velord
Copy link
Author

Velord commented Feb 6, 2024

Indeed you can create your own Navigator.
But my code is focused to add flexibility in Navigator itself level. Not at his child level, nor who will use it as parameter level, nor who wanna create Navigator directly level.
Please, take a look at this picture https://miro.com/app/board/uXjVNwLqM2U=/
Navigator2

What do you think could it add more control and flexibility for users ?

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Feb 6, 2024

The examples that I shared before does not require to change anything on the navigator, you can accomplish what you want to do it this and also, the issues that this PR fixes, the code snippet that I have shared using targetState & initialState already cover the cases, so I don't know why we need this API, even if adds more flexibility, I don't see the uses cases that the currently API does not already have.

TabNavigator, BottomSheetNavigator are pretty simple implementations for "Custom Navigators", they are so simple that you can replicate it pretty easy and customize as far as you want, so, adding de ability to Voyager custom navigators extension have a "custom navigator factory", I don't see much use cases for that and at the sametime, increase alot the complexity of the Core API of the Navigator, and complexity is the main point that the library is trying to move away.

@Velord
Copy link
Author

Velord commented Feb 7, 2024

"targetState & initialState already cover the cases" - its closed.

"TabNavigator, BottomSheetNavigator, MyCustomNavigator" - they are focused to add functionality on UI application level. They add some UI features for user experience.

Contrary to that "custom navigator factory" - is aiming to add flexibility on business logic level inside Navigator. The whole lib that focuses on UI features will be working as it works now without knowing what is the current implementation of Navigator is used.

" I don't see the uses cases that the currently API does not already have." - let's review some of them.

  1. Realism 100%. We need to write A/B tests.
    We obtain app configuration. Some users based on that configuration must be placed in specific feature. Inside that feature we need to log every movement of the user. Also it can be custom movement. For example track user only when he go back. Implementation could be that
            val appconfig = // get config
            val user = // get user
            appconfig.setUser(user) 
            val navigator = if (appconfig.isUserMustGoToFeatureX) {
                AnalyticNavigatorCreator(FirebaseAnalytic)
            } else {
                DefaultNavigatorCreator()
            }
            summonNavigatorCreator = navigator
            Navigator(Screen) {
                // transition
            }

Every Navigator after that will be with trackable feature. We don't need to change in specific places certain Navigator.

  1. Realism 95%. We are developing big/medium FInTech app. We carefully pay attention to user safety. His money are our top priority.
    Let's consider user behaviour. User don't move or away from device. When this happens we need to clear all screens and place user to the login/relogin flow. You could see that behaviour in almost every bank app or stock trading market or crypto wallet.
    Implementation could be that
val min5InMilliSeconds = 1000 * 60 * 5L
// There could be originScreen: Screen as a parameter. It will be first Screen after reset.
summonNavigatorCreator = SelfResetNavigatorCreator(min5InMilliSeconds)
Navigator(TransitionScreen) {
      TransitionDemo(it)
}


private class SelfResetNavigator @InternalVoyagerApi constructor(
    val resetAfter: Long,
    ...
) : Navigator(screens, key, stateHolder, disposeBehavior, parent, stack) {

    private val timeStamp = MutableSharedFlow<Unit>(
        extraBufferCapacity = 1,
        onBufferOverflow = BufferOverflow.DROP_OLDEST
    )

    private val scope = CoroutineScope(Job() + Dispatchers.Default)

    init {
        observeTimeStamp()
    }

    private fun observeTimeStamp() {
        scope.launch {
            timeStamp.collectLatest {
                delay(resetAfter)
                if (canPop) {
                    popAll()
                }
            }
        }
    }

    override fun push(item: Screen) {
        timeStamp.tryEmit(Unit)
        stack.push(item)
    }
    
    // other functions
}
  1. Realism 0.1%.. Voyager team decided to add some new(or replace existent) Navigator as a standard library. Also it could help rework(make simpler) Stack API without additional pain.

A added SelfResetNavigator as example to PR to see how it might works.
Delay is 5 sec.
https://github.com/adrielcafe/voyager/assets/33905170/5fcec70d-87d7-430d-892d-c679db281be3

Are there relevant use cases ? What do you think ?

@Velord Velord mentioned this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants