Convert navigation to NavigationStack 🧭 (iOS 16+)

[spenrose]

Changes

Introduces a Route and NavigationCoordinator.

A route is an enum that contains all the possible destinations to navigate to. The navigation coordinator holds an array of Routes in the path variable. This is the navigation stack. If you want to programmatically navigate somewhere you mutate the path array. There are some helpers to push a new Route or pop all the way back to the root.

Navigation View

Before

NavigationView {
    ...
}

After

@StateObject var navigationCoordinator: NavigationCoordinator = NavigationCoordinator()

NavigationStack(path: $navigationCoordinator.path) {
    ...
}
...
.navigationDestination(for: Route.self) { route in
    route.view(navigationCordinator: navigationCoordinator, damusState: damus_state)
}

Programmatically Pushing View

Before

NavigationLink(destination: SaveKeysView(account: account), isActive: $is_done) {
    EmptyView()
}

is_done = true # to navigate

After

navigationCoordinator.push(route: Route.SaveKeys(account: account))

Tap to navigate

Before

NavigationLink(destination: SaveKeysView(account: account)) {
    ...
}

After

NavigationLink(value: Route.SaveKeys(account: account)) {
    ...
}

Related PRs

Fixes #219 Fixes #1001

[alltheseas]

Thank you @spenrose. Are you able to kindly share a screen record of before, and after your PR 🙏

[spenrose]

@alltheseas Added before and after videos to description.

[alltheseas]

@ericholguin you able to lend a PR review, as you opened the original issue 🙏

[ericholguin]

This does work however it sometimes navigates you out of the profile view, really prevalent when you're in the Universe view go to profile it will bounce you back.

[spenrose]

@ericholguin Nice find. I tried to track this down and am not really sure what is going on. The only way I can get it to stop doing that is commenting out the subscribe and unsubscribe on SearchHomeModel. Hoping this might give you an idea of what could be going on to point me in the right direction.

I set a breakpoint on this line and it isn't triggered when trying to go to profile while on universe page.

[bryanmontz]

Unfortunately the issue still remains for the other views that are selectable from the side menu. This is a top-level navigation problem, and this PR fixes only a small subset of the issues.

[jb55]

@bryanmontz any ideas to fix our horrible nav issues is appreciated. I think there a NavigationStack thing now that might fix things?

[bryanmontz]

Yeah, I think it will, but it's iOS 16+ so we might have to get creative. I'm willing to look into nav issues next.

[spenrose]

@jb55 @bryanmontz I was just taking a look at the WWDC video on NavigationStack. I will take another shot at this this weekend with NavigationStack. I will also look into the other items in the Side View and fix those as well.

[spenrose]

@jb55 @bryanmontz Gave this a shot at converting over to NavigationStack. I didn't do all the NavigationLinks yet as I wanted to get your feedback before doing them all. It seems to have resolved the side menu issues and universal tab is working fine.

For simplicity sake I bumped minimum deploy target to iOS 16+ but if this looks promising I can put it back down and if else everything. That might make the code pretty gnarly though. Do you have stats on how many people still using iOS 15? I would expect it to be pretty low with this user base.

[jb55]

thanks will take a look

[jb55]

wow this works! maybe we should pull the trigger on 16 ?

[jb55]

running into lots of issues with this... placeholder events, etc.

[joelklabo]

wow this works! maybe we should pull the trigger on 16 ?

Kill it!

[spenrose]

running into lots of issues with this... placeholder events, etc.

I haven't converted everything. I can go ahead and move everything over to NavigationStack and we can do a full test.

[BenGWeeks]

Can we update this to "draft"?

[alltheseas]

@spenrose you in damus dev telegram?

damus.io/devchat?

[spenrose]

@spenrose you in damus dev telegram?

damus.io/devchat?

@alltheseas just joined

[jb55]

this is great. I was about to merged except I noticed one issue: move NavigationCoordinator into DamusState and switch EnvironmentObjects to ObservedObject. We don't allow EnvironmentObject in our codebase.

[spenrose]

@jb55 The issue with putting this into DamusState is that there could be multiple NavigationCoordinators. You pair one NavigationCoordinator to one NavigationStack. We currently have two NavigationStacks, one in SetupView and one in ContentView. If you wanted to have NavigationStack on a modal view or something you would then make new one. What do you think?

[jb55]

On Tue, Jun 20, 2023 at 09:54:37AM -0700, Scott Penrose wrote:

@jb55 The issue with putting this into DamusState is that there could be multiple NavigationCoordinators. You pair one NavigationCoordinator to one NavigationStack. We currently have two NavigationStacks, one in SetupView and one in ContentView. If you wanted to have NavigationStack on a modal view or something you would then make new one. What do you think?

I'll take another look at this soon.

[spenrose]

@jb55 Just got it rebased. Let's get it on a test flight build so we can all bash on it for a day since its a big change.

[jb55]

Thanks! merged in 9008c609e24a837201a7daf1ec9fbb809d125b99 and tweaked in f70273365451dd3e8f7caa0350ec6221ee0c65d3

[alltheseas]

@spenrose are you available to help @jb55 troubleshoot nav hangs, and crashes?