KaustubhPatange / navigator

A small navigation library for Android to ease the use of fragment transactions & handling backstack (also available for Jetpack Compose).
Apache License 2.0
99 stars 5 forks source link

Feature request : Option to not add current route to backstack when navigating #16

Closed Tolriq closed 3 years ago

Tolriq commented 3 years ago

Something like noHistory : Boolean.

For example for a welcome screen that should not be returned to after navigating to real content while still allowing a single root route and proper integrated animations when using that route.

KaustubhPatange commented 3 years ago

This is simple, all you have to do is use popUpTo just like the Navigation Component or recall the Fragment Transactions.

Let's take an example:

Consider the navigation route, ScreenA -> ScreenB -> Main. We are currently at ScreenB, but now we need to navigate to Main such that the navigation stack should look like ScreenA -> Main. This problem can be solved easily using popUpTo option.

controller.navigateTo(Main) {
    popUpTo(ScreenB) // default inclusive
}
Tolriq commented 3 years ago

Yes but you are coupling the screen with the route. Screens should know nothing about navigation internals (and multiple routes could display the same destination).

If we move those up it brings repetitive code like

if (noHistory) {
                    val currentDestination = controller.getAllHistory().firstOrNull()
                    if (currentDestination != null) {
                        popUpTo(currentDestination)
                    }
                }
if (clearTop) {
                    val currentDestination = controller.getAllHistory().lastOrNull()
                    if (currentDestination != null) {
                        popUpTo(currentDestination)
                    }
                }

That sounds like things that could be native to the library

KaustubhPatange commented 3 years ago

Screens should know nothing about navigation internals (and multiple routes could display the same destination). That sounds like things that could be native to the library

This is something that should be solved within the app & not by the library. A library is there to make your workflow easier, it gives you functionality to use but it doesn't tell you how to get it architecturally correct. This sounds more of an architecture problem (app) rather than a library.

For example, if you don't want to couple the navigation logic in the screen, build a helper class that wraps this navigation library & use that in your code so that you don't directly depend on the library but on a wrapper that encapsulates it. You can then change the navigation system in the future if you want or if you don't like you can always depend on the reactive concept (Event, Logic & Handler) to make navigation stateful.

Tolriq commented 3 years ago

This sounds more of an architecture problem (app) rather than a library.

That's the whole point :) Having the Route forced to be known by the destination is an architecture problem more or less forced by the library. It's a wrong dependency direction.

Normal activity / fragment have the notion of clearTop and noHistory hence the request as it's common navigation pattern.

A fragment should not be tied to it's activity so that it can be reused. This is the same here, a destination should not be tied to a route so that it can be reused.

With multiple backstacks you could have RootRoute.X that shows the screen and SubRoute.Y that shows the same screen probably with different arguments or not.

For the current destination to navigate away without being kept in history it needs to know from what route it's displayed this is a wrong dependency direction. Add to this dialog route and in the future bottomsheet routes and it's a lot of burden put on the client to know what to do as a screen potentially be used in all 3 different cases with 3 different history.

Anyway yes we can hack something at higher level to mimic that, but those concept of inverted dependency and navigation pattern are IMO normal and basic use case.

KaustubhPatange commented 3 years ago

a destination should not be tied to a route so that it can be reused.

I think we are missing something, from what I see a destination or more specifically a Screen should not be reused. What I mean by that is one screen should not represent multiple screens (that can be changed through arguments) because each screen is unique (of course in cases like the progress bar i.e showing loading screen, etc. it is okay because it ties to the same logic coming from one state eg: classic MVI). You can show & hide some of the components based on the arguments but not the complete screen. If you need something like this then you should create a reusable component that can be reused to construct multiple screens.

This doesn't mean you cannot navigate to this screen from a different navigation route so you should tie Screen with Route because now you will always know what that route always represents. Navigation component does this exact same thing through composable extension function which binds your current route to the content of the lambda (same goes with simple stack i.e concept of ScreenKey which actually ties Screen). This is the same reason such navigation system doesn't enforce duplicate routes as this breaks the principle of navigation.

Summary: My point is simple Screen must be tied to a route for predictable navigation. You can always construct different screen using reusable components but don't use a Reusable Screen... that doesn't sound good.

The issue with fragment was even though you don't directly tie it to the activity / navigation system directly (suppose you are using Navigation Component) they actually do! The same concept goes if you have multiple feature modules where you expose the destination through a dependency injection pattern. We think we've reduced coupling but we don't since now we depend on @Inject able fragment at runtime.

With multiple backstacks you could have RootRoute.X that shows the screen and SubRoute.Y that shows the same screen probably with different arguments or not.

This is an antipattern & is completely against the navigation principle. As I said you should make each destination unique. If you are showing the same screen with two different routes then you are duplicating the logic. What you should do is create a set of arguments for a single route & use that route in nested navigation (navigator-compose will take care of the rest) as your nested navigation doesn't about parent navigation.

For the current destination to navigate away without being kept in history it needs to know from what route it's displayed this is a wrong dependency direction.

You showed the code in above comment similar to this val currentDestination = controller.getAllHistory().lastOrNull(). Now question is do we know what route is currentDestination variable represent? We don't we just know it is something that extends Route class so you can easily pop it.

Add to this dialog route and in the future bottomsheet routes and it's a lot of burden put on the client to know what to do as a screen potentially be used in all 3 different cases with 3 different history.

Dialogs can be and should be reused because they are dialogs. That's one of the things I don't like with the Navigation Component, they make you believe that a dialog is a part of navigation but it's not since it is always going to be displayed on the top of the existing Screen. That's why my way of creating dialog is to explicitly tell Controller to create a dialog and then show it.

Also, you should not stack multiple dialogs over one another thinking that you would pop it in future. That's not how they are used. They are dialogs and should be used as dialogs, not as Screens i.e allow users to interact with the screen by showing some options, etc.

A dialog will never be a problem as during creation you tie a Dialog to a DialogRoute which then can be helped to show the same dialog.

Anyway yes we can hack something at higher level to mimic that, but those concept of inverted dependency and navigation pattern are IMO normal and basic use case.

You should not hack anything. If you have a big app then consider following the reactive concept of stateful navigation, it has so many benefits.

Tolriq commented 3 years ago

We do mix a few things effectively.

For

You showed the code in above comment similar to this val currentDestination = controller.getAllHistory().lastOrNull(). Now question is do we know what route is currentDestination variable represent?

Typically this is taken from your samples and onChanged callback. Since the onChanged callback is passed and called from a route we can assume that it's the current top route from the current controller so it works.

Now about the main reusable screens for example. My app can detect "servers" and add them from different places with some configuration steps between then go to a final different place or just close and return to where we where.. Those can happens from the first run wizard, from settings, and from some bottom sheets too.

Same for dialogs that can navigate to other destinations, you can have a detail dialog that can navigate to details screens and we should be able to return to the previous place we where. But that dialog can be called for example from an album list or an artist list.

I do not see reusing all those cases an anti pattern by it self, just more complex navigation than what jetpack compose navigation offers and reason I want to use something less restrictive.

KaustubhPatange commented 3 years ago

I do not see reusing all those cases an anti pattern by it self

This is not reusing the screen but reusing the route which is fine & that's what we should do.

Same for dialogs that can navigate to other destinations, you can have a detail dialog that can navigate to details screens and we should be able to return to the previous place we where. But that dialog can be called for example from an album list or an artist list.

You can achieve it, all you need to do is to find the controller using findController that was used to create the dialog. The call is recursive to its parent. So it will exist as long as the destination is in the backstack.

just more complex navigation than what jetpack compose navigation offers and reason I want to use something less restrictive.

This is not a restriction but providing a correct way for navigation. If this feature is added then there are two ways of doing one thing which is not something we want, we've seen this with Fragments eg: addToBackStack.

Even though it could be a valid feature request but I may not add this feature as this will break the predictable navigation. Having something to replicate doNotAddToBackStack may not be good as popUpTo solves this. Still, I'll keep this issue open for me to think whether this is something needed or not.

Edit: I just realized I can't replicate this feature using something like doNotAddToBackStack which will not add the current route to the backstack when navigating. Why? Because this navigation framework is stateful. Any changes made to the backstack is automatically tracked and are reacted by the library for eg: when you call navigateTo it adds the current route to an observable snapshot list which when changes recomposes the navigation Setup, thus the destination changes.

I might have to close this issue as this is something not possible to add.

Tolriq commented 3 years ago

Thanks for looking into this, the issue is the same with cleartop. Popupto requires that the caller knows the backstack to pop to the first one.

While I understand why you want to force unique routes and the benefits. This forces to define a lot of duplicate routes to achieve all the cases if we want abstraction and to pass those routes around the destinations.

KaustubhPatange commented 3 years ago

I'm thinking & something got into my mind. I'll finalize the API design & then we can discuss if we need this.

Tolriq commented 3 years ago

If that can help https://voyager.adriel.cafe/stack-api from another library I checked at some point.

I really loved the idea of fully controllable back stacks permitting all my use cases easily.

KaustubhPatange commented 3 years ago

Yes, I've something similar in my mind & I'll get back to you with it.

KaustubhPatange commented 3 years ago

I opened a pull request (#17) related to this issue. See if it meets your needs?

Tolriq commented 3 years ago

Got a quite busy week end will probably only be able to check all those early next. But thanks for the fast iterations.

KaustubhPatange commented 3 years ago

Hey, I have published a snapshot (0.1-alpha24-457343a-SNAPSHOT) for the latest commit in update branch which adds this feature request.

I have added all the required utilities which I think are more than enough to gain more control over the backstack. See if these changes meet your needs & try to give early feedback before merging this to master.

Tolriq commented 3 years ago

Hi,

I'm really but son is sick and tons of real life things adds up, I have quickly tested previously and it seems OK, but I won't have time to test all my use cases before some time :( Since you are still in 0.1 alpha I suppose it will still be possible to have breaking changes later.

KaustubhPatange commented 3 years ago

Hey @Tolriq,

No hurry, health & family always comes first! I'll test it more & will try to find more usecase as required & will possibly merge it.

Till then, take care of you & family :)

Tolriq commented 3 years ago

Thanks. My main untested navigation pattern that should be abstracted would be like a bottom nav.

From tab 1 of the bottom nav you like click an artist it start a sub navigation to it that can then go to albums of that artist. Then you click tab 2 and there at some point you can start the exact same sub navigation.

Should work +properly handle backstack some from the last point get back in the 2 page of the bottom nav then to full stack of page 1 of bottom nav then up again.

(Not sure it's very clear but you get the idea).

KaustubhPatange commented 3 years ago

I got the idea, I think this should work if nested navigation is properly tied to parent navigation (which is done automatically) if it doesn't then report to me immediately. Also, kindly prepare a sample if such an issue exists it would save my work of replicating it :) In the meantime, if I get some free time then I'll try myself.

KaustubhPatange commented 3 years ago

Hey again,

I also added the docs for these new APIs, check here.

Tolriq commented 3 years ago

So starting to look into that reusable nested navigation in compose and I'm lost about how to do that properly currently :(

Using a scaffold and bottom navigation first level navigation is easy:

composeNavigator.Setup(key = MainRoute.key, initial = MainRoute.Home(), controller = homeController) { destination ->
                        when (destination) {
....

Now if page 1 wants to implements nested navigation specific to that page do the same and all works.

But when you want to have reusable and cross linked navigation routes, this becomes a lot more complicated.

When in page 1 I want to start a nested navigation to a detail screen 2 I can create a new route and do nested nav with a navigator.setup ...

When in page 2 I want to start a nested navigation to a detail screen 2 (identical to the other one). I need to create a new route and another navigator.setup and so on.

When you add all the possible cases everywhere this does not seems like the proper way, so I'm probably missing something.

Since the controller and destination are all tied to a route I don't see a way to properly describe such kind of navigation

KaustubhPatange commented 3 years ago

When you add all the possible cases everywhere this does not seems like the proper way, so I'm probably missing something.

It is the proper way in my opinion. I'll tell you why! Even though you've duplicated routes (let's say D) to create nested navigation identical (to let's say B). So in a way, your backstack looks something like this [A = {1,2} , B = {1,2,3} , C = {1,2...} , D = {1,2,3}].

Now here we know B & D are identical in nested navigation. Let's say if you reuse any existing routes, the backstack key is not unique anymore. So suppose if you want to do popUntil using goBackUntil(...) it is not possible. The back navigation may not produce correct results since there is the same key in the backstack (although it would not be an issues because ComposeNavigator can handle duplicate keys). The main problem will be using popUpTo & other utilities that are used to manipulate the backstack.

I would suggest first try using an existing key & see if everything is working fine according to you. In any term longer if something fails you can always quickly refactor the routes (since you know IntelliJ Idea has great refactoring for classes).

Also, one thing to mention is if you reuse the routes (eg B for D), using findController<B>(B::class) will only retrieve the first instance of B (descending order i.e D) which means you cannot get a controller for the first B route (i.e after A) because of CompositionLocalProvider scoping.

Tolriq commented 3 years ago

But even so how do you configure those to display the proper screens.

There's infinite possible combination in my case as you can stack indefinitely, like album details -> artist list -> genre -> albums or artist -> .. and that from multiple source routes. Like an overview tab a genre tab or other kind of tabs or bottom nav start

Do you have some sample of how you'd do that?

KaustubhPatange commented 3 years ago

So suppose A,B,C are the screen in bottom navigation & A contains a nested navigation of D. Now I'm currently deeply nested in B. Suppose I have a requirement to add D as nested navigation (duplicate). What I would do is find controller of bottom navigation & I'll navigate to A with an arg that if read will navigate to D immediately & thus I'll prevent duplicate nested navigation.

So in infinite possibility cases, I think this would scale but you need to do some additional configuration to correctly handle backstack.

These will change according to your app case. Since I don't know any of your app code & can't suggest any optimal way. But these would things that I would do. Also, I would first suggest that you just think (in your mind) on how would you implement this if it were a case with fragments. If you found a solution I guess doing it with compose is not that hard?

Tolriq commented 3 years ago

But the thing is that I do not want to navigate to A, having "duplicate" nested navigation is wanted.

My English skills are lacking but with fragments I can add any fragment to the backstack and remove it and pop to where I want. (Not talking about official jetpack navigation).

About what I'd like to achieve: MainRoute.A: First tab of a bottom nav MainRoute.B: Second tab of a bottom nav ....

I can navigate down in both those tabs to other things while each have their back stack that is kept when I navigate from one tab to the other. Pressing back when deep in MainRoute.B goes up in the nested stack then when at top back goes to deep of A then up then leave the app. So by default I would have FirstRoute.A then B.... and a SecondRoute.A then B ...

Now in each of those 2 nested graph at some point I can show a button / list item / .... that navigate to a detail screen so let's say an AlbumRoute.A. that in that route could display an artist detailscreen.

Since the controller.setup is tied to the route and the received destination I can't just mix things up to add the AlbumRoute.A to the backstack of the FirstRoute.A.

If I understand correctly your solution is that I create a FirstRoute.Albums that is Albums and FirstRoute.Artist that is artist and SecondRoute.Albums SecondRoute.Artists that will duplicate code then have some kind of abstraction on the navigation that will make those components not aware of their route but will translate something to the proper routes, with all the issues about ensure proper typing. This sounds more like the jetpack navigation with strings params and conversion and if I'm going that road then why not in the end use the official library.

KaustubhPatange commented 3 years ago

You can pop to any point in the nested navigation. ComposeNavigator has goBackUntil utility that will pop recursively including nested navigation.

My English skills are lacking but with fragments I can add any fragment to the backstack and remove it and pop to where I want.

By default every screen is added to backstack since navigation is based on Jetpack Compose's snapshot list. You can remove it using controller goBack or use goBackUntil to recursively pop wherever you want.

then have some kind of abstraction on the navigation that will make those components not aware of their route

You can use a sealed interface & make a common interface for Album & Artist. This way you use it many times (a when on the interface will give you a proper route) without having to do any kind of abstraction on routes.

Tolriq commented 3 years ago

At the root at the bottom navigation we would have something like:

composeNavigator.Setup(key = MainRoute.key, initial = MainRoute.Home(), controller = homeController) { destination ->
                        when (destination) {
                            is MainRoute.Home -> HomeDestination()
                            is MainRoute.Library -> LibraryDestination()
                            is MainRoute.Search -> SearchDestination(l)
                        }
                    }

In each of those screen I'd have the nested navigation with

composeNavigator.Setup(key = XRoute.key, initial = XRoute.Home(), controller = homeController) { destination ->
                        when (destination) {
                            is XRoute.Home ->
                            is XRoute.Library -> 
                            is XRoute.Search ->
                        }
                    }

I can't just have a ZRoute.artist here to reuse the route on both sub screens.

I already have a sealed interface for a start route with one route leading to the bottom navigation screen. And one sealed interface for all the possible tab destination. The artist / albums / xxx part needs to be displayed at the when XRoute place.

So please show me how you'd configure the navigator and the "when": A: tab 1 with list of artists from bottom nav B: tab 2 with list of albums from bottom nav C: list of albums from an artist D: list of artists from an album

Start on screen A, press an artist, have a list of albums press an album have a list of artist Press B tab so press an album then an artist then an album ...

Back stack is A -> C -> D -> C -> D -> C -> B -> D -> C -> D -> C

To better visualise you can imagine that both tabs A and B are active at the same time on a tablet too.

If I press back I go back in the proper order I can press B or A to go directly to each root. All that with C D order completely reusable as there's more than 2 tabs and more than just those 2 destinations.

No need for a complete real example but more than just do something when that something does not seem possible with the current API

KaustubhPatange commented 3 years ago

Okay I got it what you said! Let's take your example on how it will be represented in the backstackmap (which is root history contains all the sub backstack).

[ A = {C,D,C,D,C} , B = {D,C,D,C} ], where A & B are sealed routes & C,D are the sub routes of A & B.

sealed class A {  
   data class C(...) : A()
   data class D(...) : A()
}
sealed class B {
   data class C(...) : B()
   data class D(...) : B()
}

If you see we are just duplicated C & D for both the routes because now they are both identifiable as A.C or B.D. Both C, D will have same parameters (you can copy paste or use sealed interfaces).

A = {C,D,C,D,C} is possible since backstack is arraylist so you can have same routes in the list. What you cannot have is [ A = {...} , A = {...} ] (which happens in nested navigation if you reuse A in a nested navigation). These will ultimately become [ A = {...} ] as you might know the root backstack of navigator is map which does not allow duplicate route.key.

If you need those C,D,C,D it is possible within Controller's backstack (arraylist) but you have to duplicate or use sealed interface for that route. Because what you are suggesting may not be even possible with the official navigation compose, simple stack or voyager because they all need unique As.

I hope you are getting it.

Tolriq commented 3 years ago

We are duplicating the C and D and the when because A.C won't be accessible from the when of the B route.

This also means that the screen in B needs to know it's in B be to navigate to B.C and not A.C.

Now when you add E F G H for each screen + 6 source sealed class you see the issue. I think simple stack and voyager do work in that case as there's no forced link between the navigation root and the child they just need to be Screen not Route than extend the parent one.

The issue here is that the navigation stack root define the possible destinations in non flexible Type.

KaustubhPatange commented 3 years ago

Simple stack does not support nested navigation for compose so its out of question. Taking on voyager its not possible without letting your root route know about its child route & the case you explained is as difficult as you implementing with navigator.

the screen in B needs to know it's in B be to navigate to B.C and not A.C

Ok I see this issue, so let me ask you this how will you propose to solve this issue from your app & what you expect from the library to offer as well.

Tolriq commented 3 years ago

Voyager have no problem at all to handle that. A destination is a screen.

Just start a new navigator for a nested one then push screen details. Exactly as I do with fragment.

There's 0 complexity from their doc and samples. A screen just needs to extends Screen.

I like your approach of decoupling the content from the route giving more controls and everything at that when level. But the hard link in class hierarchy between the controller key and it's possible backstack entries annihilate that power.

KaustubhPatange commented 3 years ago

But the hard link in class hierarchy between the controller key and it's possible backstack entries annihilate that power.

Can you elaborate more on this. Do you mean that somehow I should support duplicate nested navigation or something else which could solve this issue?

Edit: From my side I'm also thinking how to effectively solve this issue. If you find any solution/idea let me know and I'll try to implement it.

Tolriq commented 3 years ago

I know my English is bad I warned a few times :) And we probably also have some terminology issues here.

The "issue" is about typing and everything being from the key to manage the backstack root to the controllers to the content callback from Setup().

While there is one advantage of this strong typing in all the hierarchy is that you can easily use sealed classes to force exhaustive when and everything. But on the other end since you do use that type as the key for the root and nested navigation roots you are generating a hard immutable link between the navigation scopes and the routes / screens making things very limited.

I am absolutely not sure what is the best way to fix this in your current code internal as I have not really fully looked into and all your requirement of having the key a KClass of a Route. And there's also your terminology of calling things Route. But in your examples / usage those Routes are just collections of Screens without any order or relation between them except being from the same "Route" so the term may induce confusion and is probably also why your library is made like it is, you are not supposed to go of road.

IMO if possible the Setup key should not be tied to the T type so we can reuse "routes" more easily (or in the worst case have 1 big sealed class that define all screens).

Depending on your internals (and I thought it was more or less like that at first) I would have a T: Route to define the "Route" and your nested nav. And a S : Screen for all the possible destinations that you can push / pop with proper typing there's maybe even still ways to provide sealed interface for the Screens for some specific Setup().

But this is a large complex architecture issue not sure this is something you want to go after.

KaustubhPatange commented 3 years ago

I think there should be better terminology. I'll update the docs with it but consider this.

sealed class FirstRoute : Route {
   data class First(...) : FirstRoute()
   data class Second(...) : FirstRoute()
   companion object { val key = FirstRoute::class }
}

Here FirstRoute is the Route & FirstRoute.key (::class) is the key for the route. Then FirstRoute.First() and FirstRoute.Second() are subroutes of the FirstRoute with key FirstRoute.First::class & FirstRoute.Second::class respectively.

the Setup key should not be tied to the T type so we can reuse "routes" more easily (or in the worst case have 1 big sealed class that defines all screens).

This would break everything that I built. My first initial goal was to provide typesafe navigation with typesafe arguments since they are very much needed to ensure correct navigation & to also prevent accidental or incorrect navigation to screen which does not exist or is not a child of.

I guess we are too early to predict how navigation should look like in compose. My initial implementation was inspired by compose-router which does the same but the root key is string here & not the KClass<T>.

Doing so what you've said will definitely break the architecture completely which means redesign everything from the ground up & something I cannot do.


Anyway, I think I have an answer to your question! So for suppose as you said when we are in C route it needs to know that it is in B or A to go into B.C or A.C.

What would you do which I think is beneficial is to create something like a class (maybe data class) that defines all the lambdas for navigation through that nested navigation.

data class NavigationBottom(val goToC : () -> Unit, val goToD : ()) // or any name like goToDetail or goToList etc.

We can now supply it using a composition local provider using staticCompositionLocalOf & during Setup, we define these methods.

val LocalNavigationBottom = staticCompositionLocalOf<NavigationBottom> { throw Exception("Oops") }

@Composable
fun Main() {
   navigator.Setup(...) {
       val navigationBottom = remember {  NavigationBottom(goToC = {...}, goToD = {...}) }
       CompositionLocalProvider(LocalNavigationBottom provides navigationBottom) {
           when(...) {
              B -> ScreenB()
              ...
           }
       }
   }
}

fun ScreenB() {
   val navigation = LocalNavigationBottom.current
   // Use a button or anything and call navigation.goToC() 
   // & use the same to go to D from screen C
}

Reusing

Since composition local provider exists in only the provided scope you can then provide other LocalNavigationBottom to the inner subroutes or nested navigation (if you want to reuse it).

Is this approach good? Since as we know in Compose data flows down & events go up, which effectively means nested navigation could & should know parent navigation (not completely but at least an abstraction) to properly communicate with it (vice-versa is not be possible since CompositionLocalProvider takes care of that).

What about having too many cases to handle?

I think the idea could be abstracted out which means each screen has its own LocalBottomNavigation or other staticCompositionLocalOf<T> which you will define. Since a child must know where it can navigate further it is necessary not to hide this abstraction otherwise the architecture would become very complex & then you have to use solutions like voyager where you would lose typesafe navigation & maybe some other important details.

Tolriq commented 3 years ago

There's a lot to answer but again I've perfectly understood the reason of most of the decisions and respect them as perfectly valid and making sense.

But even in compose-router the key is a string not the type of the Routing, meaning that a Routing can be reused anywhere just need to start each route / nested route with a proper different key.

So for my need I could have a MusicRouting that contains all the possible screens related to MusicRouting and reuse those in any tabs or place by just varying the key and the code would be a lot more reusable. While still having type safe routes and type safe args. I would still have to hack something for the root of the tabs that may or may not be part of that route but that's very little compared to all the duplication required currently to handle the current key = the route.

Edit: Anyway I understand that it's currently not possible to separate them, so I'll try to see if I can abstract something over voyager to start with that would still allows me to get back to this library easily if this change in the future. Thanks for all the conversations about navigations, opened a lot of interesting thinking on my side.

KaustubhPatange commented 3 years ago

But even in compose-router the key is a string not the type of the Routing, meaning that a Routing can be reused anywhere just need to start each route / nested route with a proper different key.

You cannot reuse the Route key in compose-router since it is stored in a map so duplicate keys will be pruned automatically.

What I can do is hack something & could maybe possibly implement duplicate nested navigation. Something I'll have to experiment with & see how it goes!

Anyway, thanks for all the feature requests and shaping the future of the library it also opened a lot of ideas from my side on effective navigation.

Edit: I got what you said. The navigation must not be tied to the Route.key. Yeah, maybe this should change for to enable proper nested navigation. I'll experiment & will let you know!

Tolriq commented 3 years ago

You cannot reuse the Route key in compose-router since it is stored in a map so duplicate keys will be pruned automatically.

That's where we have a communication issue :) I do not want to reuse the key, it's normal that it's different for different backstack and for nesting / deep nesting.

But I can have key = "TabA" with a MusicRouting : Routing and a key = "TabB" with a MusicRouting : Routing making the routes reusable on different backstacks.

Your routes types are the keys to the backstack so a MusicRoute can not be reused in 2 backstacks that's the main pain point here that remove any possible flexibility needed when doing complex navigation with nested graphs.

If you can remove that and have backstack root / nesting keys unrelated to routes we can have a lot more flexibility. You still have a controller tied to sealed interface to have safe navigation and everything but at any time we can start a nested navigation to an new route that may have already been used, this is no more an issue.

KaustubhPatange commented 3 years ago

See my Edit in the previous comment is this what you are talking about. If yes, I totally get it. I'll find a way to remove this limitation & effectively improve it. Thanks for letting me know about this.

I guess you are right even I'm not a native English speaker so yeah could be the case of a communication issue.

Tolriq commented 3 years ago

Not native speaker either ;) But yes I think we are now on the same page, the issue is about the backstack keys to be able to dissociate them from the actual routes.

KaustubhPatange commented 3 years ago

We might lose type-safety during this process but I'll definitely find a way to overcome it.

KaustubhPatange commented 3 years ago

Your proposal is merged in the library. Check the docs & samples as they are updated with the fix.

Tolriq commented 3 years ago

Thanks a lot will look into that next week.

KaustubhPatange commented 3 years ago

I'll close this issue for now. Create another one if you get any problems with this implementation.