Why is LayoutAnchorable.Parent set to PreviousContainer on hiding?

[bdachev]

Shouldn't it be that the Parent is set to null when detaching/hiding?
I can't see in history when and why have you changed that.
(see LayoutAnchorable.UpdateParentVisibility() for reference)

[Dirkster99]

Hi, I am not sure I am understanding your question.

The current version of LayoutAnchorable.UpdateParentVisibility() is this:

private void UpdateParentVisibility()
{
  // Element is Hidden since it has no parent but a previous parent
  if (PreviousContainer != null && Parent == null)
  {
      // Go back to using previous parent
      Parent = PreviousContainer;
      ////        PreviousContainer = null;
  }

  if (Parent is ILayoutElementWithVisibility parentPane)
      parentPane.ComputeVisibility();
}

This was changed in this commit
as requested by @gpetrou in PR #32. See PR for justification of change.

The previous version was part of the solution for Issue #28

private void UpdateParentVisibility()
{
  // Element is Hidden since it has no parent but a previous parent
  if (this.PreviousContainer != null && Parent == null)
  {
    // Go back to using previous parent
    Parent = PreviousContainer;
    PreviousContainer = null;
  }

  var parentPane = Parent as ILayoutElementWithVisibility;
  if( parentPane != null )
    parentPane.ComputeVisibility();
}

All previous versions back to AvalonDock 2.0 use this code:

  private void UpdateParentVisibility()
  {
    var parentPane = Parent as ILayoutElementWithVisibility;
    if( parentPane != null )
      parentPane.ComputeVisibility();
  }

Would you recommend rolling back to the previous version or do you see a bug and recommend a completely different code inside this method?

[bdachev]

Hi @Dirkster99, As a start I just wanted to understand the reason behind adding that part to UpdateParentVisibility():

			// Element is Hidden since it has no parent but a previous parent
			if (PreviousContainer != null && Parent == null)
			{
				// Go back to using previous parent
				Parent = PreviousContainer;
				////        PreviousContainer = null;
			}

Excuse me if I'm wrong but the commit you're mentioning above is a change to other place in LayoutAnchorable.cs. There is interesting thing mentioned by @gpetrou in #32 i.e. that LayoutAnchorable.Parent is guarantied not to be null. I think that should be valid only while the anchorable is shown but once it is hidden the Parent should be set to null (or at least we have a code in our project that relies on that :)).

[Dirkster99]

I realized that I've been missing a step in the history of the LayoutAnchorable.UpdateParentVisibility() method so I've added this step now in the above history. Its quit possible that the fix in #28 is not correct :-(

My problem is that I am not the original author of AvalonDock and so I am having trouble to always understand and test all side-effects - partly because there are so many different options (using MVVM, WinForms, Binding Properties can behave differently ...) and partly because there was almost no in code documentation that was worthwhile to speak of when I took over this project. I've tried to change the in-code documentation issue with the help of @mkonijnenburg but there are still some details like the PreviousContainer property that I don't fully understand, yet.

So far, we understand that the PreviousContainer property can be used to remember the Parent as previouscontainer for re-instation later. This can be used to check if an item was the last one in the parent, it can move itself up in the layout tree by setting previouscontainer to it's grandparent (a LayoutPanel), by assumption that the parent will be garbage collected.

What I don't understand, yet, are the workflows (serializing/deserializing layout and using Hide and maybe others?) and the expected behavior. It would be really great if we could fill this gap and I would be happy to roll the above change back if you could tell me how your code relies on the PreviousContainer property being null (when being hidden?)

[bdachev]

Hi Dirk, sorry for delayed reply and thank you for throwing some light on it. It seems I must have missed mentioning of #28 in your previous comment. It is actually where that change to PreviousContainer was discussed originally. It seems to me that it solves a problem of re-opening tools/anchorables when floating.
In our project we take different and more complicated approach to handle re-opening. It is a bit difficult to explain it without showing our code. We react on IsHidden property change event. Unfortunately it is raised multiple times during Hide() and there is a case when the IsHidden is still true while the tool is hiding. So in this particular case we check whether Parent property is already set to null as an indication.
Anyway we have our branch of AvalonDock in our project and compile from source so I decided to just comment out the change made in #28 so that it does not interfere with our existing code.

[Dirkster99]

Hi Boris, thanks for your feedback. But would your problem not be solved if we set:

  // Element is Hidden since it has no parent but a previous parent
  if (this.PreviousContainer != null && Parent == null)
  {
    // Go back to using previous parent
    Parent = PreviousContainer;
    PreviousContainer = null;
  }

inside UpdateParentVisibility() ?

I think rolling back to this version seems to be advisable since the change in PR #32 was rather technical and does not seem to cover all cases correctly (Hide). Would you agree? Would this change solve your problem?

[bdachev]

Hi Dirk, the original code in UpdateParentVisibility() does not change Parent at all. That is what our code expects and that is how I changed it in our local branch:

  private void UpdateParentVisibility()
  {
    var parentPane = Parent as ILayoutElementWithVisibility;
    if( parentPane != null )
      parentPane.ComputeVisibility();
  }

I did not insist you to revert the code in your fork as it seems to fix what is described in #28 and #32. We do some other stuff in our project/branch to allow for floating tool/anchorable windows to be hidden and reopen. For example we change Close button to execute HideCommand instead of CloseCommand in case of an achorable. I can prepare push request if you like but not sure how useful it will be in general case.
B.

[Dirkster99]

Hi Boris,

whether to Hide or to Close is actually an interesting question. I came across the same in issue #71 where I am thinking that Hide is more appropriate than Close since it is more consistent.

I just don't see why this is inconsistent to the MainWindow docked behavior of LayoutAnchorables.

Would you also advocate for Hide in the case of #71? If yes, do you happen to know a good way for re-aligning the Close command into Hide for this DocumentPane case?

I think the PR for the floating window anchorable sounds interesting because it could make all 3 LayoutAnchorable places (LayoutAnchorable Docks in MainWindow, DocumentPane, and Floating Window) consistent :-)

Drk

[Dirkster99]

Hi Boris,

I was wondering if you are still willing to prepare the PR to change the CloseCommand into a Hide command and whether (for consistency sake) we should not do the same with issue #71?

[bdachev]

Hi Dirk, sorry for not answering lately but I've been busy on other tasks. I plan to be back on AvalonDock integration in our project as part of it I will try to prepare PR with changes that we did regarding docking anchorables in document area. I will have a look at what happens in #71, if we have experienced the same and if so how we have solved it.

[Dirkster99]

I tried to verify your problem about the IsHidden property changed being raised multiple times so I added the Debug.WriteLine below line in LayoutAnchorable.cs and I can see only one change per Anchorable being hidden. If you have a special case for this maybe we should open a separate issue for this? We could also include an event for this?

public void Hide(bool cancelable = true)
{
  if (!IsVisible)
  {
    IsSelected = true;
    IsActive = true;
    return;
  }

  if (cancelable)
  {
    var args = new CancelEventArgs();
    OnHiding(args);
    if (args.Cancel) return;
  }

  RaisePropertyChanging(nameof(IsHidden));
  RaisePropertyChanging(nameof(IsVisible));
  if (Parent is ILayoutGroup)
  {
    var parentAsGroup = Parent as ILayoutGroup;
    PreviousContainer = parentAsGroup;
    PreviousContainerIndex = parentAsGroup.IndexOfChild(this);
  }
  System.Diagnostics.Debug.WriteLine("IsHidden {0}", DateTime.Now);
  Root?.Hidden?.Add(this);
  RaisePropertyChanged(nameof(IsVisible));
  RaisePropertyChanged(nameof(IsHidden));
  NotifyIsVisibleChanged();
}

[bdachev]

Correct me if I am wrong but with that code you actually check the number of Hide() calls per anchorable and not the number of IsHidden property change notifications that occur while hiding. As I said before we react on IsHidden property changed in our code and it fails because it expects Parent to be null when hidden.

[Dirkster99]

You are of course correct. I assumed that all hidden changes should go through this method :-( maybe we should open a separate issue for this with a small demo app exibiting the problem in the client so we can solve this problem and make sure there is only 1 IsHidden property change notification per Hidden process...