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

RemoveAll throws if KeepAlive is false #993

Closed
Haukinger opened this issue Mar 23, 2017 · 10 comments
Closed

RemoveAll throws if KeepAlive is false #993

Haukinger opened this issue Mar 23, 2017 · 10 comments
Labels

Comments

@Haukinger
Copy link

Package info

  • Platform: WPF
  • Prism version: 6.2.0

Repro steps

  1. Have a MyViewModel implement IRegionMemberLifetime and return false from KeepAlive.get
  2. Navigate a region to a view (_regionManager.RequestNavigate( "MyRegion" ), nameof( MyView ) ))
  3. Clear the region (_regionManager.Regions["MyRegion"].RemoveAll())
  4. Exception:

"The region does not contain the specified view."
at Prism.Regions.Region.GetItemMetadataOrThrow(Object view)
at Prism.Regions.Region.Remove(Object view)
at Prism.Regions.Behaviors.RegionMemberLifetimeBehavior.OnActiveViewsChanged(Object sender, NotifyCollectionChangedEventArgs e)
at System.Collections.Specialized.NotifyCollectionChangedEventHandler.Invoke(Object sender, NotifyCollectionChangedEventArgs e)
at Prism.Regions.ViewsCollection.OnCollectionChanged(NotifyCollectionChangedEventArgs e)
at Prism.Regions.ViewsCollection.NotifyRemove(IList items, Int32 originalIndex)
at Prism.Regions.ViewsCollection.RemoveFromFilteredList(Object item)
at Prism.Regions.ViewsCollection.SourceCollectionChanged(Object sender, NotifyCollectionChangedEventArgs e)
at System.Collections.Specialized.NotifyCollectionChangedEventHandler.Invoke(Object sender, NotifyCollectionChangedEventArgs e)
at System.Collections.ObjectModel.ObservableCollection'1.OnCollectionChanged(NotifyCollectionChangedEventArgs e)
at System.Collections.ObjectModel.ObservableCollection'1.RemoveItem(Int32 index)
at System.Collections.ObjectModel.Collection`1.Remove(T item)
at Prism.Regions.Region.Remove(Object view)
at Prism.Regions.Region.RemoveAll()

When debugging, the region's ActiveViews and ItemMetadataCollection are both empty at the time the exception is caught, while Views still contains an instance of MyView.

If KeepAlive.get returns true or IRegionMemberLifetime is not implemented at all, no exception is thrown when clearing the region.

@dvorn
Copy link
Contributor

dvorn commented Mar 24, 2017

Seemingly relates to #382. @Haukinger Do you have a simple project that demonstrates the problem?

@Haukinger
Copy link
Author

Sure thing, have a look here: https://github.com/Haukinger/prism-993/tree/master

@dvorn
Copy link
Contributor

dvorn commented Mar 27, 2017

This is a problem with long history:

http://compositewpf.codeplex.com/discussions/396304
http://compositewpf.codeplex.com/discussions/322324
http://compositewpf.codeplex.com/discussions/272855

Basically, the original Prism authors do not have a solution to it: in a nutshell, their recommendation is "if a view implements KeepAlive=false, use Deactivate, otherwise you may use Remove".

Wonder whether a clean solution is possible.

@brianlagunas
Copy link
Member

I am actually leaning towards closing this as "by-design". Trying to get two different concepts to work the same way interchangeably is probably not a box I want to open. If using RegionMemberLifetime, then you are explicitly using the Navigation framework. If you are using Region.Clear/Remove, you are using View Injection. This will be a tough sell for me.

@dvorn
Copy link
Contributor

dvorn commented Mar 27, 2017

@brianlagunas However I think I can fix it...

@brianlagunas
Copy link
Member

@dvorn that would be great! This is a very low priority for me :)

@Haukinger
Copy link
Author

@brianlagunas if I cannot use Region.Remove to navigate away from a view without navigating to another view, is there any other way to achieve this?

@brianlagunas
Copy link
Member

There are a number of approaches described in the links that @dvorn provided. One of them is to deactivate the view

@dvorn
Copy link
Contributor

dvorn commented Mar 28, 2017

This is the same issue as #382. An attempt to fix it in #382 is essentially what original Prism authors recommended in http://compositewpf.codeplex.com/workitem/8224. However, sometimes this does not work for the following reason.

A region contains views as a itemMetadataCollection, and Region.Views and Region.ActiveViews are actually ViewsCollection wrappers over itemMetadataCollection which contain a cache of filtered and sorted items from itemMetadataCollection. ViewsCollections subscribe to changes on itemMetadataCollection so that when itemMetadataCollection is updated, the cached items are also updated.

The problem with the code

if (Region.Views.Contains(inactiveView)
     Region.Remove(inactiveView);

is that Region.Remove removes the view from itemMetadataCollection but Region.Views.Contains checks the presence of the view in the cached list. So the result is dependent of the order in which cached collections are updated when itemMetadataCollection changes.

If ActiveViews is updated first, the RegionMemberLifetimeBehavior will be triggered and the above code executed when Views has not been updated yet which results in a crash.

The order in which Views and ActiveViews are updated depends on the order of first accesses them: they are lazy initialized, and when initialized, they subscribe to itemMetadataCollection and subscriptions are fired in the order of subscriptions.

Views/ActiveViews may be first access, that is initialized, in any order. For instance, when a region is defined in XAML for a ContentControl, the ContentControlRegionAdapter will first access ActiveViews and then Views. In some other cases Views may be accessed first.

Now that we understand what's happening we may think about solutions. Many solutions are possible, including stop caching in Views or special eventing on ActiveViews to distinguish between removing a view from a region and deactivating a view.

I think the least disruptive approach would be to fix the order in which Views and ActiveViews are initialized: Views should be initialized first. For instance, remove lazy initialization for Views and initialize them in Region's constructor.

@lock
Copy link

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.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants