Reuse PermanentNavModel if provided in the list of navModels

[KovalevAndrey]

Description

Fixes: #604

Problem: If PermanentNavModel was provided to the constructor of the ParentNode it will be ignored as ParenNode internally uses its own instance of PermanentNavModel. This PR ensures that we reuse the same instance of PermanentNavModel if it's provided

Check list

• I have updated CHANGELOG.md if required.
• I have updated documentation if required.

[LachlanMcKee]

It seems like it's possible (though maybe not advised?) that a developer could nest a CombinedNavModel within a CombinedNavModel.

Perhaps this case should be covered as well? (if so, then the init block for the CombinedNavModel might need to be updated).

If we don't want to allow nesting CombinedNavModel within CombinedNavModel perhaps the init block of CombinedNavModel could prevent this as well? (though this hypothetically could be a breaking change.

i.e.

CombinedNavModel(
    listOf(
         CombinedNavModel(
             listOf(
                 PermanentNavModel()
             )
         )
    )
)

[KovalevAndrey]

yeah, I think we can prevent nesting of CombinedNavModel as it doesn't make much sense and harder to support

[CherryPerry]

Nesting makes sense at the current moment.
First you pass CombinedNavModel via ParentNode(navModel = backStack1 + backStack2) then this CombinedNavModel is merged with internal PermanentNavModel into another CombinedNavModel. So at the end we have CombinedNavModel inside CombinedNavModel is a pretty basic case.

[LachlanMcKee]

@KovalevAndrey it sounds like this might be wise to keep? otherwise you should definitely update the changelog to call this out as a breaking change.

[KovalevAndrey]

@LachlanMcKee Looking at the code I'm leaning towards removing the PermanentNavModel from parentNode at all and use it the same way as any other navModels

[KovalevAndrey]

Nesting makes sense at the current moment. First you pass CombinedNavModel via ParentNode(navModel = backStack1 + backStack2) then this CombinedNavModel is merged with internal PermanentNavModel into another CombinedNavModel. So at the end we have CombinedNavModel inside CombinedNavModel is a pretty basic case.

If we're using the provided API it doesn't get nested

[LachlanMcKee]

Does this need to be nullable? perhaps it could be lateinit as well?

[KovalevAndrey]

primitives can't be used with lateinit

[CherryPerry]

Let's drop lateinit instead?

val navModel: NavModel<NavTarget, *>
private val permanentNavModel: PermanentNavModel<NavTarget>
private val isPermanentNavModelProvided: Boolean

init {
   val (combinedNavModel, permanentNavModel, isProvided) = resolveNavModels(navModel, buildContext)
   this.navModel = combinedNavModel
   this.permanentNavModel = permanentNavModel
   isPermanentNavModelProvided = isProvided
}

[CherryPerry]

In general OK, but IMHO we are fixing the wrong thing. The library is Model-driven navigation, but @Composable fun PermanentChild(key) does not fit really into this category, making "navigation" UI-driven. I think we just should provide more convenient BagNavModel (that we already use in tests) where we can freely use add and remove operations and have a Composable function to render its children one by one (by key as it is right now). We should not have such weird shortcuts for child hosting as we have right now and then write a lot of code around handling them.

[CherryPerry]

Let's drop lateinit instead?

val navModel: NavModel<NavTarget, *>
private val permanentNavModel: PermanentNavModel<NavTarget>
private val isPermanentNavModelProvided: Boolean

init {
   val (combinedNavModel, permanentNavModel, isProvided) = resolveNavModels(navModel, buildContext)
   this.navModel = combinedNavModel
   this.permanentNavModel = permanentNavModel
   isPermanentNavModelProvided = isProvided
}