search

Dirkster99 / AvalonDock

Our own development branch of the well known WPF document docking library
Microsoft Public License
1.41k stars 320 forks source link

Hiding and showing anchorable in document's pane throws an exception #71

Open Rasenshurikenbum opened 5 years ago

Rasenshurikenbum commented 5 years ago

Steps to reproduce:

  1. Download this sample project: https://www.codeproject.com/KB/WPF/483507/Version_03_Edi_RecentFilesTW.zip
  2. Update AvalonDock version to 3.5.12.
  3. Drag 'Recent Files' anchorable window and drop it to the document's pane.
  4. Drag 'File Stats' anchorable window and drop it to the document's pane.
  5. Close 'File Stats' with X.
  6. Close 'Recent Files' with X.
  7. Choose 'Tools' from the menu and click 'RecentFiles'.
  8. It throws: System.InvalidCastException: 'Unable to cast object of type 'Xceed.Wpf.AvalonDock.Layout.LayoutAnchorable' to type 'Xceed.Wpf.AvalonDock.Layout.ILayoutPanelElement'.'

Other exception:

  1. Repeat step 1-6.
  2. Choose 'Tools' from the menu and click 'Properties'.
  3. Repeat step 2.
  4. It throws: System.ArgumentOutOfRangeException: 'Index must be within the bounds of the List. Parameter name: index'
Dirkster99 commented 5 years ago

Hi, would you please attach the updated version that works with 3.5.12 to verify your problem using your test client?

Thank you.

Rasenshurikenbum commented 5 years ago

If you meant to upload my test project then there it is. Edi_AvalonDock_v3.5.12.zip

Dirkster99 commented 5 years ago

hmmm, this is strange. I cannot verify your prolem although your workflow description is detailled and easy to follow. Is it possible that you used an older than 3.5.12 version and did not update correctly to the recent version (because there was a recent fix in 3.5.11 about this)?

Rasenshurikenbum commented 5 years ago

I tried both overwriting old version and installing a fresh one. Every time I get the same results. I also found out that the first exception occurs in all versions from 3.5.4 up. In version 3.5.3 no exception is thrown but instead it doesn't let the closed window to be shown again (choosing the proper view from menu does nothing). What does it mean you cannot verify my problem? Have you tried the project I sent before?

Dirkster99 commented 5 years ago

What does it mean you cannot verify my problem?

I have tested the application that you attached and it works for me without any exception being thrown. Its really strange - not sure how to fix a problem I cannot verify on my computer :-(

Would you please give me the full Statcktrace of the exception that you see? It should normally look like the Stacktrace in this Issue. Thanx

Rasenshurikenbum commented 5 years ago

Here's the stack trace:

System.InvalidCastException HResult=0x80004002 Message=Unable to cast object of type 'Xceed.Wpf.AvalonDock.Layout.LayoutAnchorable' to type 'Xceed.Wpf.AvalonDock.Layout.ILayoutPanelElement'. Source=Xceed.Wpf.AvalonDock StackTrace: at Xceed.Wpf.AvalonDock.Layout.LayoutGroup1.InsertChildAt(Int32 index, ILayoutElement element) at Xceed.Wpf.AvalonDock.Layout.LayoutAnchorable.Show() at Xceed.Wpf.AvalonDock.Controls.LayoutAnchorableItem.OnVisibilityChanged() at Xceed.Wpf.AvalonDock.Controls.LayoutItem.<>c.<.cctor>b__13_1(DependencyObject s, DependencyPropertyChangedEventArgs e) at System.Windows.PropertyChangedCallback.Invoke(DependencyObject d, DependencyPropertyChangedEventArgs e) at System.Windows.DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e) at System.Windows.FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e) at System.Windows.DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args) at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType) at System.Windows.DependencyObject.InvalidateProperty(DependencyProperty dp, Boolean preserveCurrentValue) at System.Windows.Data.BindingExpressionBase.Invalidate(Boolean isASubPropertyChange) at System.Windows.Data.BindingExpression.TransferValue(Object newValue, Boolean isASubPropertyChange) at MS.Internal.Data.ClrBindingWorker.NewValueAvailable(Boolean dependencySourcesChanged, Boolean initialValue, Boolean isASubPropertyChange) at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView, Object newValue, Boolean isASubPropertyChange) at MS.Internal.Data.ClrBindingWorker.OnSourcePropertyChanged(Object o, String propName) at System.Windows.WeakEventManager.ListenerList1.DeliverEvent(Object sender, EventArgs e, Type managerType) at System.ComponentModel.PropertyChangedEventManager.OnPropertyChanged(Object sender, PropertyChangedEventArgs args) at Edi.ViewModel.Base.ViewModelBase.RaisePropertyChanged(String propertyName) in C:\Users\Krzysztof\Desktop\Edi_AvalonDock_v3.5.12\Edi\ViewModel\Base\ViewModelBase.cs:line 13 at Edi.ViewModel.Base.ToolViewModel.set_IsVisible(Boolean value) in C:\Users\Krzysztof\Desktop\Edi_AvalonDock_v3.5.12\Edi\ViewModel\Base\ToolViewModel.cs:line 34

And here is the second exception's stack trace:

System.ArgumentOutOfRangeException HResult=0x80131502 Message=Index must be within the bounds of the List. Parameter name: index Source=mscorlib StackTrace: at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource) at System.Collections.ObjectModel.Collection1.Insert(Int32 index, T item) at Xceed.Wpf.AvalonDock.Layout.LayoutAnchorable.Show() at Xceed.Wpf.AvalonDock.Controls.LayoutAnchorableItem.OnVisibilityChanged() at Xceed.Wpf.AvalonDock.Controls.LayoutItem.<>c.<.cctor>b__13_1(DependencyObject s, DependencyPropertyChangedEventArgs e) at System.Windows.PropertyChangedCallback.Invoke(DependencyObject d, DependencyPropertyChangedEventArgs e) at System.Windows.DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e) at System.Windows.FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e) at System.Windows.DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args) at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType) at System.Windows.DependencyObject.InvalidateProperty(DependencyProperty dp, Boolean preserveCurrentValue) at System.Windows.Data.BindingExpressionBase.Invalidate(Boolean isASubPropertyChange) at System.Windows.Data.BindingExpression.TransferValue(Object newValue, Boolean isASubPropertyChange) at MS.Internal.Data.ClrBindingWorker.NewValueAvailable(Boolean dependencySourcesChanged, Boolean initialValue, Boolean isASubPropertyChange) at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView, Object newValue, Boolean isASubPropertyChange) at MS.Internal.Data.ClrBindingWorker.OnSourcePropertyChanged(Object o, String propName) at System.Windows.WeakEventManager.ListenerList1.DeliverEvent(Object sender, EventArgs e, Type managerType) at System.ComponentModel.PropertyChangedEventManager.OnPropertyChanged(Object sender, PropertyChangedEventArgs args) at Edi.ViewModel.Base.ViewModelBase.RaisePropertyChanged(String propertyName) in C:\Users\Krzysztof\Desktop\Edi_AvalonDock_v3.5.12\Edi\ViewModel\Base\ViewModelBase.cs:line 13 at Edi.ViewModel.Base.ToolViewModel.set_IsVisible(Boolean value) in C:\Users\Krzysztof\Desktop\Edi_AvalonDock_v3.5.12\Edi\ViewModel\Base\ToolViewModel.cs:line 34

Dirkster99 commented 5 years ago

OK, I tried it again and I am able to also get the exception. It is important to close the tool window with X because I previously made the mistake of closing it via Tools>... menu entry and it did work in this instace. Now I can look for a fix ... thank you for you patience

Rasenshurikenbum commented 5 years ago

Awesome, hope it's not something difficult to fix.

Dirkster99 commented 4 years ago

hmmm, I've been looking at this for a while but cannot seem to determine why the exception occurs only if I close the document with 'X' and I also noticed that it only occurs for the first tool window that is dragged into the document pane (?) but the workflow actually works for the 2nd tool window:

1 Using the attached sample project (with LayoutInitializer disabled for testing for above URL): 2 Update AvalonDock version to 4.0 3 Drag 'Recent Files' anchorable window and drop it to the document's pane. 4 Drag 'File Stats' anchorable window and drop it to the document's pane. 5 Close 'File Stats' with X. 6 Close 'Recent Files' with X. 7 Choose 'Tools' from the menu and click 'RecentFiles'. Throws System.InvalidCastException: 'Unable to cast object of type 'AvalonDock.Layout.LayoutAnchorable' to type AvalonDock.Layout.ILayoutPanelElement'.' is thrown in LayoutAnchorable.Show() at about line 449 where this is to be inserted in the previousContainerAsLayoutGroup

Repeat steps 1-4 5 Close 'Recent Files' with X. 6 Close 'File Stats' with X. 7 Choose 'Tools' from the menu and click 'RecentFiles' (no Exception is thrown - tool window re-appears in document pane after switching the property) 8 Choose 'Tools' from the menu and click 'Properties' Throws the above exception but on a different tool window

I also noticed that the PreviousContainerIndex can be -1 in the InsertAt statement so the code below could be a minor improvement in LayoutAnchorable.show() at about line 447 (but its not fixing the problem reported here):

var previousContainerAsLayoutGroup = PreviousContainer as ILayoutGroup;
if (PreviousContainerIndex < previousContainerAsLayoutGroup.ChildrenCount)
    previousContainerAsLayoutGroup.InsertChildAt((PreviousContainerIndex < 0 ? 0 : PreviousContainerIndex), this);
else
    previousContainerAsLayoutGroup.InsertChildAt(previousContainerAsLayoutGroup.ChildrenCount, this);

Whats really bothering me (because I cannot explain it) is why the same code works on one of the tool windows but not for the other even though conditions seem to be the same in both instances (inserting a LayoutAnchorable into a ILayoutGroup) but one of them ends in an exception(!)...

@scdmitryvodich Do you see what I am missing? Why is this exception one of the tool windows and not on both?

Version_03_Edi_RecentFilesTW.zip

Muhahe commented 4 years ago

@Dirkster99 i noticed this exception too, and trying to figure out some workaround. But im curious why "close" button on LayoutAnchorable works like close instead of hide. Afaik Anchorables in anchorablePanel has "hide" button, so is there some reason that Anchorable will close?

If no, then i think that replacing Close behavior for Hide behavior for LayoutAnchorables in docoment panel should work.

Also i noticed that closing and reopening of layout anchorable loses layout position. But hiding and unhiding preserves locations. So its another plus for me.

Muhahe commented 4 years ago

@Dirkster99 @Rasenshurikenbum so as i mentioned in post above, i tried to replace CloseCommand with HideCommand for LayoutAnchorableItem.cs row 198

protected override void Close()
{
    if (_anchorable.Root?.Manager == null) return;
    var dockingManager = _anchorable.Root.Manager;
        //removed original closeCommand
    //dockingManager._ExecuteCloseCommand(_anchorable);
       //replaced by hideCommand
    dockingManager._ExecuteHideCommand(_anchorable);
}

And its worked for me. But i dont know if there are other side effects

Also as @Dirkster99 asked did you noticed that when closing recentFiles previous container is set to LayoutDocumentPanel but when closing FileStats, previous container is set to LayoutDocumentPanel too, but right after is set to LayoutPanel? This seems strange to me and possible source of this exception.

Because when you try to restore layout after, there is wrong LayoutGroup with T type LayoutPanel which cant cast LayoutAnchorable

Dirkster99 commented 4 years ago

Hi Muhahe,

I've updated the test client to current sources and can still see the Exceptions previously described :-(

Version_03_Edi_RecentFilesTW_AD40_20200304.zip

Replacing the Close with Hide command is a good hint but seems to be a strange solution (because this works for many other workflows).

Maybe there is a better fix such as calling Hide instead of Close under a certain condition?

At this point I am not sure but will try to look into this but am not sure if I can find a better fix either...

amolf-se commented 4 years ago

Hi All,

Here is my point of view on this. I think that hiding when close is requested is a bad idea, this breaks the intention. There are already AutoHide, CanClose properties to influence the behavior. If you don't want the anchorable to be closed, set CanClose to false. By using these settings the error is probably bypassed.

The actual error occurs somewhere else. When closing, the CloseInternal function of LayoutContent gets called. It remembers the parent as previouscontainer for reinstation later. on line 706 it checks that if it was the last one in the parent, it moves itself up in the layout tree by setting previouscontainer to it's grandparent (a LayoutPanel), by assumption that the parent will be garbage collected. This will not happen because in the garbage collection routine (LayoutRoot.CollectGarbage) there is an exception for the LayoutDocumentPane of the main view (line 379). The grandparent (LayoutPane) does not accept an LayoutAnchorable or LayoutDocument, because the do not implement ILayoutPanelElement. So in my opinion the code in LayoutContent.CloseInternal should be adapted to keep the parent if it is the 'main' documentpane. I'm not sure this will cover all the situations with floating windows correctly. I hope someone can do the actual changes and various tests.

Dirkster99 commented 4 years ago

@mkonijnenburg Your analysis is right :-) as usual :-)

I created a branch to make the current solution available for testing and review: https://github.com/Dirkster99/AvalonDock/tree/Issue_71

I created a new internal bool IsParentRemoved(ILayoutContainer parent) method and its now getting called by the:

to make this dependency more obvious and ensure consistency in the result. Otherwise, the move up the parent, grandparent chain is only done if IsParentRemoved indicates that situation 👍 so this fixes the exceptions.

The only thing remaining to close this issue is the broken binding which is visible when I close the Anchorable in the DocumentPane with the (X) button and look at the Menu Entry which shows the Anchorable still being checked :-(

Dirkster99 commented 4 years ago

I found a fix in LayoutAnchorable.cs in this commit which is essentally setting the IsVisible property to false when closing the LayoutAnchorable.

When testing, I am finding another related issue which is that the LayoutAnchorable in the DocumentPane reloads without showing the Close (X) button probably because the wrong control is applied in this case(?)

The Close and Close All, Close But this in the Menu Context (which otherwise work) are also not working for the Layout reload case.

Untitled

TBD I am not sure whether this is a good solution to start with. Users clicking on the (X) of a tool window probably expect a consistent behavior, so we should call HideCommand instead of CloseCommand when clicking the (X) on a LayoutAnchorable in the DocumentPane (See Also Close and Close All But This in context menu), right?

PS' I tried this feature in all previous versions down to verions 2.0 and it has never workd before. So. its more like implementing a new feature rather than fixing a real but ....

LyonJack commented 4 years ago

1593571267(1) I have try resolve this problem，make sure these code will work

Dirkster99 commented 4 years ago

I have tested this fix (as it is already commited in master) but it does not solve the problem reference in this workflow. I am not sure how this change could be related to the exceptions that are visible when the above workflow is executed?

LyonJack commented 4 years ago

Hi @Dirkster99 This is a mistake of mine. I confused this problem with the one of the others #71

Dirkster99 commented 4 years ago

Yes this is a confusing issue ticket. The resolution is probably very difficult and involved (which is why I was not able to resolve it yet). From what I saw it looks like:

There also seems to be an inconsistency between Close()and Hide() being applied to the LayoutAnchorable as discussed here. It looks like:

Dirkster99 commented 4 years ago

@LyonJack What Do you mean by this?

This is a mistake of mine. I confused this problem with the one of the others #71

Which one is the other issue (because 71 is the issue you are looking at)?

LyonJack commented 4 years ago

Yes, I was confused when dealing with the problem of #71 This problem continues to open

LyonJack commented 4 years ago

image

LyonJack commented 4 years ago

Hi @Rasenshurikenbum, I try to fix this problem in the new version,and I have tried your demo,it works well.Please have a test it's ok now.I dont know the version map with git 3.5.12,so I just fix the new version. @Dirkster99 if this ok you can fix the old version of adock

LyonJack commented 4 years ago

Hi @Dirkster99 I found that you have made the branch management of issue. Next time I solve the corresponding problem, I will switch to the corresponding branch image

Dirkster99 commented 4 years ago

I guess the change in #181 does not hurt but it does not fix the problem in this issue :-(

@LyonJack Please have a look at this workflow - it is important close the document with 'X' since the observed errors will otherwise not occur as mentioned (I've had trouble verifying that too...)