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

Persistence in SwiftUI #120

Open
Tyler-Keith-Thompson opened this issue Sep 1, 2021 · 14 comments
Open

Persistence in SwiftUI #120

Tyler-Keith-Thompson opened this issue Sep 1, 2021 · 14 comments
Labels
enhancement New feature or request

Comments

@Tyler-Keith-Thompson
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

In UIKit flowPersistence is a deliberately supported feature. in SwiftUI right now removedAfterProceeding is supported (partially by accident) and persistWhenSkipped really isn't. I'd like to see support for those, if possible.

Describe the solution you'd like

For default presentation:

removedAfterProceeding means the @State variable holding onto the view is cleared, this is mostly useful for animations.
persistWhenSkipped I do not believe has any real meaning here, if it's skipped with our default presentation mode then there's really nothing to persist.

For navigation links:

removedAfterProceeding in a perfect world this (somehow) animates in the next view then retroactively changes the back-stack to remove the item you just proceeded from. However I think that's impossible because of how animations for Navigation Views and back stack management work in SwiftUI. So I'm fine with the new view merely replacing the one that gets removed after proceeding.
persistWhenSkipped means that a skipped item is in the backstack. You can navigate backward to it, but when you navigate forward from the previous item it's not animated in. (Again, realizing that animations may be very hard to control here)

For modals:

removedAfterProceeding in a perfect world this (somehow) animates in the next view then retroactively changes the back-stack to remove the item you just proceeded from. However I think that's impossible because of back stack management work in SwiftUI. So I'm fine with the new view merely replacing the one that gets removed after proceeding.
persistWhenSkipped means that a skipped item is in the backstack. You can dismiss backward to it, but when you navigate forward from the previous item it's not animated in. (Again, realizing that animations may be very hard to control here)

Describe alternatives you've considered

Expressly have an error (ideally compile time) that stops users from being able to apply persistences that do not work.

@Tyler-Keith-Thompson Tyler-Keith-Thompson added the enhancement New feature or request label Sep 1, 2021
@Tyler-Keith-Thompson Tyler-Keith-Thompson added this to the SwiftUI Support milestone Sep 1, 2021
@morganzellers
Copy link
Contributor

Began Working on persistWhenSkipped in navigation links. Findings below:

  • I started at first by changing launch() and proceed() in WorkflowViewModel:

From this:

extension WorkflowViewModel: OrchestrationResponder {
    func launch(to destination: AnyWorkflow.Element) {
        body = destination
    }

    func proceed(to destination: AnyWorkflow.Element, from source: AnyWorkflow.Element) {
        body = destination
    }
}

To this:

extension WorkflowViewModel: OrchestrationResponder {
    func launch(to destination: AnyWorkflow.Element) {
        if destination.value.metadata.persistence == .persistWhenSkipped, let next = destination.next {
            body = next
        } else {
            body = destination
        }
    }

    func proceed(to destination: AnyWorkflow.Element, from source: AnyWorkflow.Element) {
        if destination.value.metadata.persistence == .persistWhenSkipped, let next = destination.next {
            body = next
        } else {
            body = destination
        }
    }
}

I reverted this change after realizing that Workflow handles this already via a double proceed() call in setupProceedInWorkflowStorage()

Lines 190 & 210

In addition, the Workflow class has easy access to shouldLoad, which keyed me into the fact that the under-the hood proceed logic is already taken care of, and my focus needs to be more "View specific".

@morganzellers
Copy link
Contributor

morganzellers commented Sep 8, 2021

This realization made me go look at proceedInWorkflow() in WorkflowItem:

private func proceedInWorkflow(element: AnyWorkflow.Element?) {
    if let body = element?.extractErasedView() as? Content, elementRef === element || elementRef == nil {
        elementRef = element
        content = body
        persistence = element?.value.metadata.persistence ?? .default
    } else if persistence == .removedAfterProceeding {
        content = nil
        elementRef = nil
    }
}

After some debugging of this example scenario...:

var body: some View {
    WorkflowLauncher(isLaunched: .constant(true)) {
        thenProceed(with: FlowRep1.self) {
            thenProceed(with: SkippedFlowRep.self) {
                thenProceed(with: FlowRep2.self).presentationType(.navigationLink)
            }
            .persistence(.persistWhenSkipped)
            .presentationType(.navigationLink)
        }
        .presentationType(.navigationLink)
        
    }.embedInNavigationView()
}
struct FlowRep1: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        Button("FlowRep1 proceed") { proceedInWorkflow() }
    }
}

struct SkippedFlowRep: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        VStack {
            Button("SkippedFlowRep proceed") { proceedInWorkflow() }
            Button("SkippedFlowRep backup") { try? backUpInWorkflow() }
        }
    }
    func shouldLoad() -> Bool { false }
}

struct FlowRep2: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        Button("FlowRep2 backup") { try? backUpInWorkflow() }
    }
}

...I realized that the proceedInWorkflow() function in WorkflowItem was not handling the scenario for a persistence of .persistWhenSkipped.

I updated the function as such:

private func proceedInWorkflow(element: AnyWorkflow.Element?) {
    persistence = element?.value.metadata.persistence ?? .default

    if let body = element?.extractErasedView() as? Content, elementRef === element || elementRef == nil {
        elementRef = element
        content = body
    } else if persistence == .removedAfterProceeding {
        content = nil
        elementRef = nil
    } else if persistence == .persistWhenSkipped {
        elementRef = element?.next
        content = element?.extractErasedView() as? Content
    }
}

This got me to a scenario where I was seeing:

FlowRep1 -> SKIP -> FlowRep2 --- Successfully skipping the SkippedFlowRep view when proceeding from FlowRep1.
AND
SkippedFlowRep <- FlowRep2 --- Successfully backing up from FlowRep2 to SkippedFlowRep using backupInWorkflow() and NavView Back button.

Unfortunately, neither proceeding nor backing up from SkippedFlowRep is working - it just stays there.

@morganzellers
Copy link
Contributor

For that, I had a suspicion that something along the lines of this block will be needed in the proceedInWorkflow() function but not sure where:

if content == nil { 
    _ = element?.traverse(direction: .backward) { 
        if $0.value.metadata.persistence == .persistWhenSkipped,
           let body = $0.extractErasedView() as? Content {
            elementRef = $0
            content = body
            isActive = true
            persistence = .persistWhenSkipped
        }
        return true
    }
}

Adding this block into the proceedInWorkflow() function gave SkippedFlowRep the ability to proceed to FlowRep2 after FlowRep2 backed up to SkippedFlowRep:

private func proceedInWorkflow(element: AnyWorkflow.Element?) {
    persistence = element?.value.metadata.persistence ?? .default

    if let body = element?.extractErasedView() as? Content, elementRef === element || elementRef == nil {
        elementRef = element
        content = body
    } else if persistence == .removedAfterProceeding {
        content = nil
        elementRef = nil
    } else if persistence == .persistWhenSkipped {
        elementRef = element?.next
        content = element?.extractErasedView() as? Content
    }

    if content == nil {
        _ = element?.traverse(direction: .backward) {
            if $0.value.metadata.persistence == .persistWhenSkipped,
               let body = $0.extractErasedView() as? Content {
                elementRef = $0
                content = body
                isActive = true
                persistence = .persistWhenSkipped
            }
            return true
        }
    }
}

@morganzellers
Copy link
Contributor

This AM I verified this behavior in Xcode 13 & 12.5.1

@morganzellers
Copy link
Contributor

Moved to testing out this example app configuration:

WorkflowLauncher(isLaunched: .constant(true)) {
  thenProceed(with: FlowRep1.self) {
      thenProceed(with: SkippedFlowRep.self) {
          thenProceed(with: FlowRep2.self) {
              thenProceed(with: SkippedFlowRep.self) {
                  thenProceed(with: FlowRep1.self).presentationType(.navigationLink)
              }
              .persistence(.persistWhenSkipped)
              .presentationType(.navigationLink)
          }
          .presentationType(.navigationLink)
      }
      .persistence(.persistWhenSkipped)
      .presentationType(.navigationLink)
  }
  .presentationType(.navigationLink)
}.embedInNavigationView()

struct FlowRep1: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        VStack {
            Button("FlowRep1 proceed") { proceedInWorkflow() }
            Button("FlowRep1 backup") { try? backUpInWorkflow() }
        }
    }
}

struct SkippedFlowRep: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        VStack {
            Button("SkippedFlowRep proceed") { proceedInWorkflow() }
            Button("SkippedFlowRep backup") { try? backUpInWorkflow() }
        }
    }
    func shouldLoad() -> Bool { false }
}

struct FlowRep2: View, FlowRepresentable {
    var _workflowPointer: AnyFlowRepresentable?
    var body: some View {
        VStack {
            Button("FlowRep2 proceed") { proceedInWorkflow() }
            Button("FlowRep2 backup") { try? backUpInWorkflow() }
        }
    }
}

Scenario:

FlowRep1 -> SKIP (SkippedFlowRep) -> FlowRep2 -> SKIP (SkippedFlowRep) -> FlowRep1

Proceeding through this scenario works, however backing up from the second SkippedFlowRep incorrectly takes you to a FlowRep1.

@morganzellers
Copy link
Contributor

This

Proceeding through this scenario works, however backing up from the second SkippedFlowRep incorrectly takes you to a FlowRep1.

Is making me think that second SkippedFlowRep, that I'm trying to backup from is actually the instance of the first SkippedFlowRep because if we look at the scenario:

FlowRep1 -> SKIP (SkippedFlowRep1) -> FlowRep2 -> SKIP (SkippedFlowRep2) -> FlowRep1

SkippedFlowRep1 should be:

SkippedFlowRep1.next = FlowRep2
SkippedFlowRep1.previous = FlowRep1

SkippedFlowRep2 should be:

SkippedFlowRep2.next = FlowRep1
SkippedFlowRep2.previous = FlowRep2

So if going back from SkippedFlowRep2 takes me to FlowRep1, then that makes me think I need to do some more checks in my proceedInWorkflow() function to make sure I'm setting the correct instance of SkippedFlowRep.

@morganzellers
Copy link
Contributor

After learning that Content has an underlying type, all of the checks on if let body = element?.extractErasedView() as? Content make much more sense.

After some debugging, I have come to the point that this if-check from proceedInWorkflow() wasn't actually doing what I thought:

else if persistence == .persistWhenSkipped {
    elementRef = element?.next
    content = nextView as? Content
}

So I've updated the function to look like this:

private func proceedInWorkflow(element: AnyWorkflow.Element?) {
    persistence = element?.value.metadata.persistence ?? .default

    if let body = element?.extractErasedView() as? Content, elementRef === element || elementRef == nil {
        elementRef = element
        content = body
    } else if persistence == .removedAfterProceeding {
        content = nil
        elementRef = nil
    }

    if content == nil {
        _ = element?.traverse(direction: .backward) {
            if $0.value.metadata.persistence == .persistWhenSkipped,
               let body = $0.extractErasedView() as? Content {
                elementRef = $0
                content = body
                isActive = true
                persistence = .persistWhenSkipped
            }
            return true
        }
    }
}

Because I believe the if content == nil check is actually doing what I thought the previous one was.

With this change, I'm seeing the navigation stack function the way we expect, but not the transitions. On a proceed from FlowRep1, I'm seeing this happen:

FlowRep1 -> SkippedFlowRep -> FlowRep2 -> BACK (SkippedFlowRep)

@morganzellers
Copy link
Contributor

After some debugging and chipping away at the changes made prior, we got back to the state of proceedInWorkflow() as it is on main:

private func proceedInWorkflow(element: AnyWorkflow.Element?) {
    if let body = element?.extractErasedView() as? Content, elementRef === element || elementRef == nil {
        elementRef = element
        content = body
        persistence = element?.value.metadata.persistence ?? .default
    } else if persistence == .removedAfterProceeding {
        content = nil
        elementRef = nil
    }
}

This, with the addition of a dispatch for proceed calls to give them more time to finish:

func proceed(to destination: AnyWorkflow.Element, from source: AnyWorkflow.Element) {
    DispatchQueue.main.asyncAfter(deadline: .now().advanced(by: .seconds(1))) {
        self.body = destination
    }
}

Got us to a place where we're seeing the navigation stack function correctly AND the flow proceed forward correctly.

The big thing to note here is that updating model.body in rapid succession does not allow both updates to fully percolate through the system. The second update ends up replacing the first. The dispatch adds time between updates for the changes to percolate out to all.

@Richard-Gist
Copy link
Collaborator

We continued to investigate and CurrentValueSubject behaves similarly to what we are seeing with model.body being updated, AND PassthroughSubject is not going out to all views.

@Richard-Gist
Copy link
Collaborator

Richard-Gist commented Sep 15, 2021

I created another spike branch called kafka-like that does some more things around bunching events.

My thought is that because PassthroughSubject does not go out to all subscribers, and CurrentValueSubject does not send out all events, that I could leverage model.body changing and simply track all my events myself. That is currently in place and works, SORT OF. The behavior when persisting a skipped screen in the middle of a flow is broken. For example, FR1 -> FR2 (skipped) -> FR3 ends up rendering as FR1 on screen, then briefly FR2, then FR3 briefly, then FR2 again. BUT SUCCESS when you have FR1 as skipped, then the app will properly launch with FR2 in place and FR1 in the stack.

So if FR1 works as skipped I then tested out FR2 as well, so the example being: FR1(skip) -> FR2(skip) -> FR3. And when I launched the app, everything was correct and I was looking at FR3 and could navigate backward all the way to FR1. But then 💥 Because if you slow animations in the simulator, you'll see FR1 sliding off the screen, FR2 sliding in, then it behaves the same as if FR2 is skipped in the middle (see above).

This leads me to believe that things are only working correctly because everything is set up correctly before the first view is rendered. Once a view has rendered, we seem to be hitting race time issues with navigation links being flipped and views being re-rendered and everything going down the toilet.

As part of this avoidance of re-rendering, I've tried to capture the state that changes in a StateObject to allow the state to persist WITHOUT also causing another rendering pass, but that hasn't worked so far.

And so the investigation continues.

@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Sep 15, 2021

I created a branch called replay-subject @morganzellers You were right to use ReplaySubject not PassthroughSubject when we were working together. @Richard-Gist says my branch is better than his, I agree with him.

So the short version:

  • If you skip the first screen it works great
  • If you skip the first n+ screens it does work...but when you back up and go forward it does things that are difficult to predict
  • If you skip the middle screen it just doesn't work, it goes to the right screen, but then like automatically backs up
  • If you skip at the end it works

The ReplaySubject is influenced, but not copied verbatim, by what we read on Medium but changed to suit our specific use-cases. It doesn't have a buffer it just replays all values.

@Richard-Gist
Copy link
Collaborator

I think the way forward here is to figure out why do we wiggle back and forth in the animation. To do this, I'm starting to venture into recreating SwiftCurrent piece by piece with dumb logic to figure out what causes the behavior. So far all our investigations have led to a better understanding of SwiftUI and @State and @StateObject and when views are calling body and such, but no further information on why any of this is happening.

Ultimately we have to figure out why our navigation links isActive state is flipped to false even though it should be true.

@morganzellers
Copy link
Contributor

morganzellers commented Sep 17, 2021

Just capturing some thoughts on:

Ultimately we have to figure out why our navigation links isActive state is flipped to false even though it should be true.

  • Does the back button reset isActive if you entered via the navigation link?
  • Does re-rendering affect @State variables' values? What about with default values?
  • Does view swapping the destination of a navigation link break the link until the source view is rendered again?
    • Does this put isActive back to default state?

👋 Have a good weekend!

@Richard-Gist
Copy link
Collaborator

Richard-Gist commented Sep 30, 2021

This issue is being removed from our board at this time. We tried to resolve it but the efforts you see above led to incorrect behavior in NavigationLinks and Modals. We have not found a suitable solution that works for iOS 15.

The community is welcome to implement this feature, and if we have not documented enough, let us know and we will get you caught up.

The main gotchas occur when you have 4 FlowRepresentables and you skip the first 3 with .persistWhenSkipped when all views are in a NavigationLink or all views are in a Modal presentationType.

We have also decided that if we can get this to work with iOS 15, that will be enough. We do not need .persistWhenSkipped to support anything less than iOS 15.

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 a pull request may close this issue.

3 participants