search

gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.67k stars 688 forks source link

Get rid of `Toplevel` - Introduce `Runnable` and `Overlapped` instead #2491

Open tig opened 1 year ago

tig commented 1 year ago

This PR is for a major rehaul of how Terminal.Gui deals with non-Tiled view arrangements and "runnable" views.

@bdisp, @tznind, @dodexahedron, and @migueldeicaza - PLEASE READ AND TURN ON YOUR BRAINSTORM MODE

We have a chance to get the design of how all this works in v2 right. We should approach this throwing away existing notions of how Terminal.Gui has always worked in these regards. This does not mean we WILL throw all those things away, but that we should not constrain our thinking at this point.

I've written the below in the spirit of generating debate and ideas. I'm highly motivated to start working on this as I've been noodling on it for years. But my thinking is still very far from clear.

We will need a new "Multiple overlapped, MDI-like views" example (see #3636).

Related Issues & PRs

Background

In v2, we have the following layout abilities:

Assertions (these are up for challenge/debate!)

Keyboard Nav

See https://github.com/tig/Terminal.Gui/blob/8e70e2ae8faafab7cb8305ec6dbbd552c7bf3a43/docfx/docs/navigation.md

Requirements

Justification / Issues with current systems (very old text; ignore if you want)

Overlapped is partially supported in v1 using Toplevel views. However, the v1 design is lacking:

tig commented 1 year ago

2544 makes progress on this.

tig commented 9 months ago

I'm Closing

And putting the discussion here to simplify things. There's no need for two issues:.

From 2502

This is all debatable, but here's my thinking:

TopLevel is currently built around several concepts that are muddled and have led to overly complex code:

Application is the place where all of the above TopLevel gunk happens in addition to other "application" stuff.

The MdiContainer stuff is complex, perhaps overly so, and is not actually used by anyone outside of the project. It's also misnamed because Terminal.Gui doesn't actually support "documents" nor does it have a full "MDI" system like Windows (did). It seems to represent features useful in overlapping Views, but it is super confusing on how this works, and the naming doesn't help. Application is full of code like if (MdiTop != null && top != MdiTop && top != Current && Current?.Running == false && !top.Running) which just screams poor-OO design and fragility. This all can be refactored to support specific scenarios and thus be simplified.

My proposal:

Making this happen requires the following to be done first:

But it seems doable, and will significantly simplify the programming model (and codebase).

What do folks think?

BDisp commented 9 months ago

In this case a Button can be Modal and run itself?

Application.Run(new Button("I'm Popeye"))
tig commented 9 months ago

In this case a Button can be Modal and run itself?

Application.Run(new Button("I'm Popeye"))

Yep, A better example (from https://github.com/gui-cs/Terminal.Gui/issues/1720).

The code here: https://github.com/gui-cs/Terminal.Gui/pull/3154

(this part):

void ColorBtnClicked (Button btn)
{
    var fgColor = blocksPB.ColorScheme.Normal.Foreground;
    var bgColor = blocksPB.ColorScheme.Normal.Background;

    var colorPicker = new ColorPicker {
        Title = btn.Text.
    };

    if (btn.Text == "Foreground") {
        colorPicker.SelectedColor = fgColor.ColorName;
    } else {
        colorPicker.SelectedColor = bgColor.ColorName;
    }

    var dialog = new Dialog {
        Title = btn.Text
    };

    dialog.LayoutComplete += (sender, args) => {
        // TODO: Replace with Dim.Auto
        dialog.Bounds = new Rect (0, 0, colorPicker.Frame.Width, colorPicker.Frame.Height);
        dialog.X = Pos.Center ();
        dialog.Y = Pos.Center ();

    };

    dialog.Add (colorPicker);
    colorPicker.ColorChanged += (s, e) => {
        dialog.RequestStop ();
    };
    Application.Run (dialog);

    if (btn.Text == "Foreground") {
        ChangeColor (colorPicker.SelectedColor, bgColor.ColorName);
    } else {
        ChangeColor (fgColor.ColorName, colorPicker.SelectedColor);
    }

    colorPicker.Dispose ();
}

Would be:

        void ColorBtnClicked (Button btn)
        {
            var fgColor = blocksPB.ColorScheme.Normal.Foreground;
            var bgColor = blocksPB.ColorScheme.Normal.Background;

            var colorPicker = new ColorPicker {
                Title = btn.Text
            };
            if (btn.Text == "Foreground") {
                colorPicker.SelectedColor = fgColor.ColorName;
            } else {
                colorPicker.SelectedColor = bgColor.ColorName;
            }
            Application.Run (colorPicker);

            if (btn.Text == "Foreground") {
                ChangeColor (colorPicker.SelectedColor, bgColor.ColorName);
            } else {
                ChangeColor (fgColor.ColorName, colorPicker.SelectedColor);
            }
        }
BDisp commented 9 months ago

I see. Well done.

tig commented 9 months ago

@bdisp pointed out in #3154 :

How about popup near the respective buttons on available space (below, up, right, left) as the ContextMenu does?

This should be a goal of the new Overlapped concept: it should be easy to say "position this overlapped view relative to this view".

More details of what we mean here:

tig commented 9 months ago

I've closed:

As a dupe of this.

tig commented 9 months ago

Related:

dodexahedron commented 2 months ago

I've got some catch-up to do, but the skimming I did just now gave me warm fuzzies.

If I have any thoughts to add, I'll do so after I finish some work stuff I'm currently at a wait phase of - hopefully later tonight.

dodexahedron commented 2 months ago

Ugh. I'm shot. Brain is too fried to give it good attention tonight.

Will take a good look tomorrow for sure, though, barring any emergencies. šŸ˜…

dodexahedron commented 2 months ago

I like a lot of what I've read so far and do have some thoughts to add to the discussion. I have to run to an appointment, but I'll work on writing stuff up when I return.

Quickly, I think the stack idea is the right spirit, but a tree is probably a better structure for what is effectively a DOM. I think modeling everything after that paradigm is the simplest thing for us to do and meshes well with a lot of built-in .net stuff and familiar concepts for a wide range of developers. I'll expand on that/refine it when I get back, though.

In particular, Stack<T> is not fun to work with when you need random access capability, as you're forced to enumerate the whole thing to get to anything not on the top. Dictionary types work really well for keyed trees though.

Now, if you also wanted to maintain a stack internally, purely for rendering purposes and not in the public api surface, that could potentially have interesting value.

dodexahedron commented 2 months ago

OK, now that I've had a couple of reads over the initial post, I can refine my earlier musing and add others...

I'm going to break it up into multiple comments, trying to at least sorta stick to one major point each, starting at the top of the "Assertions" section and working my way down.

Application/The stack stuff

Again, I like the intent here. But we don't actually have to function quite that low-level, here, since we're already on top of .net, which provides simpler abstractions for us.

Correct me if I'm off base, but my reading of that is that it's intended/aimed at how we carry out both dispatching and handling of "messages" (which I'll use here in place of the word "event" so that I can use "event" to mean a C#/.net event instead), such as user input messages, property change messages, etc.

If that reading is essentially correct, then that's one big place we can lean on .net and events. We already do, to an extent, but it's not a consistently applied concept or implementation, at present (though so far V2 has made significant improvements there). A big hurdle and potential pain point for consumers, with how it is now, is that there's not really a well-defined flow or order of things, that can be relied upon universally.

The stack or tree concept of course makes good sense in that regard, insofar as it provides a defined root from which we can start and search until something is handled (or not).

The logical conclusion of that line of thinking winds up being that we should have an actual DOM for the UI. I think that high-level concept, in general, actually addresses a lot of pain points we have or would like to address, and for more than just message passing.

What I mean by not having to operate low-level is that we have events and we have more flexible data structures than stack at our disposal. Yes, a DOM has a single root, but that's the only mandatory standalone element. Even something as simple as XML has a single root, but then is a tree from there on.

Where I think we can improve/rework things to fix it

Keeping the concept of nodes in the DOM very high level and generic is a necessity, there, and enabling easy access and navigation through the tree is a highly likely and very reasonable expectation from a consumer. Most frameworks expose it very simply, as the tree it is, with every node having a reference to its parent as well as a collection of references to children (also terms I really think we should adopt instead of how we say SuperView and SubView, which is the same concept but different for no apparent value-add). A good template to follow is probably the way XML works - an XmlNode can contain 0 to n XmlNodes, and the specifics of them are implementation details of the document type definition or schema - not a definition set by XML itself, which remains completely content-agnostic as long as the syntax is valid.

The specifics of those data structures would of course be one of the first discussion points, and I'd suggest use of collections in System.Collections.Concurrent as a proactive measure to aid thread-safety with minimal overhead cost. In particular, ConcurrentDictionary is pretty ideal for a tree node's "Children" collection, as it enables both enumeration and indexed/keyed access. What are the keys? Probably strings, but care just has to be taken with them if, for example, they are synced to the "names" of the nodes.

Whatever the structure, care also has to be taken to properly manage the lifetime of nodes. For example, if a user disposes a node, the base Dispose implementation could be responsible for making sure the reference is removed from its parent and that all of that disposed node's children are also dealt with appropriately. That specific examples is a perhaps draconian and simple one, but the point is there are things we need to either handle or at least not fail catastrophically or indeterminately on, to help avoid us facilitating a consumer getting into a bad program state that shouldn't have been allowed to happen in the first place.

Alrighty... Going to take a little break for a while or until tomorrow and then come back for the next part, since I don't want to venture too far into implementation at this point in the process.

And of course if I misread you, then the above is obviously not directly a response to that bullet point, but I do believe the general concepts stand regardless.

tig commented 2 months ago

And of course if I misread you, then the above is obviously not directly a response to that bullet point, but I do believe the general concepts stand regardless.

My intent is not to completely rearchitect how the TG View hierarchy works. The tree model based on Subviews and Superview is not going anywhere in v2. What you are discussing is a much much deeper change than what I'm willing to take on for v2.

What you're proposing all makes good sense and is smart. But not for now.

What this PR is about is the following:

When Miguel built gui-cs, he decided to have a View subclass called Toplevel that combined three concepts: Supported being "run", with a complete "run state". Effectively a mini application within the process started when dotnet run ran. Multiple Toplevels could be run NESTED. The second thing Toplevel did was support being the root of a View hierarchy and things you'd expect to happen for the root of such a hierarchy were done within the Toplevel class (e.g. event dispatch). The third was support for "PopUp" windows (aka Modals aka Dialog).

Later, a bunch of other functionalities was added to TopLevel. The Most pronounced was the idea of multiple, non-modal, overlapped Toplevels. This was introduced by @bdisp in the form of MdiContainer etc... which was renamed to Overlapped in early v2 branches. With this we got the idea of movable overlapped views and a z-order. It worked, but because @BDisp was constrained with not breaking the core-API he had to slam it all into both Toplevel and Application. It is not a clean architecture, is fragile, is a bug farm, and actually makes it hard to implement new things in v2 (e.g. for a View to be overlapped it must inherit from Toplevel).

Here are examples of code in Application and Toplevel that highlight how complex and fragile this stuff is. This code is at the CORE of TG and it's freaking scary to look at. I want to be clear that I'm criticizing the code here, not who wrote it. I am responsible for a bunch of this too (to retain API compat we had no choice but to continue to hack at it).

image

image

image

This issue, and the PR's I've started for it are about cleaning this mess up. It is about decoupling "runnable views", "event dispatch at the top of the hierarchy", and "overlapped" from Toplevel which will result in Toplevel going away.

You could (and I think you are) argue that TG's model for event dispatch and View hierarchy could be redesigned too. I don't disagree. But that is not what we're doing here.

Make sense?

dodexahedron commented 2 months ago

And of course if I misread you, then the above is obviously not directly a response to that bullet point, but I do believe the general concepts stand regardless.

My intent is not to completely rearchitect how the TG View hierarchy works. The tree model based on Subviews and Superview is not going anywhere in v2. What you are discussing is a much much deeper change than what I'm willing to take on for v2.

Ok, phew. I was a bit concerned for exactly that reason, at this stage of the game for v2. šŸ˜…

I'm dealing with some server rebuilds at the moment, so this is just an ack. I'll read over this in detail when I'm done.

dodexahedron commented 2 months ago

Did you mean for this to get closed with that PR?

Re-opening in case not. Go ahead and close if you want.

Might be a good idea to continue the conversation elsewhere, though?

BDisp commented 2 months ago

Did you mean for this to get closed with that PR?

Re-opening in case not. Go ahead and close if you want.

Might be a good idea to continue the conversation elsewhere, though?

This wasn't closed intentional by @tig but by the use of Fixes #2491 in #3634 when was merged.

tig commented 2 months ago

Did you mean for this to get closed with that PR? Re-opening in case not. Go ahead and close if you want. Might be a good idea to continue the conversation elsewhere, though?

This wasn't closed intentional by @tig but by the use of Fixes #2491 in #3634 when was merged.

Yea. Lesson learned. Dont write "Partially Fixes #xxx". Write "Fixes (Partially) #xxx".

dodexahedron commented 2 months ago

Ha yeah. There are a bunch of "closing comments" that cause that kind of automatic action.

I think I neglected to mention that when I made a side remark about the issue title convention we use, probably like a year ago. šŸ˜…šŸ¤·ā€ā™‚ļø

Fixes, closes, resolves, and I think a few other words in combination with an issue number will close automatically. Only in English, I believe, or at least last time I saw the docs for it I'm pretty sure that was part of it. You know: In case you want to use ancient Egyptian or something. šŸ˜

tig commented 2 months ago

And of course if I misread you, then the above is obviously not directly a response to that bullet point, but I do believe the general concepts stand regardless.

My intent is not to completely rearchitect how the TG View hierarchy works. The tree model based on Subviews and Superview is not going anywhere in v2. What you are discussing is a much much deeper change than what I'm willing to take on for v2.

Ok, phew. I was a bit concerned for exactly that reason, at this stage of the game for v2. šŸ˜…

I'm dealing with some server rebuilds at the moment, so this is just an ack. I'll read over this in detail when I'm done.

BTW, your comments above regarding a "DOM" DID get me thinking differently and I wanted to acknowledge that.

I still am very much against having v2 be where we add a DOM to Terminal.Gui.

However, I now see clear steps we can take in that direction that will enable such a thing post-v2.

Some notes I wrote on this yesterday, that I thought i'd share:

Focus Chain & DOM ideas

The navigation/focus code in View.Navigation.cs has been rewritten in v2 (in https://github.com/gui-cs/Terminal.Gui/pull/3627) to simplify and make more robust.

The design is fundamentally the same as in v1: The logic for tracking and updating the focus chain is based on recursion up and down the View.Subviews/View.SuperView hierarchy. In this model, there is the need for tracking state during recursion, leading to APIs like the following:

// From v1/early v2: Note the `force` param.
private void SetHasFocus (bool newHasFocus, View view, bool force = false)

// From #3627: Note the `traversingUp` param
 private bool EnterFocus ([CanBeNull] View leavingView, bool traversingUp = false)

The need for these "special-case trackers" is clear evidence of poor architecture. Both implementations work, and the #3627 version is far cleaner, but a better design could result in further simplification.

For example, moving to a model where Application is responsible for tracking and updating the focus chain instead View. We would introduce a formalization of the Focus Chain.

Focus Chain: A sequence or hierarchy of UI elements (Views) that determines the order in which keyboard focus is navigated within an application. This chain represents the potential paths that focus can take, ensuring that each element can be reached through keyboard navigation. Instead of using recursion, the Focus Chain can be implemented using lists or trees to maintain and update the focus state efficiently at the Application level.

By using lists or trees, you can manage the focus state without the need for recursive traversal, making the navigation model more scalable and easier to maintain. This approach allows you to explicitly define the order and structure of focusable elements, providing greater control over the navigation flow.

Now, the interesting thing about this, is it really starts to look like a DOM!

Designing a DOM for UI library involves creating a structured representation of the UI elements and their relationships.

  1. Hierarchy and Structure- Root Node: The top-level node representing the entire application or window.
    • View Nodes: Each UI element (View) is a node in the DOM. These nodes can have child nodes, representing nested or contained elements.
  2. Node Properties- Attributes: Each node can have attributes such as id, class, style, and custom properties specific to the View.
    • State: Nodes can maintain state information, such as whether they are focused, visible, enabled, etc.
  3. Traversal Methods- Parent-Child Relationships: Nodes maintain references to their parent and children, allowing traversal up and down the hierarchy.
    • Sibling Relationships: Nodes can also maintain references to their previous and next siblings for easier navigation.
  4. Event Handling- Event Listeners: Nodes can have event listeners attached to handle user interactions like clicks, key presses, and focus changes.
    • Event Propagation: Events can propagate through the DOM, allowing for capturing and bubbling phases similar to web DOM events.
  5. Focus Management- Focus Chain: Maintain a list or tree of focusable nodes to manage keyboard navigation efficiently.
    • Focus Methods: Methods to programmatically set and get focus, ensuring the correct element is focused based on user actions or application logic.
  6. Mouse Events - Mouse handling in Terminal.Gui involves capturing and responding to mouse events such as clicks, drags, and scrolls. In v2, mouse events are managed at the View level, but for a DOM-like structure, this should be centralized.
  7. Layout - The Pos/Dim system in Terminal.Gui is used for defining the layout of views. It allows for dynamic positioning and sizing based on various constraints. For a DOM-model we'd maintain the Pos/Dim system but ensure the layout calculations are managed by the DOM manager.
  8. Drawing - Drawing in Terminal.Gui involves rendering text, colors, and shapes. This is handled within the View class today. In a DOM model we'd centralize the drawing logic in the DOM manager to ensure consistent rendering.

This is all well and good, however we are NOT going to fully transition to a DOM in v2. But we may start with Focus/Navigation (item 3 above). Would retain the existing external View API for focus (e.g. View.SetFocus, Focused, CanFocus, TabIndexes, etc...) but refactor the implementation of those to leverage a FocusChain (or FocusManager) at the Application level.

I do NOT plan on tackling this in #3627. Nor do I consider it needed to address THIS issue. I now have #3627 working well enough (with tons of new primitive unit tests) that the "old model" will work fine. I still have to fix a bunch of issues in built-in Views causing ./Views unit test failures, and things aren't quite working completely rigth, but all other unit tests pass.

There's a small chance that fixing these remaining issues may force me to revisit this decision. But for now...