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

[XF] Navigating between Tabs #650

Closed
dansiegel opened this issue Jun 20, 2016 · 101 comments
Closed

[XF] Navigating between Tabs #650

dansiegel opened this issue Jun 20, 2016 · 101 comments

Comments

@dansiegel
Copy link
Member

@dansiegel dansiegel commented Jun 20, 2016

When working with a Tabbed Page, and navigating between tabs there is currently no implementation that allows you to properly handle the navigation events as INavigationAware is only called by the INavigationService, not by native navigation, like in the event of tapping a different Tab.

As discussed we could possibly add a new interface and tie this in via a custom behavior.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jun 20, 2016

I'm curious if we could create a new interface that the NavigationService can use to control the TabbedPage navigation behavior. Something like

public interface ITabbbedPageNavigationOptions
{
    bool UseTabbedBasedNavigation {get;}
}

Then the TabbedPage could implement the interface and switch tabs instead of calling PushModelAsync/PushAsync.

But... that would require calling the navigation action off of the TabbedPageViewModel itself, not a page within the TabbedPaged. SO how would that work?

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jun 21, 2016

I'm thinking an approach similar to the ViewModelLocator's AutoWireViewModel property might work particularly well so that we can easily attach an event handler to the TabbedPage CurrentPageChanged and PagesChanged events.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jun 21, 2016

The only issue I can think about with the behavior is preventing the INavAware from invoking multiple times; once when navigated to, then again when responding the the tab changed event.

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jun 21, 2016

I would say that you want to fire when you load the initially selected tab, and again any time you change tabs. You may have an OnNavigatedFrom/To that needs to be handled at least if we're sticking with the current INavAware. Certainly how to handle things may get tricky, but that's on the developer to handle in their implementation not so much on the framework.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jun 21, 2016

Maybe a new interface would be better as to not to cause confusion between a navigation operation and a tab switching operation. I'm curious if passing parameters between tab changes would be needed, and if so how we could solve that.

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jun 22, 2016

I can certainly think of a few times where you may want to react to a change on one tab and update other tabs. I suppose you could use events, but I think there's a better way.

@polepage
Copy link

@polepage polepage commented Jun 29, 2016

I solved this particular issue with a Behavior that can be associated with a TabPage. It listens to the page change event, and calls TabNaviagatedFrom and TabNavigatedTo from the inner page ViewModels that implement ITabNavigationAware (a lot like the INavigationAware that already exist). I use a NavigationParameters object to pass parameters from one tab to the other.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jun 29, 2016

@polepage would you mind sharing your approach?

@polepage
Copy link

@polepage polepage commented Jun 29, 2016

Navigated from prompt the last view model to set the data to give to the next, Navigated to gives that data to the newly selected page. I used a different interface so a Page can also be INavigationAware. The thing works with any MultiPage,l not just TabPage.

public interface IMultiPageNavigationAware
{
  void OnInternalNavigatedFrom(NavigationParameters navParams);
  void OnInternalNavigatedTo(NavigationParameters navParams);
}

public class MultipageNavigationBehavior : Xamarin.Behaviors.Behavior<MultiPage<Page>>
{
  private Page _lastSelectedPage;

  protected override void OnAttach()
  {
    AssociatedObject.CurrentPageChanged += CurrentPageChangedHandler;
  }

  protected override void OnDetach()
  {
    AssociatedObject.CurrentPageChanged -= CurrentPageChangedHandler;
  }

  private void CurrentPageChangedHandler(object sender, EventArgs e)
  {
    NavigationParameters navParams = new NavigationParameters();
    if (_lastSelectedPage != null)
    {
      IMultiPageNavigationAware lastPageAware = _lastSelectedPage.BindingContext as IMultiPageNavigationAware;
      if (lastPageAware != null)
      {
        lastPageAware.OnInternalNavigatedFrom(navParams);
      }

      IMultiPageNavigationAware newPageAware = AssociatedObject.CurrentPage.BindingContext as IMultiPageNavigationAware;
      if (newPageAware != null)
      {
        newPageAware.OnInternalNavigatedTo(navParams);
      }
    }

    _lastSelectedPage = AssociatedObject.CurrentPage;
  }
}
@polepage
Copy link

@polepage polepage commented Jun 29, 2016

I use Xamarin.Behavior.Behavior(T) instead of the built in Xamarin.Forms.Behavior(T) because I had issues with the Xamarin Forms one some times ago and I never checked if my issues were fixed.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jun 29, 2016

@polepage thanks for sharing

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jun 29, 2016

@polepage this is a great approach!
@brianlagunas I would say we should add something like the ViewModelLocator so we can simply add the property to the TabbedPage and it can attach the behavior. I can look into this more later tonight.

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jul 19, 2016

@brianlagunas any thoughts as to whether this behavior should:

  • Be added automagically by the NavigationService for CarouselPages and TabbedPages
  • Be a Bindable Property that adds the behavior similar to the ViewModelLocator
  • Force developers to explicitly add the behavior? (Keep in mind with the other two, a developer always has the ability to do this if they want)
<TabbedPage xmlns:b="clr-namespace:Prism.Behaviors;assembly=Prism.Forms"
            xmlns:prism="clr-namespace:Prism.Mvvm;assembly=Prism.Forms"
            prism:ViewModelLocator.AutowireViewModel="True"
            prism:PrismBehaviors.AddMultiPageNavigation="True">
    <TabbedPage.Behaviors>
        <!-- Explicitly Added -->
        <b:MultiPageNavigationBehavior />
    </TabbedPage.Behaviors>
</TabbedPage>
@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jul 20, 2016

My gut has me leaning towards a behavior

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Jul 20, 2016

Well it has to be a behavior. The question is how do you think that behavior should be added. Should it be automatically by the NavigationService, by a BindableProperty making it an opt-in feature or explicitly added like any other behavior?

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Jul 20, 2016

I am leaning towards explicitly

@toadzky
Copy link

@toadzky toadzky commented Aug 3, 2016

I would really like to see this, is there anything I can do to help?

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Aug 3, 2016

@toadzky I have it all working. There are just some implementation details that have to be figured out. If this is holding you back I've created a Gist you can borrow from to get you through until it gets added. For now you'll need to add the behavior to your MultiPage (TabbedPage/CarouselPage).

That said any feedback people have on the expected behavior would be great. Specifically would the community prefer to have this behavior automatically added by the NavigationService or should it be on the Developer to add to each page? Or is there another way all together that people would like to see it work?

@toadzky
Copy link

@toadzky toadzky commented Aug 3, 2016

Personally, I would prefer it be added by default with a way to disable it. I think the more common use case is going to be that developers want it to have that behavior out of the box. In my case, the user selects an item from a list and it navigates to a TabbedPage where each page displays different data that has to be loaded. All of the pages need the parameters, but I can only give them to the first one.

I'll give the gist a try and let you know if any pain points jump out at me.

@msloan
Copy link

@msloan msloan commented Aug 5, 2016

I very much agree with @toadzky, this is behavior that I was expecting so it seems like it should be the default.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Aug 8, 2016

It can't be default because there is no easy way to remove it if it's not wanted. It will have to be an opt-in.

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Aug 8, 2016

I would make two points here:

  1. If your VM does not implement the IMultiPageNavigationAware you shouldn't notice a difference.
  2. If someone prefers to use their own behavior or something... We can easily incorporate a bindable property that they can attach similar to the VML.AutoWire property. We could use either a nullable bool or just name it something like DisableBehaviorInjection.
@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Aug 8, 2016

I am not a fan of adding a separate attached property to turn off a behavior. That is not a common or expected approach in XAML development.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Aug 9, 2016

So after playing around a little bit, I do kind of like that it is done automatically :)

The dev experience is much nicer. We just have to think of a good way to opt-out. Any other ideas besides an attached property? Maybe an interface IMultiPageNavigationOptions just like we have for NavigationPage and MasterDetail? It follows the same pattern anyways.

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Aug 9, 2016

If we were to do something like that, I would image we would want something that goes on the MultiPage (Tabbed/Carousel).

public interface IMultiPageNavigationOptions
{
    bool InjectNavigationBehavior { get; }
}

public class PageNavigationService
{
   async Task ProcessNavigationForTabbedPage( ... )
   {
       var options = currentPage as IMultiPageNavigationOptions;

       if( options == null || options.InjectNavigationBehavior )
           currentPage.Behaviors.Add( new MultiPageNavigationBehavior() );
   }
}
@sunefred
Copy link

@sunefred sunefred commented Sep 5, 2016

Would it be possible to have more general interfaces IDeactivateAware and IActivateAware that could also be called when activating and deactivating views through calls to Region.Activate etc. I wouldn't call switching tabs a navigation per say, it is more a user interaction to activate and deactivate a views.

@kdawg1406
Copy link

@kdawg1406 kdawg1406 commented Feb 19, 2017

Where are we on this issue? One of the most important features of Prism Navigation is object construction. Seems like we need a PrismTabbedPage that can handle tab navigation and object construction from the container.

@brianlagunas brianlagunas removed this from the Prism for XF 6.3 milestone Feb 26, 2017
@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Feb 26, 2017

Starting this thread up again. I think the best approach for tab change notifications is to use IActiveAware. Passing parameters between tab selections don't make a lot of sense. If this behavior is desired it can be achieved using either the IEventAggregator, or a class that maintains the state between the tabs. Like a singleton state manager service. When IActiveAware.IsActive is being set, you can set/read the parameters from this service. This also aligns the tab behavior with exactly how Prism for WPF works.

However, propagating parameters passed from the TabbedPage to the CurrentPage does make sense. For example, given I navigate to directly to the TabbedPage:

NavigatAsync("MyTabbedPage?name=Brian");

The first child should also be passed the parameter.

@kdawg1406
Copy link

@kdawg1406 kdawg1406 commented Feb 26, 2017

@dansiegel
Copy link
Member Author

@dansiegel dansiegel commented Feb 26, 2017

@brianlagunas I would say INavigationAware isn't what it was when this thread started. Namely the concept of OnNavigatingTo could allow all children to properly initialize so you could have something like: NavigateAsync("MyTabbedPage?tab=TabA&tab=TabB&name=Brian") and both TabA and TabB could initialize with Brian as a parameter even though only TabA will show up.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Feb 27, 2017

Okay, so the real issue is being able to pass parameters to either the first tab or all tabs when navigating to the TabbedPage for the first time. This in combination with IActiveAware should address the majority of TabbedPage issues?

@beylkhanovdamir
Copy link

@beylkhanovdamir beylkhanovdamir commented Mar 29, 2017

This in combination with IActiveAware should address the majority of TabbedPage issues?

IsActiveChanged handler is not firing when I'm switching between tabs in my app

@beylkhanovdamir
Copy link

@beylkhanovdamir beylkhanovdamir commented Mar 29, 2017

@brianlagunas

my version of prism is 6.3 pre-2

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Mar 29, 2017

You are responsible for firing that, not the framework. In the IsActive property setter, you raise that event, similar to how you implement INotifyPropertyChanged.

@beylkhanovdamir
Copy link

@beylkhanovdamir beylkhanovdamir commented Mar 29, 2017

but what profit in this way? according with your way I need anyways detect when tab was changed, but how I'll define it?

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Mar 29, 2017

Like I said, in the IsActive property setter, you know if the tab is active or not. Raise it there.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Sep 20, 2017

I have been investigating this issue and it's much more complex than I thought it would be. I have a branch that kind of works, but not perfectly.

If you navigate to the TabbedPage with no further navigation segments (NavigateAsync("TabbedPage")) all tabs will invoke the INavAware methods just fine.

However, I cannot prevent the INavAware methods from being invoked twice if you provide a deep link such as NavigateAsync("TabbedPage/NavigationPage/SomeTab"). In this case SomeTab will have it's INavAware methods invoked twice.

Now keep in mind that this would only happen if you are trying to deep link into your TabbedPage.

I am really leaning towards requiring having the propagation of parameters to all the other tabs being the responsibility of the developer. Essentially, this is all that would be required:

    public class MyTabbedPage : TabbedPage, INavigatingAware
    {
        public virtual void OnNavigatingTo(NavigationParameters parameters)
        {
            foreach(var child in Children)
            {
                PageUtilities.OnNavigatingTo(child, parameters);

                if(child is NavigationPage navPage)
                {
                    PageUtilities.OnNavigatingTo(navPage.CurrentPage, parameters);
                }
            }
        }
    }

This would give the developer full control over what tabs get what parameters. Although, it is a little more code that has to be written. We could provide some Gists in the docs to help, or maybe even some VS code snippets.

Thoughts?

@powerdude
Copy link

@powerdude powerdude commented Sep 21, 2017

i have 2 suggestions:

  • move the if check into the PageUtilities.OnNavigatingTo. No reason for developer to know the distinction there.
  • provide a behavior I could add to my code to "enable" this feature. No need for developer to litter their code with this code. That way it could be enhanced later if necessary.
@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Sep 21, 2017

It would not be wise to place the "if check" in the PageUtilties as that method is specific to invoking methods on the target page. Adding that logic may cause issues in other places the method is used (for example during deep linking). If we provided a behavior, it wouldn't solve the problem we have now... the INavAware methods would be invoked twice when you provide a deep link for a tab (NavigateAsync("TabbedPage/NavigationPage/SomeTab")). By handling it in the code-behind, you can prevent this from happening on a case-by-case basis.

@powerdude
Copy link

@powerdude powerdude commented Sep 22, 2017

Wouldn't it be easier to add the code to a behavior and use the behavior on a case by case basis. That way it is part if the library, allows developers to add the feature if they wanted it and would better than paying a code snippet everywhere. The behavior could do the if check if you don't want to move it into page utilities. I'd think more people would comfortable adding behaviors instead of posting code. Just my $0.02 and it fits better into the paradigm.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Sep 22, 2017

You can't use a behavior in this case because we have to respond to the INavAware methods that are called on the page directly. There is nothing for a behavior to listen to. I also don't think you fully undertand the issue with trying to call the INavAware methods on all tabs in general. There is no way to prevent them from being invoked twice.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 4, 2017

Okay, I have made great progress on this issue. This is where we are. I had to change the behavior of the TabbedPage.

NavigateAsync("TabbedPage/SomeTab") will no longer select the "SomeTab" tab of the TabbedPage. Instead it will navigate to the SomeTab page. If you wish to select a tab, you simply set a parameter (yet to be named) like this:

NavigateAsync($"TabbedPage?{KnownNavigationParameters.SelectedTab}=SomeTab").

To select tabs that are nested in a NavigationPage you simply provide a special syntax:

NavigateAsync($"TabbedPage?{KnownNavigationParameters.SelectedTab}=NavigationPage|SomeTab").

This is actually more flexible than what we have now.

Now the new problem. How do we indicate that I want to navigate within a Tab that is wrapped in a navigationPage? For example, right now you can use NavigateAsync("TabbedPage/NavigationPage/SomeTab/SomeOtherTab"). This will have the TabbedPage as the root of the app, and navigate the SomeOtherTab within the current tab.

With this new approach this will not work. I need to know how to determine the intent of the navigation.

Should we use another parameter in the target similar to:

NavigateAsync($"TabbedPage?{KnownNavigationParameters.SelectedTab}=NavigationPage|SomeTab/SomeOtherTab?navWithinTab=true").

Should the default behavior always navigate from the TabbedPage.Current page, or do we default to navigating from the TabbedPage itself?

What are your thoughts?

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 5, 2017

Another option is to have somewhat of a nested navigation which is indicated by the '|' character:

NavigateAsync($"TabbedPage?selectedTab=NavigationPage|ViewA|ViewB/SomeOtherView")

This means that you will not be able to pass individual parameters to any nested pages. Only to the NavigationPage.CurrentPage

How common is it to do a nested navigation in a tab during a deep link?

@OpticNectar
Copy link

@OpticNectar OpticNectar commented Oct 5, 2017

Would it not be easiest to just register a page as a TabbedPage Tab? That way when that page is in the URL Prism knows it's a tab and selects that tab.

NavigateAsync("TabbedPage/FirstTabNavPage/ViewA")

The only thing that it wouldn't cover is if you want to navigate a tab without selecting it. For this I think it would make sense to have a bool option for SelectTab or something like that.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 5, 2017

Knowing that is a tab is irrelevant. The syntax you provided will no longer work in the new API. You will need to provide a selectedTab parameter.

@OpticNectar
Copy link

@OpticNectar OpticNectar commented Oct 5, 2017

Are there docs for the new API anywhere so I can mess around with it?

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 5, 2017

No, I am working on it now.

@OpticNectar
Copy link

@OpticNectar OpticNectar commented Oct 5, 2017

Awesome! On top of things as always. Let me know when you have them put together i'd love to tryout the new version. The hodge podge of a mess I have now getting the tabs to work is having all sorts of issues.

@brianlagunas brianlagunas added this to the Prism 7.0 milestone Oct 5, 2017
@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 5, 2017

There is now a breaking change in the TabbedPage behavior. Please read PR #1195 for detail ont he new syntax.

The good news is now all tabs will receive parameters in the OnNavigatingTo method. You no longer have t manually forward these.

@brianlagunas
Copy link
Member

@brianlagunas brianlagunas commented Oct 5, 2017

Feel free to test this out with the latest 7.0.0.124-ci build on MyGet

@lock
Copy link

@lock lock bot commented Jan 30, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.