Prism creating new view even if view exists in region.

[wowfood]

Package info

Platform: Windows 7
Prism version: 6.3.0
Other version info: Prism.Core, Prism.Ninject, Prism.Wpf:

https://stackoverflow.com/questions/39776983/prevent-prism-from-creating-new-view-on-navigation/46214420#46214420

Repro steps

Experienced the same issue as this old stack overflow question, and I've put the cause as an answer, but in summary of it all.

Problem

When navigating to an existing view, a new instance of the view is added to the region, rather than reusing the existing version of said view.

Cause

Using a PageName to set a constant GUID for the page, but the pagename doesn't match the class name of the view. e.g.

Kernel.RegisterTypeForNavigation<PageNameView>( PageNames.P1);

public static class PageNames
{
    public const string P1= "FirstPage";
}

In the prism source, when checking to see if a view needs to be added to a region, the code checks where the navigationcontext.guid (FirstPage), is equal to the name or fullname of the view already in the region (PageNameView), because the two don't match, a new instance of the view is added to the region (meaning there can be multiple PageNameView in the region).

How to prevent this

Simple way of preventing it is to make sure your class name matches your PageName.

I don't know if this is a bug, or is working as intended.

I will say, if this is working as intended it would be nice if we could use anything as the GUID, simply for the sake of refactoring. (change class name for some reason, and suddenly IsNavigationTarget isn't hit).

[brianlagunas]

It appears this is the same problem that Autofac had in #1120. The reason this is happening is because the original author of the Ninject container support did not properly implement it. It is missing the NinjectRegionNavigationContentLoader. I do not use Ninject, so I am not exactly sure of how to implement the content loader.

Please follow the exact steps in #1120 to implement a Ninject content loader in your project. Register it with your container and test it out. If it works, please let me know.

[brianlagunas]

Do you have an update to share?

[wowfood]

Sorry haven't had a chance to yet. We're on a tight schedule with our current project so for now I'm relying on the workaround.

When I get a chance I'll have a go at following the steps in the other issue.

----edit----

I've had a quick look, but I'm struggling to find any ninject equivalent to the container.registrations stuff. I'll keep looking round but can't promise much.

---edit---

Made some progress (i think) but still not working. Must be doing something wrong.

[brianlagunas]

Try this one and let me know if it works

    public class NinjectRegionNavigationContentLoader : RegionNavigationContentLoader
    {
        IKernel _kernel;

        public NinjectRegionNavigationContentLoader(IServiceLocator serviceLocator, IKernel kernel) : base(serviceLocator)
        {
            _kernel = kernel;
        }

        protected override IEnumerable<object> GetCandidatesFromRegion(IRegion region, string candidateNavigationContract)
        {
            IEnumerable<object> contractCandidates = base.GetCandidatesFromRegion(region, candidateNavigationContract);

            if (!contractCandidates.Any())
            {
                var matchingRegistration = _kernel.GetBindings(typeof(object)).Where(x => candidateNavigationContract.Equals(x.Metadata.Name, StringComparison.Ordinal)).FirstOrDefault();

                if (matchingRegistration == null)
                    return contractCandidates;

                var kernelTarget = matchingRegistration.ProviderCallback.Target;
                var typeCandidateName = kernelTarget.GetType().GetField("prototype").GetValue(kernelTarget).ToString();
                contractCandidates = base.GetCandidatesFromRegion(region, typeCandidateName);
            }

            return contractCandidates;
        }
    }

In bootstrapper

        protected override void ConfigureKernel()
        {
            base.ConfigureKernel();
            Kernel.Rebind<IRegionNavigationContentLoader>().To<NinjectRegionNavigationContentLoader>().InSingletonScope();
        }

[wowfood]

Yes that all seems to work, I've got the code all implemented, but it won't allow me to put together a PR. I'm pretty certain the only difference between my code and yours is some syntax stuff.

        protected override IEnumerable<object> GetCandidatesFromRegion(IRegion region,
            string candidateNavigationContract)
        {
            if (candidateNavigationContract == null || candidateNavigationContract.Equals(string.Empty))
            {
                throw new ArgumentNullException(nameof(candidateNavigationContract));
            }

            var contractCandidates = base.GetCandidatesFromRegion(region, candidateNavigationContract);

            if (!contractCandidates.Any())
            {
                //First try friendly name registration. If not found, try type registration
                var matchingRegistration = _kernel
                    .GetBindings(typeof(object)).FirstOrDefault(r => candidateNavigationContract.Equals(
                        r.BindingConfiguration.Metadata.Name,
                        StringComparison.Ordinal));

                //   var matchingRegistration = this.container.Registrations.Where(r => candidateNavigationContract.Equals(r.Name, StringComparison.Ordinal)).FirstOrDefault();
                if (matchingRegistration == null)
                {
                    matchingRegistration = _kernel
                        .GetBindings(typeof(object)).FirstOrDefault(r => candidateNavigationContract.Equals(
                            r.BindingConfiguration.Metadata.GetType().Name,
                            StringComparison.Ordinal));
                }

                if (matchingRegistration == null)
                {
                    return new object[0];
                }

                var kernelTarget = matchingRegistration.ProviderCallback?.Target;
                var typeCandidateName = kernelTarget?.GetType().GetField("prototype").GetValue(kernelTarget).ToString();

                contractCandidates = base.GetCandidatesFromRegion(region, typeCandidateName);
            }

            return contractCandidates;
        }

[wowfood]

As a note, I keep getting the following when I attempt to push my branch in order to put together a PR.

git -c diff.mnemonicprefix=false -c core.quotepath=false push -v --tags --set-upstream origin Ninject_content_loader_implementation:Ninject_content_loader_implementation
Fatal: ArgumentException encountered.

remote: Permission to PrismLibrary/Prism.git denied to wowfood.

fatal: unable to access 'https://github.com/PrismLibrary/Prism.git/': The requested URL returned error: 403
Pushing to https://github.com/PrismLibrary/Prism.git

Completed with errors, see above.

[brianlagunas]

For one, you can't push any changes you would have to submit a PR. Two, your code won't work because Metadata.GetType is not the type of the object being resolved.

[wowfood]

Are you referring to the gettype after the null check? I'm going to guess it was working for me because it wasn't hitting that null check anyway.

As for the pull request. This was all done on a separate branch off of master, I had thought that to do a pull request I first had to push this separate branch, and then the pull request is between that branch and master. (Sorry this is pretty much my first forray into using github, still a noob in this area)

[brianlagunas]

I'm going to guess it was working for me

Don't guess. Debug and verify values.

You do need to create a branch off master. That is what you use to create the PR. Does this help:
https://help.github.com/articles/creating-a-pull-request/

[wowfood]

I apologise and I'll make sure to test it more thuroughly next time.

As for the branch stuff. I checked out master, and created a local branch from that called Ninject_content_loader_implementation

It's when I attempt to push back that local branch to the remote, so that I can create a PR that I'm getting the above issue. Like I said, i apologize for being a bit slow in this area.

--edit--

Figured it out. Had to fork the branch to create teh PR, rather than branching directly off of it.

[moe45673]

@brianlagunas Hey, my company is with v6.2 but the IsNavigationTarget issue has been annoying me to no end as my URIs cannot match the shortfilenames. So I worked up a fix, ensuring it has no effect on performance.

Basically, it's create a class that's a glorified Dictionary<Uri, Type> called UriTypeMapper with appropriate getters and setters. It's a Singleton that's registered with Container in Bootstrapper.ConfigureContainer().

Implement the IRegionNavigationContentLoader.

In the IRegionNavigationContentLoader..GetCandidatesFromRegion() method, call UriTypeMapper.GetTypeFromString(string str). GetTypeFromString() first checks the Dictionary.Contains(str) method - if false it calls the private Refresh() method (which goes through the DIContainer.Registration collection and builds the Dictionary from scratch). Regardless, it returns the associated type or throws KeyNotFoundException.

Once back to the calling method, it goes through the Region and builds the collection based off view.GetType() == UriTypeMapper.Value, and not string.equals(view.GetType().Name || view.GetType().Full yawn... Fullname, uriAsString). Which it then returns.

So it's a mapping class that allows the RegionManager to tap into the Registrations while preserving decoupling. That's the idea, a few improvements can be tweaked such as a better way to update the mapping Dictionary.

[brianlagunas]

This shouldn't be an issue if there is a proper RegionNavigationContentLoader for the container you are using. If it's not working for you then make sure you are using the latest 6.3 version. Also, we will need to know what container you are using. A lot of these container support was added by the community and they didn't do it correctly.

[moe45673]

@brianlagunas Thanks for the response. Using UnityContainer FYI so it shouldn't make a difference.

The default functionality of URIs being matched to GetType().Name is written in the Prism Documentation:

http://prismlibrary.github.io/docs/wpf/Navigation.html
"Note: For Prism to determine the type of the target view, the view's name in the navigation URI should be the same as the actual target type's short type name. For example, if your view is implemented by the MyApp.Views.EmployeeDetailsView class, the view name specified in the navigation URI should be EmployeeDetailsView. This is the default behavior provided by Prism. You can customize this behavior by implementing a custom content loader class; you can do this by implementing the IRegionNavigationContentLoader interface or by deriving from the RegionNavigationContentLoader class."

I created a custom mapper because I disagree that that's the way the default behavior should be. A Uri should not need to be tied to the typename.

[brianlagunas]

Ahhh... now I see what you mean. The issue is ensuring the use of the mapper to produce reliable results. Since there are many ways to register a view for navigation, this may be a difficult thing to enforce. I would have to see what you are talking about exactly before really commenting on the feasibility of it. Show me the code :)

[moe45673]

Hey man, I've got a lot of respect for you and Prism itself so I'm glad to hear that my desire for URI-Type independence is considered noteworthy :)

Here's a link to the code and where it's used. I'm using WPF, FWIW

https://pastebin.com/uk8W2DWT

[brianlagunas]

Thank you for the kind words. Just so you know... everyone's code is noteworthy. Sparking conversations is how change is made.

Regarding this implementation. I am concerned about if other containers will play nice with this. Maybe if we introduced some type of NavigationRegistry (similar to what we have in XF) that when you use the container.RegisterForNavigation method, we will register your custom name with the type. Then the content loader will look in the registry for the mapping.

See this as an example: https://github.com/PrismLibrary/Prism/blob/master/Source/Xamarin/Prism.Forms/Ioc/IContainerRegistryExtensions.cs#L34

This approach should not break anyone, and will only work with using the built-in registration methods. Otherwise, it will fall back to the current way of resolving type names.

What do you think?

[moe45673]

Hmmmm, I am a bit torn on focusing on registering a view for navigation as my design is usable with any aspect of Prism (such as IEventAggregator), not just the RegionNavigation aspect, since the Mapper holds ALL the registrations in the container. It doesn't care who calls it. It runs quickly in the GetTypeFromUri() method because it's a Dictionary (and not a List like in Unity) and when searching candidates in the ContentLoader, it only has a relative handful of views in the region to go through when comparing Types.

There are some aspects that need to be tightened up in my UriTypeMapper. One is if a registration is edited in the unityContainer.Registrations after launch, the GetTypeFromUri() won't trigger the Refresh method and will return the original Type. Another is the way I add a Key if Registration.Name is null; I use the MappedToType.fullname. I am unsure if that is correct but I don't want to use "MappedToType.Name" because different aspects of a large application can use the same classnames (with different namespaces).

Perhaps there can be a custom IContainerRegistrationBehaviours interface that holds a Refresh() method whose concrete implementations are based on the concrete Container used. A reference to this is composed within UriTypeMapper and UriTypeMapper.Refresh() wraps IContainerRegistrationBehaviours.Refresh()? I think that's the Strategy design pattern?

I don't want to squander this opportunity but it's late and its Valentine's Day and I've got a wife waiting at home. Additionally, I want to have a close look at the suggestions and code you sent me tomorrow when I'm more awake and when my colleagues are here to consult with. They're really supportive of my desire to get the damned IsNavigationTarget() to properly work in our application with our Uri-naming constraints.

I really appreciate your attention and will pick this back up tomorrow. Cheers, man!

[brianlagunas]

Keep in mind, not all containers behave the same and may not give you access to registrations and types like your approach requires. Also, Prism is moving away from container specific features and towards our new Prism.Ioc namespace and classes.

See here: https://github.com/PrismLibrary/Prism/tree/master/Source/Prism/Ioc

So this needs to exist agnostic to a container.

[moe45673]

Wouldn't my paragraph about IContainerRegistrationBehaviours address this? The UriTypeMapper.RefreshMapping() method is the only part that relies on the concrete container and as long as there is some way to access the Uris and mapped types in the desired container, it doesn't matter how it's done.

Hopefully other containers offer events or something you can subscribe to when the Registrations are updated and the mapper can populate its dictionary that way.....

[moe45673]

Seriously, though, I want to address your points coherently but my brain works better earlier in the day and I need to study your links. We'll speak tomorrow :)

[moe45673]

Hey Brian! I have studied your points with a fresh mind. I really like what the RegisterForNavigation method is doing but my issue with it is that it is limited to the RegionManager element of Prism. I also have issue with the fact that it needs to be explicitly called by the user for the registry to be updated. I fully believe that a Prism-managed auto-updated global registry is the way to go as then the user can use their Uri's anywhere.

I fully agree that the registry/mapper needs to be container agnostic. My main experience with Prism is with WPF and Unity, so I am unfamiliar with other DI implementations. Could you point me in the direction of a widely used container or two that makes it difficult to scan the internal Registrations?

I checked out the IOC folder, is the idea behind this that all containers that are compatible with Prism need to implement these interfaces? Is there a way to ensure that something algorithmically similar to RegisterForNavigation() is how the Extension.Register() methods are implemented, like the Template pattern would do?

[brianlagunas]

I'm not sure what you mean by "limited to the RegionManager". How/where else would this be used? This issue here is the IsNavigationTarget not respecting the custom registered name. Trying to design for possible external usages is not something I am really interested in. Anyways, using a navigation registry doesn't limit how it can be used. You would still be able to access it just like you can your approach.

Ninject is a perfect example of a container that is a pain in the ass to get registration info. Unfortunately, every container that we currently support, and possibly potential future containers, would need to be researched in order to support your approach.

In order for a container to be used with Prism, it will need to implement the required interfaces. In fact, the implementations are quite simple compared to how they must be done now.

[moe45673]

@brianlagunas Regarding your first point, I hope I'm not misinterpreting but if you're referring to RegisterForNavigation(), obviously that's designed and only meant to be used by the Regions. What I meant to say was that that method should be done away with and the functionality of it should be built into Ioc.IContainerRegistry.Register(). The "NavigationRegistry" object that the former method calls for should actually be a global registry, not one specifically made for the Region namespace.

I respectfully disagree about what the issue is. I think the fact that IsNavigationTarget doesn't respect custom registered names is a symptom of a deeper issue. The real core of the problem is one of two things

1. Either passing the Container's internal Registration.Name to the NavigationContentLoader is inappropriately tightly coupling the two objects, in which case another method for identifying the candidates should be used
2. Every object in Prism should have access to the registrations for cases like these.

The second point is actually true in practice with containers like Unity, only it takes a large amount of time to search through the Registrations.

Note that this is from my viewpoint down here in the weeds and if you can school me good, I'm really eager to learn more and adapt my own approach to code.

I'm going to look at NInject and see what they provide for access so I can see about making it more agnostic. Your input is awesome, thanks!

Having said all that, I'm fairly confident that my mapper would work in any instance. It doesn't have to be a <Uri, Type> dictionary (I just did it that way to whip something up quickly, back of a napkin/proof-of-concept type thing), it would more appropriately be a <Uri, IContainerRegistration> type dictionary to match my concept of it.

[brianlagunas]

RegisterForNavigation is a helper method to enforce the convention for registering a view for navigation, since it must be done in a specific way in order to function properly. You can't just register a view type as you would any other service with the container. RegisterForNavigation is also a standard practice for other supported platforms such as Xamarin.Forms, so we need to be careful about the library conventions when working with multiple platforms. We can't have it work on way on one platform, and completely different on another.

I'm not opposed to your approach, but this does rely on the container providing this information as well as the ability to update the mappings when new registrations are made at anytime during the app lifecycle. Except in the case of immutable container which can never have new registrations during app runtime. This in itself could be very challenging depending on the container being used.

Not to mention this line of code here ServiceLocator.Current.GetInstance. This line of code will never be allowed to be added into Prism again. We are actually in the process of trying to remove every instance of it from Prism.

The scope of this enhancement is for navigation only. When general statements like "Every object in Prism should have access to the registrations for cases like these", I always pause and get defensive. Unless other scenarios can be identified and validated as use-cases for this enhancement, there is no need trying to support unknown use-cases.

[moe45673]

I apologize, I was hasty in declaring that the NavigationRegistry should be done away with. I haven't looked into that specific component as it's Xamarin-focused. I did realize after I posted that the NavigationRegistry likely has specific data beyond that within the container. Cue Homer_Doh.jpg. Nonetheless, NavigationRegistry's inclusion or not should have no bearing on my proposal.

Can I ask why Xamarin has a registry and WPF does not?

The reason I bring up my point 2 in my prior post is that the default comparison code feels really, really amateur to me, that hardcoded comparison, and I can't help picturing that the guy writing the code was too tired when he reached this unexpected hurdle . I'm no Alan Turing but even I see that and I can't imagine you don't. So who knows what future interfaces will be added to Prism, only for this issue to come up again?
In other words, It happened before, it can happen again. It's a little stronger than a hypothetical what-if. However, you're a well known software architect and I'm not even sure everyone in my office knows my name, so I defer to you :)

And that bit about the ServiceLocator, I get why you wrote that. Yeah, that's bad code but it can be easily fixed by injecting the container into the constructor or by nabbing a reference from the RegionManager or by.... well, there are other ways. Yes, it's bad and I get why it bothers you, but it is not the torpedo to sink my proposal (as mentioned, this is more a Proof of Concept to ask your advice on if my department should go ahead with and if it's safe for future versions of Prism; I also bring it up with you just in case it is the solution you guys have been searching for). I do like that you mentioned how the ServiceLocator bugs you and I appreciate any further criticisms you may have because experience :)

[brianlagunas]

The reason XF has a registry and WPF does not is because I didn't write the WPF region code :). XF navigation is quite complex, and we needed a registry to keep track of information about the views available for navigation. I agree that there is improvement needed here. Especially since IsNavigationTarget doesn't work with custom navigation keys. Seems like that should have been fixed before region navigation was ever released.

Regarding number 2, this code has been in the wild for a decade and used in millions of apps. Literally millions. If there was a need for Prism to access the custom registration keys in other objects, we would know by now. So it is very much a hypothetical.

The only difference between your approach and my approach is that you are relying on the container to provide the information, manage updating it whenever new registrations are made (which can happen anywhere at anytime), and the rest of baggage that may come with it. This is brittle, creates a dependency (we are trying to eliminate these now), and can be very limiting in which container will expose the API necessary to accomplish the task. Where-as if there was just a simple NavRegistry that was populated when pages were registered for navigation (that pattern isn't going anywhere), then not only is the reliance on a container removed, but it will be faster and much easier to maintain. If you really wanted to access the nav keys for a view, you could just use the NavRegistry to get them.

The scope of this enhancement is only to work with navigation, not to provide some large directory of all registered services in an app.

[moe45673]

Hey, @brianlagunas I know I've been quiet but had an extremely busy week and had to put this aside. My hardcoded fix for Unity is satisfactory for my company's purposes and will be pushed into production if no changes are made. I want to keep this conversation going regardless, just for my own personal neurosis about leaving a quick-fix and missing out on a learning experience. Just finished up a milestone this week so I will dedicate more time to this next week (and will properly respond to your post and all the fun things I repeatedly say)

[GioviQ]

I have the same issue with DryIoc, so I matched uris with view names to solve.

[lock]

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.