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

Route Composer does not find a ViewController inside a tab bar when it is being pushed #33

Closed
0rtm opened this issue Aug 23, 2019 · 22 comments
Assignees
Labels
bug Something isn't working

Comments

@0rtm
Copy link

0rtm commented Aug 23, 2019

I really like your framework. It works great. However I found a small bug.

I have the following setup:

/// Selector scene
UINavigationController(Nav1):
	AccountSelectorViewController

Pushes: ->

// Home Scene
UITabBar:
	UINavigationController:
		GreenViewController
	UINavigationController:
		RedViewController

Each ViewController has NavigationContext object that is Equatable. This context is set on the TabBar too.

When tabbar is pushed, first NavigationController (Nav1) is being hidden.
When tabbar is popped, first NavigationController(Nav1) is shown again

Here is routes configuration:

struct Path {

    static var accountSelector: DestinationStep<AccountSelectorViewController, NavigationContext?> {
        return StepAssembly(
            finder: ClassFinder<AccountSelectorViewController, NavigationContext?>(),
            factory: ClassFactory<AccountSelectorViewController, NavigationContext?>())
            .using(UINavigationController.push())
            .from(NavigationControllerStep())
            .using(GeneralAction.replaceRoot())
            .from(GeneralStep.current())
            .assemble()
    }

    static var accountHome: DestinationStep <TabbarViewController, NavigationContext?> {
        return StepAssembly(
            finder: ClassWithContextFinder<TabbarViewController, NavigationContext?>(),
            factory: TabBarFactory())
            .using(UINavigationController.push())
            .from(accountSelector.expectingContainer())
            .assemble()
    }

    static var green: DestinationStep <GreenViewController, NavigationContext?> {
        return StepAssembly(
            finder: ClassWithContextFinder<GreenViewController, NavigationContext?>(),
            factory: NilFactory())
            .from(Path.accountHome)
            .assemble()
    }


    static var red: DestinationStep <RedViewController, NavigationContext?> {
        return StepAssembly(
            finder: ClassWithContextFinder<RedViewController, NavigationContext?>(),
            factory: NilFactory())
            .from(Path.accountHome)
            .assemble()
    }
    
}

The bug happens when RedViewController is selected and app tries to deeplink to RedViewController but with a different context.

Here is full console output when this happens:

[Router] Started to search for the view controller to start the navigation process from.

[Router] BaseStep<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : FinderFactory<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>>(configuration: nil, finder: RouteComposer.ClassWithContextFinder<RouteComposerBug.RedViewController, Swift.Optional<RouteComposerBug.NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator()))))> hasn't found a corresponding view controller in the stack, so it will be built using FinderFactory<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>>(configuration: nil, finder: RouteComposer.ClassWithContextFinder<RouteComposerBug.RedViewController, Swift.Optional<RouteComposerBug.NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator()))).

[Router] BaseStep<ClassWithContextFinder<TabbarViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : TabBarFactory())> hasn't found a corresponding view controller in the stack, so it will be built using TabBarFactory().

[Router] BaseStep<ClassFinder<AccountSelectorViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : ClassFactory<AccountSelectorViewController, Optional<NavigationContext>>(nibName: nil, bundle: nil, configuration: nil))> found <RouteComposerBug.AccountSelectorViewController: 0x7f94808049d0> to start the navigation process from.

[Router] Started to build the view controllers stack.

[Router] TabBarFactory() built a <RouteComposerBug.TabbarViewController: 0x7f9482847a00>.

[Router] PushAction<UINavigationController>() has applied to <RouteComposerBug.AccountSelectorViewController: 0x7f94808049d0> with <RouteComposerBug.TabbarViewController: 0x7f9482847a00>.

[Router] Composition Failed Error: ClassWithContextFinder<RedViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) hasn't found its view controller in the stack.

[Router] Unsuccessfully finished the navigation process.
@0rtm 0rtm changed the title Route Composer does not find a ViewController inside a tab bar if it being pushed Route Composer does not find a ViewController inside a tab bar when it is being pushed Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

Hi @0rtm and thank you. Ill have a look later today and will try to come with some answer asap.

@ekazaev ekazaev self-assigned this Aug 23, 2019
@ekazaev ekazaev added the bug Something isn't working label Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm I just built a case to reproduce you issue, and i could not. Push worked fine for me to find the enclosed RedViewController inside of the TabBar that is being pushed. Could it be that you have some other issue? Could it be that the TabBarFactory fails or builds the RedViewController with a delay?

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

Here is my tabbar factory code

struct TabBarFactory: Factory {

    typealias ViewController = TabbarViewController

    typealias Context = NavigationContext?

    func build(with context: NavigationContext?) throws -> TabbarViewController {

        let vc = TabbarViewController()

        let greenVC = GreenViewController(nibName: "GreenViewController", bundle: nil)
        greenVC.context = context
        greenVC.tabBarItem = UITabBarItem(tabBarSystemItem: .favorites, tag: 0)


        let redVC = RedViewController(nibName: "RedViewController", bundle: nil)
        redVC.context = context
        redVC.tabBarItem = UITabBarItem(tabBarSystemItem: .history, tag: 1)

        var navvedVcs = [UIViewController]()

        for vc in [greenVC, redVC] {
            let nav = UINavigationController(rootViewController: vc)
            navvedVcs.append(nav)
        }

        vc.viewControllers = navvedVcs

        return vc
    }


}

This is how I handle url

    func application(_ app: UIApplication, open url: URL, options: [UIApplication.OpenURLOptionsKey : Any] = [:]) -> Bool {

        guard let urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false),
            let queryItems = urlComponents.queryItems
            else {
                return false
            }


        guard let account = queryItems.first(where : {$0.name == "account"})?.value else {
            return false
        }

        switch urlComponents.host {
        case "green":

           try? Navigation.router.navigate(to: Path.green, with: NavigationContext(account: account), animated: true, completion: nil)

            return true
        case "red":

               try? Navigation.router.navigate(to: Path.red, with: NavigationContext(account: account), animated: true, completion: nil)

            return true

        default:
            return false
        }
    }

The bug happens when context is different and Route Composer needs to rebuild the entire stack

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm
Now i see. It is not a problem of action or the route composer.
Lets discuss the scenario.

  • You have a RedViewController visible.
    To simplify lets say that the context type is Int, and RedViewCOntroller has it equal to 1.
  • User deeplinks to RedViewController with the context equal to 2.
  • ClassWithContextFinder sees the RedViewController, but it context is equal to 1, so finder says - no it is worng view controller and ignores it.
  • Then router starts to build the whole stack from scratch, but the finder still cant find it (here i do not really understand why, but it can be equatable implementation that i also need to see the equatable implementation)

But i assume in your case you expect that the RedViewController will be found without rebuilding the whole view controller stack (it is obviously not necessary). And just it context will be updated.
So i assume you need to use a ClassFinder instead, and you have to update the context of the view controller that is found usnig the custom ContextTask or you can reuse ContextSettingTask not to create a custom context updating task.

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

ClassWithContextFinder has a very specific use case for the if you can have the multiple view controllers instance in your app. Like imagine that you display multiple products view controller with differnt product id's in the UINavigationStack. THis finder will be able to answer the question if such product is present or not, and if it is not present - there is a factory that will take care of it.
But from your scenario i see that context is a seconday thing, if the view controller exists - you need to update its context, but not rebuild the whole chain.

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

And you can see in your log

[Router] Started to search for the view controller to start the navigation process from.

[Router] BaseStep<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : FinderFactory<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>>(configuration: nil, finder: RouteComposer.ClassWithContextFinder<RouteComposerBug.RedViewController, Swift.Optional<RouteComposerBug.NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator()))))> hasn't found a corresponding view controller in the stack, so it will be built using FinderFactory<ClassWithContextFinder<RedViewController, Optional<NavigationContext>>>(configuration: nil, finder: RouteComposer.ClassWithContextFinder<RouteComposerBug.RedViewController, Swift.Optional<RouteComposerBug.NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator()))).

[Router] BaseStep<ClassWithContextFinder<TabbarViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : TabBarFactory())> hasn't found a corresponding view controller in the stack, so it will be built using TabBarFactory().

[Router] BaseStep<ClassFinder<AccountSelectorViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) : ClassFactory<AccountSelectorViewController, Optional<NavigationContext>>(nibName: nil, bundle: nil, configuration: nil))> found <RouteComposerBug.AccountSelectorViewController: 0x7f94808049d0> to start the navigation process from.

Your stack is being rebuild every time.

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

Please tell me if i understand correctly your business needs, then we can proceed.
Or you actually want to rebuild the whole stack. Then we have to look in to Equatable implementation

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm If you actually need to rebuild the whole stack in this case (i am not sure why, but lets say it is so). You need to put a break point in to func isTarget(for context: Context) -> Bool implementation inside of the RedViewController and it must have 2 calls, first when the route composer just look for the such view controller, second when RouteComposer just rebuilt the whole stack and what to be sure that it has rebuilt it correctly. It should return true because i see that your TabBarFactory correctly pushes the context to its children.
This error is being thrown by the FinderFactory that RouteComposer uses to be sure that it rebuild the whole stack correctly.

[Router] Composition Failed Error: ClassWithContextFinder<RedViewController, Optional<NavigationContext>>(iterator: RouteComposer.DefaultStackIterator(options: current, contained, presented, presenting, startingPoint: RouteComposer.DefaultStackIterator.StartingPoint.topmost, windowProvider: RouteComposer.KeyWindowProvider(), containerAdapterLocator: RouteComposer.DefaultContainerAdapterLocator())) hasn't found its view controller in the stack.

@ekazaev ekazaev added help wanted Extra attention is needed and removed bug Something isn't working labels Aug 23, 2019
@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

@ekazaev I my case it needs to rebuild the entire stack. User selects account first and then goes to the appropriate tabbar

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm Ok, let assume it is so. Then it seems that there is something with the isTarget/Equatable implementation. Basically you need to try to do what i asked:
You need to put a break point in to func isTarget(for context: Context) -> Bool implementation inside of the RedViewController and it must have 2 calls, first when the route composer just look for the such view controller, second when RouteComposer just rebuilt the whole stack and what to be sure that it has rebuilt it correctly.
It should return true on the second stop on the breakpoint and you need to findout and tell me why the contexts are not equal. I see that your TabBarFactory correctly pushes the context to its children on creation.
What happens now from what i see (Ill simplify context type to int).

  • You have a RedViewController with context equal to 1
  • You ask router to deeplink to the RedViewController with context equal to 2.
  • ClassWithContextFinder does not find it because 1 is not equal to 2
  • Router rebuilds the whole chain and i see that you TabBarFactory build the RedViewController and pushes the new context that is equal to 2 in to it.
  • On the last step router tries to make sure that it did everything correctly and runs ClassWithContextFinder again expecting to find a RedViewController with context equal to 2 and fails. My assumption is that the problem is either in isTarget/Equatable implementation or TabBarFactory dis not pushed the correct context value to the RedViewController that it has just built

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

@ekazaev I just tried it, Red VC isTarget is called only once

Implementation is very simple

extension RedViewController: ContextChecking {

    typealias Context = NavigationContext?

    func isTarget(for context: NavigationContext?) -> Bool {
        return self.context == context
    }

}

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

Here is NavigationContext implementation:

struct NavigationContext {

    let account: String

}

extension NavigationContext: Equatable {}

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

  • My assumption is that the problem is either in isTarget/Equatable implementation or TabBarFactory dis not pushed the correct context value to the RedViewController that it has just built

I think the problem has something to do with push animation.
I tried the same setup but instead of pushing tabbar it replaced root view controller. In this case it was able to select correct tab when context is new

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm can you send me your test project if it is possible?

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm so i wont build the stack myself. It can be a bug, or it can be the issue I mentioned before with the action. Unfortunately I deleted the code as I thought it’s not the case

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

@ekazaev Here is the example project https://github.com/0rtm/RouteComposerBug.
To reproduce the bug try running these commands in terminal

xcrun simctl openurl booted rcbug://red?account=Account2
xcrun simctl openurl booted rcbug://red?account=Account1

@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm I see the issue. It is not exactly the RouteComposer issue, but there is a way to avoid such UIKit behaviour for sure.
So basically Router asks UIKit to run next commands:
navigationController.popToViewController(accountViewController, animated: true)
and then after
navigationController.pushViewController(redViewController, animated: true)
And this is what brings a confusion as UINavigationController is not always works fine with the sequence of calls. But i will be able to fix it and will come with the fix shortly.
I was waiting when this transaction issue will appear but could not come with the scenario.
So it is an important thing to fix

@ekazaev ekazaev added bug Something isn't working and removed help wanted Extra attention is needed labels Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm https://www.dropbox.com/s/vwpl3so5kqykhit/BugFix.mov?dl=0 the behaviour after the fix

ekazaev added a commit that referenced this issue Aug 23, 2019
ekazaev added a commit that referenced this issue Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm New 2.1.4 version of RouteComposer is released and the issue you are facing is fixed there. It is also covered by the UI test so it wont appear in future.

Thank you for help.

If you'll have any other issues or questions, please do not hesitate to open another issue.

@ekazaev ekazaev closed this as completed Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 23, 2019

@0rtm As a suggestion:

  1. You can replace TabBarFactory with CompleteFactoryAssembly to avoid writing a boiler plate code. But do not forget to use some kind of ContextTask so the generic factory would pass the Context instance deeper to its children.
  2. In the configuration of the accountSelector step there should be .from(GeneralStep.root())

@0rtm
Copy link
Author

0rtm commented Aug 23, 2019

@ekazaev thanks! You are doing a great work

@ekazaev ekazaev reopened this Aug 23, 2019
@ekazaev
Copy link
Owner

ekazaev commented Aug 24, 2019

@0rtm I was actually in rush and did not notice that i was testing your code with my modifications :(

Youll still have to update to even newer version 2.1.5 and use a tweak CATransaction.wrap(...) in your configuration:

    static var accountHome: DestinationStep <TabbarViewController, NavigationContext?> {
        return StepAssembly(
            finder: ClassWithContextFinder<TabbarViewController, NavigationContext?>(),
            factory: TabBarFactory())
            .using(CATransaction.wrap(UINavigationController.push()))
            .from(accountSelector.expectingContainer())
            .assemble()
    }

When the push and then pop happens one after another UINavigationController does not have the pushed view controller in its containing view controllers until the end of the animation (which is for some reason is different from its usual behaviour). I am able to reproduce this weird behaviour of UINavigationController without RouteComposer and it causes malfunction of the Router. This is why you need to add it to your configuration to make the router wait till the end of the animation before it will start to check that it build the chain correctly. With this addition it is fine.
Sorry about that.

@ekazaev ekazaev closed this as completed Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants