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

Add support for FrameworkContentElement as a region in Prism.Wpf #1321

Closed
JimWuerch opened this issue Jan 3, 2018 · 14 comments
Closed

Add support for FrameworkContentElement as a region in Prism.Wpf #1321

JimWuerch opened this issue Jan 3, 2018 · 14 comments
Labels

Comments

@JimWuerch
Copy link

Package info

  • Platform: WPF
  • Prism version: 6.3.0
  • Xamarin version (if applicable):
  • Windows 10 SDK version (if applicable):
  • Other version info:

Repro steps

In source\wpf\prism.wpf\regions\behaviors\delayedregioncreationbehavior.cs the WireUpTargetElement() function expects that the target element is FrameworkElement. I'm trying to use regions with a 3rd party windowing toolkit (devex), but the UI bits that I want to use are derived from FrameworkContentElement.

This actually leads to a really hard to figure out bug where sometimes regions get created and sometimes they dont, based on whether or not the garbage collector has run since the view InitializeComponent() has been run. If the GC doesn't run, the DelayedRegionBehavior object instance still exists, and the region gets created. But if the GC runs, that object is gone and the region never gets created.

I tested a quick fix that added a 2nd check in WireUpTargetElement() for FrameworkContentElement and it fixed my issues.

It'd also be nice if some sort of message was logged when WireUpTargetElement() fails, because at that point it's up to fate as to whether or not your region will get created.

@brianlagunas
Copy link
Member

This is one of those issues that applies to a specific implementation. The recommendation in this case would be to create your own region behavior and register it in the app that requires this functionality. If in the future more people need this functionality, it can be re-evaluated for inclusion in the Prism code base.

@galakt
Copy link
Contributor

galakt commented Jan 4, 2018

Same as #452

@brianlagunas
Copy link
Member

That is a different issue. That was not a region support request, it was for a ViewModelLocator request which also has a work-around. Even if it was the same, two requests do not constitute a wide usage requirement.

@galakt
Copy link
Contributor

galakt commented Jan 4, 2018

@brianlagunas By "same" i mean linked to DevExpress, sorry for misleading

@JimWuerch
Copy link
Author

It would've saved me a lot of time if using a non-FrameworkElement never worked, instead of only working if the garbage collector hasn't run between creating the behavior and the WeakDelegateManager having Raise called.

FrameworkElement and FrameworkContentElement are generated from the same source, just FrameworkContentElement doesn't have layout. It's certainly possible users are using FrameworkContentElement for regions, and they are just getting lucky. We were fine until we added a splash screen that must have used to enough resources to trigger a GC call. I'll ask on reddit to find out how to work around this.

@brianlagunas
Copy link
Member

The work-around is to replace the current delayed region behavior with your own implementation to account for the required FrameworkContentElement.

@brianlagunas
Copy link
Member

Have you tried just registering your behavior in the ConfigureDefaultRegionBehaviors?

@JimWuerch
Copy link
Author

Sorry, I deleted my previous comment. I'm not sure what key I would use to register my behavior, because DelayedRegionCreationBehavior doesn't get registered there, nor does it have a BehaviorKey string member.

@brianlagunas
Copy link
Member

give it what ever key you want. This behavior should be specific to your RameworkContentElement and not duplicate the existing one. You aren't replacing, you're adding.

@spsrbud
Copy link

spsrbud commented Jan 8, 2018

I had the exact same problem as the OP using a custom region adapter for Avalon Dock's LayoutDocumentPaneGroup. The real problem is that LayoutDocumentPaneGroup does not inherit from FrameworkElement, but it is a DependencyObject. The OP is correct about the WireUpTargetElement() method keeping the DelayedRegionCreationBehavior instance alive because the FrameworkElement's Loaded event now has a reference to the instance of DelayedRegionCreationBehavior. If the TargetElement is not a FrameworkElement then it runs the risk of the GC destroying it before WeakDelegatesManger can invoke the handler for updating the regions. This is due to the fact that an instance of DelayedRegionCreationBehavior is created locally in RegionManager's CreateRegion(DependencyObject element) method. After that method executes, the instance of DelayedRegionCreationBehavior has nothing holding onto it (except for the FrameworkElement's Loaded event) and it gets marked for deletion by the GC.

A good fix would be to figure out a way to keep the instance of DelayedRegionCreationBehavior alive without having to use the FrameworkElement's Loaded event.

I worked around the issue myself by creating a UserControl that wrapped Avalon Dock's LayoutDocumentPaneGroup. Then, I created a custom region adapter for my UserControl (since it's a FrameworkElement), and I haven't seen the problem since.

@JimWuerch
Copy link
Author

At this point, I'd like to have just had it fail to create the DelayedRegionBehavior instance or thrown an exception when the WireUpTargetElement() fails to do anything. Or heck, even log a message or something. It'd have been obvious right away that something was up, instead of having it suddenly stop working later when we added something else (a splash screen) that caused the GC to run.

Leaving in code in the library that has a silent failure caused us a lot of lost time.

I worked around it by having my own copy of RegionManager and DelayedRegionCreationBehavior. I'm unconvinced that FrameworkContentElement shouldn't be equally as valid as FrameworkElement, given they are generated from the same source. I think if a GC.Collect() was stuck in right before the call to WeakDelegatesManager.Raise(), I'd bet a bunch of apps out there might stop working, because they are just getting lucky that the Initialize() call on their class gets called before the GC wipes out the behavior.

@brianlagunas brianlagunas reopened this Jan 10, 2018
@brianlagunas
Copy link
Member

brianlagunas commented Jan 10, 2018

Submit a PR with your proposed fix with supporting tests. if it's low enough impact, we will consider adding it.

@brianlagunas
Copy link
Member

Fixed by the community! A big thank you to @JimWuerch for the PR

@lock
Copy link

lock bot commented Jan 29, 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 29, 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

4 participants