gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.68k stars 689 forks source link

Changing `Visible` does not effect `Subviews`'s `Visible` #3703

Closed tig closed 1 month ago

tig commented 2 months ago

I was today years old when I noticed that superView.Visible = x does not set Visible on the subviews to x.

We have a hacky internal API, View.CanBeVisible(view), apparently, because of this.

Enabled propogates.

Shouldn't Visible too?

I'd really like to be able to use overllapped.Visible = false to "close" an overlapped that might be "reopened".

Note, I think this means that there's tons of places where stuff is currently happening that shouldn't because a View's superview is hidden, but the View isn't.

tig commented 2 months ago

A reasonable workaround that will work for toggling the visibility of an overlapped view (or any view) is to:

overlapped.Visible = !overlapped.Visible;
overlapped.Enabled = overlapped.Visible;
dodexahedron commented 2 months ago

I don't think cascading setting the Visible property explicitly would be a good idea. That would wreck whatever state someone has set up.

It all goes back to the drawing logic.

Whether or not something is visible is typically expected to be a recursive lookup from this to whatever contains this, all the way to root. Anything being not visible causes any descendants of that object in the tree to also be not visible. And the tree walk can abort as soon as it hits the first false.

You can do it with one property, but it's better (and common) to do it with two:

Visible/Visibility: The desired visibility of an item. Basically a direct get/set on the backing field of this, only. No recursion involved here.

IsVisible: A non-nullable boolean, get-only, computed property which takes the logical AND of (whether the backing field for Visible would be visible) && (IsVisible property of the parent of this in the tree).

The problem with the one-bool approach we have now is that it does not distinguish between intent and reality, and it can result in getting a different value back from Visible.get than you just fed to Visible.set, which is something that should generally be avoided outside of error conditions (in which case you're supposed to preserve the previous value anyway).

CanBeVisible is, indeed, a hack, and the above suggestion fixes the problem in fewer lines of code.

Some frameworks use a tri-state to enable visible/invisible/inherit behavior, but if you think about it, that's actually redundant and needless complexity because true has the effect of inherit - if the parent/SuperView is not visible, you'll still be invisible anyway.

dodexahedron commented 2 months ago

An IsVisible recursive property is a one-liner and it goes in the base View class:

public bool IsVisible => _visible && SuperView is not { IsVisible: false };

That returns true if _visible is true AND SuperView is either null (which means this is the root of the tree) or has IsVisible==true.

No loops, no statics - Just a simple recursive property that short-circuits false as soon as it finds a false.

dodexahedron commented 2 months ago

Now... If you want to extend that concept to communicate to descendants in the tree of an actual visibility change, due to an ancestor's Visible property changing causing the computed IsVisible property to potentially have changed, that's a separate matter and should be handled by an event subscription, either internally (like during View.Add) or by the consumer, IF they want that behavior. I kinda lean toward the latter, but there's a perfectly reasonable argument to be made in favor of the former, too, so long as it can be disabled/canceled by a consumer.

But... That's what INPC is for. Just add an additional call to PropertyChanged("IsVisible") on change of Visible, if the computed IsVisible has also changed. Then anyone who cares can react if they want.

tznind commented 2 months ago

I also think Visible should not cascade as a property. It leads to weirdness e.g.

Add child to parent Set parent visible false (if cascade child would now be visible false) Remove child Set parent visible true (cascade misses because child was removed) Add child

dodexahedron commented 2 months ago

Exactly.

Or consider any situation where the consumer has explicitly set various sibling Views in the same SuperView to different Visible states and expects that to be respected when the SuperView is hidden or shown.

Heck, we could even potentially have things in TG itself which want to take advantage of that, for sake of simplicity, such as showing and hiding borders or context menus without having to rebuild them, yet still be able to just skip doing anything related to them while drawing by checking that one simple property.

BDisp commented 2 months ago

IIRC when I created the CanBeVisible method was due the fact the unneeded propagation to the sub-views properties because when Visible is false then any drawn, except to cleaning if NeedsDisplay is true, is done on it nor on the sub-views, even any of them have Visible as true. The same behavior to keystrokes or mouse events. When I implemented the Enable property were the needed to propagate it because they are always drawn and receive keyboard and mouse events if the CanFocus is true. So if a sub-view that has Enable as true is added to a super-view that has Enable as false, then the sub-view will set Enable as false but backing in the field _oldEnabled as true. So, the View.Add method controls this things there as well. Thus I also have to same opinion as @tznind that the Visible mustn't cascade as a property. Also I'm not very fan of left object created but not used always in the memory with Visible as false, if they may never been used during the execution app. For example, the MenuBar is always visible during the app execution and need to have the shortcuts in the object to run any action. But doesn't have various Menu loaded in the memory. Only open and close as needed. I think this is more preferable than having a bunch of invisible objects in the memory that can consume much more recurses than loading and unloading objects.

tig commented 1 month ago

Closing by design.